Bug 210304 – Do more checking before reusing precompiled sandbox

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brent Fulgham]

We recently discovered that the system sandbox framework version does not always change when breaking changes in the sandbox format are made. This can lead to the precompiled version of a sandbox not working properly when a sandbox version changes.

To guard against this, make the following changes:

1. Compare the SANDBOX_BUILD_ID in place when the sandbox was compiled to the value on the current system. SANDBOX_BUILD_ID, which is a GUID, is always regenerated when the sandbox framework is rebuilt.

2. Compare the OS version in place when the sandbox was compiled to the value on the current system. This will trigger us recompiling sandboxes even when the sandbox framework did not change, but this is a small performance cost that would only happen after a software update.

[Brent Fulgham]

<rdar://problem/61155623>

[Brent Fulgham]

Created attachment 396015 [details]
Patch

[Brent Fulgham]

Created attachment 396016 [details]
Patch

[Brent Fulgham]

Created attachment 396019 [details]
Patch

[Geoffrey Garen]

Comment on attachment 396019 [details]
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:424
> +    std::strncpy(cachedHeader.osVersion, osVersion.utf8().data(), versionLength);

Would be slightly nicer to use sizeof(cachedSandboxHeader.osVersion) here, or to use versionLength below. That way, the length we copy in the equal to the length we compare.

Relatedly: What guarantees that systemMarketingVersion() will always return a string that is equal in size to versionLength? Seems like we need a length check before the copy. And maybe we should reserve extra space in case systemMarketingVersion() starts returning something bigger in the future?

[Saam Barati]

Comment on attachment 396019 [details]
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:469
>      if (cachedSandboxHeader.versionNumber != CachedSandboxVersionNumber)

Style wise it really feels like this should be the first branch.

[Saam Barati]

Comment on attachment 396019 [details]
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:469
>      if (cachedSandboxHeader.versionNumber != CachedSandboxVersionNumber)

Style wise it really feels like this should be the first branch.

[Saam Barati]

Comment on attachment 396019 [details]
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:94
> +constexpr unsigned guidLength = 36 + 1;

Nit: I’d name these “size” since length is really this - 1

[Brent Fulgham]

Created attachment 396356 [details]
Patch

[Darin Adler]

Comment on attachment 396356 [details]
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:427
> +    std::strncpy(cachedHeader.sandboxBuildID, SANDBOX_BUILD_ID, sizeof(cachedHeader.sandboxBuildID));
> +    RELEASE_ASSERT(!cachedHeader.sandboxBuildID[guidSize - 1]);
> +    std::strncpy(cachedHeader.osVersion, osVersion.utf8().data(), sizeof(cachedHeader.osVersion));
> +    RELEASE_ASSERT(!cachedHeader.osVersion[versionSize - 1]);

It’s almost never good to use strncpy; I suggest using strlcpy instead.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:476
> +    if (std::strncmp(cachedSandboxHeader.sandboxBuildID, SANDBOX_BUILD_ID, sizeof(cachedSandboxHeader.sandboxBuildID)))
>          return false;
> +    if (std::strncmp(cachedSandboxHeader.osVersion, osVersion.utf8().data(), sizeof(cachedSandboxHeader.osVersion)))
> +        return false;

These should be strcmp; no reason I can think of to use strncmp to compare two strings that are both null-terminated.

[Saam Barati]

Comment on attachment 396356 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        We recently discovered that the system sandbox framework version does not always change
> +        when breaking changes in the sandbox format are made. This can lead to the precompiled

Logically, this feels very weird. Can we tell them not to do that?

I'm not against us adding the code you're adding here, too. It just feels weird that they can make a breaking change without updating the version.

[Alexey Proskuryakov]

There are many system dylibs that are not set up to ever update the version, as there is very little observable effect, if any. Still, see rdar://problem/61155623.

[Brent Fulgham]

Created attachment 396448 [details]
Patch

[Geoffrey Garen]

Comment on attachment 396448 [details]
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:425
> +    ASSERT(copied = guidSize - 1);

==

[Brent Fulgham]

(In reply to Geoffrey Garen from comment #14)
> Comment on attachment 396448 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396448&action=review
> 
> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:425
> > +    ASSERT(copied = guidSize - 1);
> 
> ==

Oh hell. Fixing.

[Brent Fulgham]

Created attachment 396453 [details]
Patch

[Darin Adler]

Comment on attachment 396453 [details]
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:426
> +    RELEASE_ASSERT(!cachedHeader.sandboxBuildID[guidSize - 1]);

OK to have this, but overkill since we are using strlcpy, which guarantees null termination. That’s it’s whole reason for existing.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:428
> +    RELEASE_ASSERT(!cachedHeader.osVersion[versionSize - 1]);

Ditto.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:477
> +    if (std::strcmp(cachedSandboxHeader.osVersion, osVersion.utf8().data()))

Given that this is an ASCII string, this can just be written:

    if (cachedSandboxHeader.osVersion != systemMarketingVersion())

The String class already has a != operator that works.

[Brent Fulgham]

Comment on attachment 396453 [details]
Patch

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

>> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:426
>> +    RELEASE_ASSERT(!cachedHeader.sandboxBuildID[guidSize - 1]);
> 
> OK to have this, but overkill since we are using strlcpy, which guarantees null termination. That’s it’s whole reason for existing.

I'll remove both of these.

>> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:477
>> +    if (std::strcmp(cachedSandboxHeader.osVersion, osVersion.utf8().data()))
> 
> Given that this is an ASCII string, this can just be written:
> 
>     if (cachedSandboxHeader.osVersion != systemMarketingVersion())
> 
> The String class already has a != operator that works.

Okay -- that's much tidier.

[Brent Fulgham]

Created attachment 396459 [details]
Patch for landing

[EWS]

Committed r260098: <https://trac.webkit.org/changeset/260098>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396459 [details].

[Brent Fulgham]

This is actually <rdar://problem/61537700>.