Bug 210304 - Do more checking before reusing precompiled sandbox
Summary: Do more checking before reusing precompiled sandbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-09 14:01 PDT by Brent Fulgham
Modified: 2020-04-14 14:50 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.45 KB, patch)
2020-04-09 14:53 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.47 KB, patch)
2020-04-09 14:55 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.48 KB, patch)
2020-04-09 15:05 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.59 KB, patch)
2020-04-13 16:34 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.62 KB, patch)
2020-04-14 12:13 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.62 KB, patch)
2020-04-14 13:09 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (5.46 KB, patch)
2020-04-14 14:08 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2020-04-09 14:01:38 PDT
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.
Comment 1 Brent Fulgham 2020-04-09 14:09:03 PDT
<rdar://problem/61155623>
Comment 2 Brent Fulgham 2020-04-09 14:53:30 PDT
Created attachment 396015 [details]
Patch
Comment 3 Brent Fulgham 2020-04-09 14:55:55 PDT
Created attachment 396016 [details]
Patch
Comment 4 Brent Fulgham 2020-04-09 15:05:58 PDT
Created attachment 396019 [details]
Patch
Comment 5 Geoffrey Garen 2020-04-09 15:49:47 PDT
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?
Comment 6 Saam Barati 2020-04-13 01:35:35 PDT
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.
Comment 7 Saam Barati 2020-04-13 01:35:45 PDT
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.
Comment 8 Saam Barati 2020-04-13 01:38:17 PDT
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
Comment 9 Brent Fulgham 2020-04-13 16:34:48 PDT
Created attachment 396356 [details]
Patch
Comment 10 Darin Adler 2020-04-13 18:07:25 PDT
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.
Comment 11 Saam Barati 2020-04-14 11:47:10 PDT
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.
Comment 12 Alexey Proskuryakov 2020-04-14 11:58:26 PDT
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.
Comment 13 Brent Fulgham 2020-04-14 12:13:31 PDT
Created attachment 396448 [details]
Patch
Comment 14 Geoffrey Garen 2020-04-14 12:35:43 PDT
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);

==
Comment 15 Brent Fulgham 2020-04-14 13:09:15 PDT
(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.
Comment 16 Brent Fulgham 2020-04-14 13:09:53 PDT
Created attachment 396453 [details]
Patch
Comment 17 Darin Adler 2020-04-14 13:37:43 PDT
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.
Comment 18 Brent Fulgham 2020-04-14 14:00:31 PDT
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.
Comment 19 Brent Fulgham 2020-04-14 14:08:14 PDT
Created attachment 396459 [details]
Patch for landing
Comment 20 EWS 2020-04-14 14:30:30 PDT
Committed r260098: <https://trac.webkit.org/changeset/260098>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396459 [details].
Comment 21 Brent Fulgham 2020-04-14 14:50:14 PDT
This is actually <rdar://problem/61537700>.