Add some reality checks for injected bundle paths
Created attachment 273929 [details] Patch
The patch I attached is a first cut. Not sure that silent failure is good enough for ease of understanding this behavior; might be better if the UI process threw an exception when this happened for easier debugging, or at least maybe something should go into a log or console. Requiring that the bundle be in the resource directory might break some existing clients: perhaps they put the bundle somewhere else inside the app's bundle. No test coverage yet; I hope we have some injected bundle tests already, but I am not sure we do.
<rdar://problem/13353592>
Comment on attachment 273929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273929&action=review > Source/WebCore/platform/mac/FileSystemMac.mm:106 > + return [[NSURL fileURLWithPath:path isDirectory:YES] getResourceValue:&quarantineDictionary forKey:NSURLQuarantinePropertiesKey error:nullptr] How did you test this code? I'm not convinced that this is the right check. When Gatekeeper adds "user approved" quarantine flag, then this API reports that the file is not quarantined. It does that surprisingly often without actual user confirmation, so we had to use a lower level API elsewhere. > Source/WebKit2/WebProcess/WebProcess.cpp:273 > + if (parameters.injectedBundlePath.startsWith(parameters.uiProcessBundleResourcePath)) > + return false; As you commented elsewhere, this check doesn't add much, because UI process can lie about its resource path. There is probably a way to get this information from libxpc. > Source/WebKit2/WebProcess/WebProcess.cpp:274 > + // It's never a good idea to allow injected bundles that are quarantined; likely indicates fraud. I think that this is too strict. We often send archives of built WebKit around, expecting people to use WebKitTestRunner. But WebKitTestRunner's injected bundle would be quarantined when received from outside. Perhaps we should allow quarantined injected bundle when WebKit XPC binary itself is quarantined. Or maybe only do this check when WebKit is in /System/Library. This needs to be tested.
Agreed that there should be logging. Additionally, I'd make WebProcess crash - if we believe that it's under attack, there is no reason to continue, and if the check is not right for some existing clients, letting them continue without injected bundle seems unlikely to be useful.
Comment on attachment 273929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273929&action=review >> Source/WebCore/platform/mac/FileSystemMac.mm:106 >> + return [[NSURL fileURLWithPath:path isDirectory:YES] getResourceValue:&quarantineDictionary forKey:NSURLQuarantinePropertiesKey error:nullptr] > > How did you test this code? > > I'm not convinced that this is the right check. When Gatekeeper adds "user approved" quarantine flag, then this API reports that the file is not quarantined. It does that surprisingly often without actual user confirmation, so we had to use a lower level API elsewhere. Don’t we want to respect the "user approved" flag?
Comment on attachment 273929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273929&action=review >>> Source/WebCore/platform/mac/FileSystemMac.mm:106 >>> + return [[NSURL fileURLWithPath:path isDirectory:YES] getResourceValue:&quarantineDictionary forKey:NSURLQuarantinePropertiesKey error:nullptr] >> >> How did you test this code? >> >> I'm not convinced that this is the right check. When Gatekeeper adds "user approved" quarantine flag, then this API reports that the file is not quarantined. It does that surprisingly often without actual user confirmation, so we had to use a lower level API elsewhere. > > Don’t we want to respect the "user approved" flag? I think we do want to respect the user approved flag here. Checking with security experts at Apple.