Bug 197898 – Only cache bytecode for API clients in data vaults

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tadeu Zagallo]

<rdar://problem/45945449>

[Tadeu Zagallo]

Created attachment 369910 [details]
Patch

[Saam Barati]

Comment on attachment 369910 [details]
Patch

How does this not break all of our API tests?

[Saam Barati]

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?

[Saam Barati]

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.

[Saam Barati]

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";
    }
}

[Tadeu Zagallo]

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.

[Tadeu Zagallo]

Created attachment 369958 [details]
Patch

[Tadeu Zagallo]

Created attachment 370158 [details]
Patch

[Sam Weinig]

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.

[Tadeu Zagallo]

Created attachment 370220 [details]
Patch

[Keith Miller]

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.

[Tadeu Zagallo]

Created attachment 370300 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370300 [details]
Patch for landing

Clearing flags on attachment: 370300

Committed r245564: <https://trac.webkit.org/changeset/245564>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tadeu Zagallo]

Reopening to attach new patch.

[Tadeu Zagallo]

Created attachment 370333 [details]
Patch

[Keith Miller]

Comment on attachment 370333 [details]
Patch

Seems correct to me but I'd get someone else to double check this.

[Tadeu Zagallo]

Committed r245594: <https://trac.webkit.org/changeset/245594>

[Tadeu Zagallo]

Landed the fix for production builds in r245594: <https://trac.webkit.org/changeset/245594>

[Saam Barati]

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?

[Tadeu Zagallo]

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.