[macOS] Sandbox extensions should be created with audit tokens, not PIDs

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

When creating sandbox extensions targeting a specific process, audit tokens should be used instead of PIDs.

Also, an API test should be created to test this.

[Per Arne Vollan]

*** Bug 202540 has been marked as a duplicate of this bug. ***

[Per Arne Vollan]

Created attachment 380578 [details]
Patch

[Brent Fulgham]

Comment on attachment 380578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380578&action=review

> Source/WTF/wtf/Platform.h:1524
> +#define HAVE_SANDBOX_ISSUE_MACH_EXTENSION_TO_PROCESS_BY_AUDIT_TOKEN 1

More secure, and works on more iOS targets. Win-win! :-)

> Source/WTF/wtf/Platform.h:1527
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)

Looks like we were overly pessimistic about macOS support in the last patch.

> Source/WebKit/UIProcess/WebPageProxy.cpp:1087
> +            createdExtension = SandboxExtension::createHandle(resourceDirectoryURL.fileSystemPath(), SandboxExtension::Type::ReadOnly, sandboxExtensionHandle);

Is it correct to create the sandbox extension if we do not have an audit token? I'm not sure under what conditions an audit token would be unavailable.

> Source/WebKit/UIProcess/WebPageProxy.cpp:1136
> +    if (SandboxExtension::createHandleForReadByAuditToken(basePath, *(process.connection()->getAuditToken()), sandboxExtensionHandle))

Could 'getAuditToken()' be returning a WTF::nullopt here? Why don't we do the check for connection and audit token here, like you did at line 1108?

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:133
> +                SandboxExtension::createHandle("/", SandboxExtension::Type::ReadOnly, fileReadHandle);

Why is it okay to attempt to create a sandbox extension without an audit token here, but we do an early return on iOS?

[Alexey Proskuryakov]

Comment on attachment 380578 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380578&action=review

>> Source/WTF/wtf/Platform.h:1527
>> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)
> 
> Looks like we were overly pessimistic about macOS support in the last patch.

There are two things about (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000):

1. We no longer support building with iOS 12.
2. This check turns it off on tvOS and watchOS, is that what you intended?

[Per Arne Vollan]

Created attachment 380659 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 380578 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380578&action=review
> 
> > Source/WTF/wtf/Platform.h:1524
> > +#define HAVE_SANDBOX_ISSUE_MACH_EXTENSION_TO_PROCESS_BY_AUDIT_TOKEN 1
> 
> More secure, and works on more iOS targets. Win-win! :-)
> 
> > Source/WTF/wtf/Platform.h:1527
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)
> 
> Looks like we were overly pessimistic about macOS support in the last patch.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1087
> > +            createdExtension = SandboxExtension::createHandle(resourceDirectoryURL.fileSystemPath(), SandboxExtension::Type::ReadOnly, sandboxExtensionHandle);
> 
> Is it correct to create the sandbox extension if we do not have an audit
> token? I'm not sure under what conditions an audit token would be
> unavailable.
> 

I believe it is expected to have an audit token, and I added an assert in the latest patch. We are getting the audit token from the xpc connection, which could possibly fail, but I am not aware of any circumstances where this would fail.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:1136
> > +    if (SandboxExtension::createHandleForReadByAuditToken(basePath, *(process.connection()->getAuditToken()), sandboxExtensionHandle))
> 
> Could 'getAuditToken()' be returning a WTF::nullopt here? Why don't we do
> the check for connection and audit token here, like you did at line 1108?
> 

Fixed.

> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:133
> > +                SandboxExtension::createHandle("/", SandboxExtension::Type::ReadOnly, fileReadHandle);
> 
> Why is it okay to attempt to create a sandbox extension without an audit
> token here, but we do an early return on iOS?

It is expected that we have an audit token in both cases. As for the iOS case, we currently don't support creating a mach extension without audit token, but we could add that in order to be able to fall back.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 380578 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380578&action=review
> 
> >> Source/WTF/wtf/Platform.h:1527
> >> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000)
> > 
> > Looks like we were overly pessimistic about macOS support in the last patch.
> 
> There are two things about (PLATFORM(IOS_FAMILY) &&
> __IPHONE_OS_VERSION_MIN_REQUIRED >= 120000):
> 
> 1. We no longer support building with iOS 12.
> 2. This check turns it off on tvOS and watchOS, is that what you intended?

Would you prefer changing this to '__IPHONE_OS_VERSION_MIN_REQUIRED >= 130000'?

It was not my intention to leave out tvOS and watchOS. I added those platforms in the latest patch.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 380662 [details]
Patch

[Per Arne Vollan]

rdar://problem/55957373

[Alexey Proskuryakov]

> Would you prefer changing this to '__IPHONE_OS_VERSION_MIN_REQUIRED >= 130000'?

13.0 is the minimum version, so leaving this part out would be correct.

[Per Arne Vollan]

Created attachment 380674 [details]
Patch

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #10)
> > Would you prefer changing this to '__IPHONE_OS_VERSION_MIN_REQUIRED >= 130000'?
> 
> 13.0 is the minimum version, so leaving this part out would be correct.

I changed this to '__IPHONE_OS_VERSION_MIN_REQUIRED >= 130000' in the latest patch.

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 380674 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380674&action=review

> Source/WTF/wtf/Platform.h:1523
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)

We only support building with iOS 13 or newer, so this would be just PLATFORM(IOS).

If we need it for all of these platforms, I think we want to say PLATFORM(IOS_FAMILY)

> Source/WTF/wtf/Platform.h:1527
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)

Ditto.

[Per Arne Vollan]

Created attachment 380791 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #13)
> Comment on attachment 380674 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380674&action=review
> 
> > Source/WTF/wtf/Platform.h:1523
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
> 
> We only support building with iOS 13 or newer, so this would be just
> PLATFORM(IOS).
> 
> If we need it for all of these platforms, I think we want to say
> PLATFORM(IOS_FAMILY)
> 
> > Source/WTF/wtf/Platform.h:1527
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
> 
> Ditto.

Done!

[Brent Fulgham]

Comment on attachment 380791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380791&action=review

Looks good,

> Source/WTF/wtf/spi/darwin/SandboxSPI.h:-61
> -extern const uint32_t SANDBOX_EXTENSION_USER_INTENT;

Was this never used? Good to remove.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #16)
> Comment on attachment 380791 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380791&action=review
> 
> Looks good,
> 
> > Source/WTF/wtf/spi/darwin/SandboxSPI.h:-61
> > -extern const uint32_t SANDBOX_EXTENSION_USER_INTENT;
> 
> Was this never used? Good to remove.

It was used, but it was not strictly needed.

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 380791 [details]
Patch

Clearing flags on attachment: 380791

Committed r251087: <https://trac.webkit.org/changeset/251087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Per Arne Vollan]

*** Bug 200941 has been marked as a duplicate of this bug. ***