<rdar://problem/45945449>
Created attachment 369910 [details] Patch
Comment on attachment 369910 [details] Patch How does this not break all of our API tests?
Comment on attachment 369910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369910&action=review > Source/JavaScriptCore/API/JSScript.mm:87 > +#if USE(APPLE_INTERNAL_SDK) I don't like this. We should mock this API for non-apple internal builds. Those functions should still be linked against. > Source/JavaScriptCore/API/JSScript.mm:88 > + if (rootless_check_datavault_flag(FileSystem::fileSystemRepresentation(directory).data(), nullptr)) { What does this do on iOS?
Comment on attachment 369910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369910&action=review >> Source/JavaScriptCore/API/JSScript.mm:87 >> +#if USE(APPLE_INTERNAL_SDK) > > I don't like this. We should mock this API for non-apple internal builds. Those functions should still be linked against. Check out something like wtf/spi/darwin/SandboxSPI.h for how we do stuff like this. FWIW, the code that uses this inside Source/WebKit is also doing the thing we try not to do, so we could fix that as well.
Comment on attachment 369910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369910&action=review >> Source/JavaScriptCore/API/JSScript.mm:88 >> + if (rootless_check_datavault_flag(FileSystem::fileSystemRepresentation(directory).data(), nullptr)) { > > What does this do on iOS? Does the second argument not matter here? Comment inside AuxiliaryProcessMac.mm seems to indicate it must match the data vault plist entry: // These strings must match the last segment of the "com.apple.rootless.storage.<this part must match>" entry in each // process's restricted entitlements file (ex. Configurations/Networking-OSX-restricted.entitlements). constexpr const char* processStorageClass(AuxiliaryProcess::ProcessType type) { switch (type) { case AuxiliaryProcess::ProcessType::WebContent: return "WebKitWebContentSandbox"; case AuxiliaryProcess::ProcessType::Network: return "WebKitNetworkingSandbox"; case AuxiliaryProcess::ProcessType::Plugin: return "WebKitPluginSandbox"; } }
Comment on attachment 369910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369910&action=review Sorry, I didn't mean to r? just yet. I do need to fix the tests, just not quite sure how to go about it yet. >>> Source/JavaScriptCore/API/JSScript.mm:87 >>> +#if USE(APPLE_INTERNAL_SDK) >> >> I don't like this. We should mock this API for non-apple internal builds. Those functions should still be linked against. > > Check out something like wtf/spi/darwin/SandboxSPI.h for how we do stuff like this. FWIW, the code that uses this inside Source/WebKit is also doing the thing we try not to do, so we could fix that as well. Sounds good, I'll do that. >>> Source/JavaScriptCore/API/JSScript.mm:88 >>> + if (rootless_check_datavault_flag(FileSystem::fileSystemRepresentation(directory).data(), nullptr)) { >> >> What does this do on iOS? > > Does the second argument not matter here? Comment inside AuxiliaryProcessMac.mm seems to indicate it must match the data vault plist entry: > // These strings must match the last segment of the "com.apple.rootless.storage.<this part must match>" entry in each > // process's restricted entitlements file (ex. Configurations/Networking-OSX-restricted.entitlements). > constexpr const char* processStorageClass(AuxiliaryProcess::ProcessType type) > { > switch (type) { > case AuxiliaryProcess::ProcessType::WebContent: > return "WebKitWebContentSandbox"; > case AuxiliaryProcess::ProcessType::Network: > return "WebKitNetworkingSandbox"; > case AuxiliaryProcess::ProcessType::Plugin: > return "WebKitPluginSandbox"; > } > } The second argument is optional.
Created attachment 369958 [details] Patch
Created attachment 370158 [details] Patch
Comment on attachment 370158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370158&action=review > Source/JavaScriptCore/ChangeLog:9 > + For security reasons, enforce that API clients only store cached bytecode in data vaults. Please elaborate on what these security reasons are, and how data vaults help solve the problem.
Created attachment 370220 [details] Patch
Comment on attachment 370220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370220&action=review r=me unless this will break open source API tests. > Source/JavaScriptCore/API/JSScript.mm:107 > + if (!validateBytecodeCachePath(cachePath, error)) > + return nil; Should we only do this check if the client is an internal client? Won't this cause API tests for builds that are not using the internal SDK to fail? Maybe I'm missing something though. > Source/JavaScriptCore/API/JSScript.mm:121 > + if (!validateBytecodeCachePath(cachePath, error)) Ditto.
Created attachment 370300 [details] Patch for landing
Comment on attachment 370300 [details] Patch for landing Clearing flags on attachment: 370300 Committed r245564: <https://trac.webkit.org/changeset/245564>
All reviewed patches have been landed. Closing bug.
Reopening to attach new patch.
Created attachment 370333 [details] Patch
Comment on attachment 370333 [details] Patch Seems correct to me but I'd get someone else to double check this.
Committed r245594: <https://trac.webkit.org/changeset/245594>
Landed the fix for production builds in r245594: <https://trac.webkit.org/changeset/245594>
Comment on attachment 370300 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=370300&action=review > Source/JavaScriptCore/API/JSScript.mm:80 > + createError([NSString stringWithFormat:@"Cache path `%s` already exists and is not a file", systemPath.utf8().data()], error); Why not %@? Don't think this is guaranteed to be ascii ditto below > Source/JavaScriptCore/testapi.entitlements:10 > +<?xml version="1.0" encoding="UTF-8"?> > +<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> > +<plist version="1.0"> > +<dict> > + <key>com.apple.security.cs.allow-jit</key> > + <true/> > + <key>com.apple.rootless.storage.JavaScriptCore</key> > + <true/> > +</dict> > +</plist> What happens when someone in non internal SDK tries to build testapi with this entitlement? Does it just work or is it a compile error?
Comment on attachment 370300 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=370300&action=review >> Source/JavaScriptCore/API/JSScript.mm:80 >> + createError([NSString stringWithFormat:@"Cache path `%s` already exists and is not a file", systemPath.utf8().data()], error); > > Why not %@? Don't think this is guaranteed to be ascii > > > ditto below I didn't know I could use %@ with non-ObjC objects. >> Source/JavaScriptCore/testapi.entitlements:10 >> +</plist> > > What happens when someone in non internal SDK tries to build testapi with this entitlement? Does it just work or is it a compile error? The entitlement was supposed to only be applied if HAVE_INTERNAL_SDK, otherwise testapi will crash on launch. I think I might have gotten it wrong though, I'll test it.