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.
*** Bug 202540 has been marked as a duplicate of this bug. ***
Created attachment 380578 [details] Patch
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?
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?
Created attachment 380659 [details] Patch
(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!
(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!
Created attachment 380662 [details] Patch
rdar://problem/55957373
> 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.
Created attachment 380674 [details] Patch
(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!
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.
Created attachment 380791 [details] Patch
(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!
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.
(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!
Comment on attachment 380791 [details] Patch Clearing flags on attachment: 380791 Committed r251087: <https://trac.webkit.org/changeset/251087>
All reviewed patches have been landed. Closing bug.
*** Bug 200941 has been marked as a duplicate of this bug. ***