Bug 184991 – We should cache the compiled sandbox profile in a data vault

• Component: WebKit2
• Status: RESOLVED FIXED

[Saam Barati]

So we don't need to compile it every time we launch the web content process on mac

[Saam Barati]

(In reply to Saam Barati from comment #0)
> So we don't need to compile it every time we launch the web content process
> on mac

I think I need to make this a more general solution and make this how it works for all child processes.

[Saam Barati]

Created attachment 338858 [details]
WIP

Super hacky ATM, but the UIProcess compiled the sandbox, sent it to web content processes, and the web content process applied it, and nothing crashed. It seems to work. I just need to clean things up and do some security related stuff and verification that the sandbox is what we expect.

[Brent Fulgham]

Comment on attachment 338858 [details]
WIP

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

This is a great idea!

> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:118
> +        parameters.compiledSandboxSize = xpc_data_get_length(sandbox);

parameters.compiledSandbox should be a std::optional<Vector<unsigned char>> (or maybe uint8_t)

[Geoffrey Garen]

Comment on attachment 338858 [details]
WIP

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

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:230
> +    if (m_launchOptions.processType == ProcessLauncher::ProcessType::Web) {

Would be nice to do this for all child process types. Network process and database process, in particular, would benefit.

[Saam Barati]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 338858 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338858&action=review
> 
> > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:230
> > +    if (m_launchOptions.processType == ProcessLauncher::ProcessType::Web) {
> 
> Would be nice to do this for all child process types. Network process and
> database process, in particular, would benefit.

Other processes don't really get a benefit from this they're launch once (unless we did the work concurrently to spawning them, then IPCd the compiled sandbox to them). Any process that is launch more than once would get a benefit though.

[Saam Barati]

Comment on attachment 338858 [details]
WIP

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

>> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:118
>> +        parameters.compiledSandboxSize = xpc_data_get_length(sandbox);
> 
> parameters.compiledSandbox should be a std::optional<Vector<unsigned char>> (or maybe uint8_t)

This is memory we're getting from xpc, so we can't put it inside of a Vector (AKAIK), since it isn't memory we own.

[Brent Fulgham]

(In reply to Saam Barati from comment #6)
> Comment on attachment 338858 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338858&action=review
> 
> >> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:118
> >> +        parameters.compiledSandboxSize = xpc_data_get_length(sandbox);
> > 
> > parameters.compiledSandbox should be a std::optional<Vector<unsigned char>> (or maybe uint8_t)
> 
> This is memory we're getting from xpc, so we can't put it inside of a Vector
> (AKAIK), since it isn't memory we own.

We launch a second network process for "Private Browsing" use.

[Saam Barati]

Created attachment 338915 [details]
patch for feedback

This seems mostly done. I just want to get some feedback on it. There are a couple things I need to tidy up and figure out.

[Saam Barati]

Comment on attachment 338915 [details]
patch for feedback

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

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:189
> +        SandboxProfile* sandboxProfile = sandbox_compile_file(profilePath.data(), params, &error);

I verified locally that compiling the sandbox in the UIProcess and the web process give me the same compiled binary back.

[EWS Watchlist]

Attachment 338915 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/mac/ChildProcessMac.mm:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:59:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:309:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

(In reply to Brent Fulgham from comment #7)
> (In reply to Saam Barati from comment #6)
> > Comment on attachment 338858 [details]
> > WIP
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=338858&action=review
> > 
> > >> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:118
> > >> +        parameters.compiledSandboxSize = xpc_data_get_length(sandbox);
> > > 
> > > parameters.compiledSandbox should be a std::optional<Vector<unsigned char>> (or maybe uint8_t)
> > 
> > This is memory we're getting from xpc, so we can't put it inside of a Vector
> > (AKAIK), since it isn't memory we own.
> 
> We launch a second network process for "Private Browsing" use.

Interesting. It sounds like we may want some of the architecture me and Ryosuke discussed yesterday, where we spawn off the compilation in another thread, and make process launch wait for that before sending its first IPC message. That said, I doubt this would be a huge win if we spawn at most 2 network processes.

[Brent Fulgham]

(In reply to Saam Barati from comment #11)

> > > 
> > We launch a second network process for "Private Browsing" use.
> 
> Interesting. It sounds like we may want some of the architecture me and
> Ryosuke discussed yesterday, where we spawn off the compilation in another
> thread, and make process launch wait for that before sending its first IPC
> message. That said, I doubt this would be a huge win if we spawn at most 2
> network processes.

I’m wrong. We have separate SESSIONS, but not separate processes.

[Saam Barati]

I'm going to do some perf testing of this. Just waiting on a full root build.

[Saam Barati]

Created attachment 338951 [details]
WIP

See if non apple internal now builds.

[EWS Watchlist]

Attachment 338951 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/mac/ChildProcessMac.mm:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:74:  The parameter name "params" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:75:  The parameter name "params" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:76:  The parameter name "params" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:77:  The parameter name "profile" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:309:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 9 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 338951 [details]
WIP

Attachment 338951 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7475372

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 338955 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Saam Barati]

Created attachment 338963 [details]
WIP

Add some logging to get information for what the bots are crashing on.

[EWS Watchlist]

Attachment 338963 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/mac/ChildProcessMac.mm:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:74:  The parameter name "params" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:75:  The parameter name "params" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:76:  The parameter name "params" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/spi/darwin/SandboxSPI.h:77:  The parameter name "profile" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:309:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 9 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 338963 [details]
WIP

Attachment 338963 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7476498

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 338968 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 338963 [details]
WIP

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:127
> +        RELEASE_ASSERT_WITH_MESSAGE(userDirectorySuffix != parameters.extraInitializationData.end(), "When specifying a compiled sandbox to be applied, we must also specify a suffix.");

Looks like EWS hits this assertion?

[Geoffrey Garen]

Comment on attachment 338963 [details]
WIP

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

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:121
> +    static size_t constexpr macPageSize = 4096;
> +    static_assert(sizeof(SandboxProfile) < macPageSize, "");
> +    alignas(macPageSize) static char sandboxProfileBuffer[macPageSize];

I think this would be cleaner if you did a dynamic allocation using the system page size API.

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:207
> +            char* newData = static_cast<char*>(fastAlignedMalloc(macPageSize, alignedSize));
> +            memcpy(newData, sandboxProfile->data, sandboxProfile->size);
> +            memset(newData + sandboxProfile->size, 0, alignedSize - sandboxProfile->size);
> +            mprotect(newData, alignedSize, PROT_READ);

You should just mmap here. It's bad code smell to mprotect malloc memory, and if you mmap you don't need to do the rounding or the zeroing.

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:208
> +            sandboxProfile->data = bitwise_cast<unsigned char*>(newData);

Does this statement leak the old memory allocated to sandboxProfile->data?

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:318
> +    if (m_launchOptions.processType == ProcessLauncher::ProcessType::Web)
> +        addSandboxAndUserDirectorySuffix(bootstrapMessage.get(), extraInitializationData.get(), clientIdentifier);

Do we need a !PLATFORM(IOS) here?

[Alexey Proskuryakov]

Comment on attachment 338963 [details]
WIP

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

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:123
> +    String userDirectorySuffix = makeString("com.apple.WebKit.WebContent+", clientIdentifier);

Have we already discussed whether we need to stop having WebContent processes share cache and temporary directories?

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:134
> +            exit(EX_NOPERM);

Not sure if silently terminating the process is still the right thing to do now that it's the UI process. If anything, this should be RELEASE_ASSERT I think.

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:147
> +            setenv("DIRHELPER_USER_DIR_SUFFIX", WebCore::FileSystem::fileSystemRepresentation(userDirectorySuffix).data(), 1);

That's quite a race with other threads. I actually don't think that it's OK...

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:150
> +            sandboxParameters.addConfDirectoryParameter("DARWIN_USER_TEMP_DIR", _CS_DARWIN_USER_TEMP_DIR);
> +            sandboxParameters.addConfDirectoryParameter("DARWIN_USER_CACHE_DIR", _CS_DARWIN_USER_CACHE_DIR);

This is all inside call_once, and it depends on clientIdentifier function argument. I know that clientIdentifier will never change, so there will be no problem in practice, but code structure looks wrong because of this. We respect the function argument the first time, and then half-respect it on subsequent calls.

[Alexey Proskuryakov]

I do not think that this can be done in UI process. As the freshly launched WebContent process is super powerful (it's not sandboxed at all), it can't trust any input from UI process.

[Geoffrey Garen]

(In reply to Alexey Proskuryakov from comment #25)
> I do not think that this can be done in UI process. As the freshly launched
> WebContent process is super powerful (it's not sandboxed at all), it can't
> trust any input from UI process.

I thought about this a bit and here's my take:

On iOS this isn't a concern because the sandbox is compiled into the kernel.

On macOS, this isn't a concern because nothing requires the UI process to be sandboxed, so using the WebProcess to un-sandbox yourself is not a meaningful attack.

If you imagine a future imaginaryOS that requires all apps to be sandboxed, imaginaryOS could resolve this attack by:

(a) compiling sandboxes into the kernel, as iOS does

OR

(b) having the WebContent process compile the sandbox and cache it to disk using a "data vault".

I don't think either of these solutions is required today, though. And it's likely that any OS that required all apps to be sandboxed and would already do (a) for us.

[Saam Barati]

Comment on attachment 338963 [details]
WIP

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

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:121
>> +    alignas(macPageSize) static char sandboxProfileBuffer[macPageSize];
> 
> I think this would be cleaner if you did a dynamic allocation using the system page size API.

We can do that.

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:207
>> +            mprotect(newData, alignedSize, PROT_READ);
> 
> You should just mmap here. It's bad code smell to mprotect malloc memory, and if you mmap you don't need to do the rounding or the zeroing.

We do this in a few places in JSC :|

Maybe I should just use OSAllocator...

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:208
>> +            sandboxProfile->data = bitwise_cast<unsigned char*>(newData);
> 
> Does this statement leak the old memory allocated to sandboxProfile->data?

yeah, I need to call sandbox_free_profile

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:318
>> +        addSandboxAndUserDirectorySuffix(bootstrapMessage.get(), extraInitializationData.get(), clientIdentifier);
> 
> Do we need a !PLATFORM(IOS) here?

I think so. It's curious that this file is named ProcessLauncherMac but there are ifdefs for PLATFORM(IOS). Not sure what's going on there.

[Alexey Proskuryakov]

> On macOS, this isn't a concern because nothing requires the UI process to be sandboxed

macOS AppStore does.

[Saam Barati]

Comment on attachment 338963 [details]
WIP

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

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:123
>> +    String userDirectorySuffix = makeString("com.apple.WebKit.WebContent+", clientIdentifier);
> 
> Have we already discussed whether we need to stop having WebContent processes share cache and temporary directories?

I have not been part of any such discussions.

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:134
>> +            exit(EX_NOPERM);
> 
> Not sure if silently terminating the process is still the right thing to do now that it's the UI process. If anything, this should be RELEASE_ASSERT I think.

Yeah I think we should just CRASH() here.

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:147
>> +            setenv("DIRHELPER_USER_DIR_SUFFIX", WebCore::FileSystem::fileSystemRepresentation(userDirectorySuffix).data(), 1);
> 
> That's quite a race with other threads. I actually don't think that it's OK...

What other threads would be setting/reading this? We could do this concatenation ourselves I suppose

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:150
>> +            sandboxParameters.addConfDirectoryParameter("DARWIN_USER_CACHE_DIR", _CS_DARWIN_USER_CACHE_DIR);
> 
> This is all inside call_once, and it depends on clientIdentifier function argument. I know that clientIdentifier will never change, so there will be no problem in practice, but code structure looks wrong because of this. We respect the function argument the first time, and then half-respect it on subsequent calls.

What do you think would be better to do? Assert? Have a hashmap?

[Saam Barati]

Comment on attachment 338963 [details]
WIP

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

>>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:150
>>> +            sandboxParameters.addConfDirectoryParameter("DARWIN_USER_CACHE_DIR", _CS_DARWIN_USER_CACHE_DIR);
>> 
>> This is all inside call_once, and it depends on clientIdentifier function argument. I know that clientIdentifier will never change, so there will be no problem in practice, but code structure looks wrong because of this. We respect the function argument the first time, and then half-respect it on subsequent calls.
> 
> What do you think would be better to do? Assert? Have a hashmap?

We could just make this function generate the client identifier itself. But then I'd just write a function that abstracts this, call it from two places, and the function is idempotent. I'm not sure that's much better, but perhaps it's cleaner.

[Saam Barati]

Created attachment 339519 [details]
WIP

I still need to figure out some behavior around getting the entitlements into the binary in a testable way. I'm currently signing it manually to test the data vault.

Comment on attachment 339519 [details]
WIP

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

> Source/WebKit/Configurations/WebContent.Development.entitlements:9
> +	<key>com.apple.rootless.storage.SafariFamily</key>

Strange!

> Source/WebKit/Configurations/WebKit.xcconfig:115
> +FRAMEWORK_AND_LIBRARY_LDFLAGS = -lobjc -framework CFNetwork -framework CoreAudio -framework CoreFoundation -framework CoreGraphics -framework CoreText -framework Foundation -framework ImageIO -framework IOKit -framework WebKitLegacy -lnetwork $(WK_ACCESSIBILITY_LDFLAGS) $(WK_APPKIT_LDFLAGS) $(WK_ASSERTION_SERVICES_LDFLAGS) $(WK_CARBON_LDFLAGS) $(WK_CORE_PDF_LDFLAGS) $(WK_CORE_PREDICTION_LDFLAGS) $(WK_CORE_SERVICES_LDFLAGS) $(WK_GRAPHICS_SERVICES_LDFLAGS) $(WK_IOSURFACE_LDFLAGS) $(WK_LIBWEBRTC_LDFLAGS) $(WK_MOBILE_CORE_SERVICES_LDFLAGS) $(WK_MOBILE_GESTALT_LDFLAGS) $(WK_OPENGL_LDFLAGS) $(WK_PDFKIT_LDFLAGS) $(WK_SAFE_BROWSING_LDFLAGS) $(WK_UIKIT_LDFLAGS) $(WK_LIBSANDBOX_LDFLAGS);

Believe it or not, this list is supposed to be sorted.

(In reply to Saam Barati from comment #31)
> Created attachment 339519 [details]
> WIP
> 
> I still need to figure out some behavior around getting the entitlements
> into the binary in a testable way. I'm currently signing it manually to test
> the data vault.

See also the discussion in bug 184485 comment 5.

[Saam Barati]

Comment on attachment 339519 [details]
WIP

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

>> Source/WebKit/Configurations/WebContent.Development.entitlements:9
>> +	<key>com.apple.rootless.storage.SafariFamily</key>
> 
> Strange!

Indeed. This will need to be removed ;-)
I added this when I was banging my head against the wall of figuring a way to test the entitlement being present.

[Ben Richards]

Created attachment 342768 [details]
add sandbox file caching

[Ben Richards]

wow sorry something went wrong with that patch let me try again

[Ben Richards]

Created attachment 342781 [details]
WIP

implemented separate data vaults for each process type

[Chris Dumez]

Comment on attachment 342781 [details]
WIP

Please do not set r? flag for WIP patches.

[Saam Barati]

Comment on attachment 342781 [details]
WIP

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

Will keep looking, just a few nits

> Source/WebKit/Shared/ChildProcess.h:48
> +    enum ProcessType {

let's use enum class

> Source/WebKit/Shared/mac/ChildProcessMac.mm:134
> +    tracePoint(static_cast<TracePointCode>(InitializeSandboxStart));
> +    auto stopTraceOnExit = makeScopeExit([] {
> +        tracePoint(static_cast<TracePointCode>(InitializeSandboxEnd));
> +    });

This can just be:
TraceScope traceScope(InitializeSandboxStart, InitializeSandboxEnd)

> Source/WebKit/Shared/mac/ChildProcessMac.mm:462
> +            case WebContentType:
> +                madeDirectory = !(rootless_mkdir_datavault(sandboxPath.data(), 0700, "WebKitWebContentSandbox"));
> +                break;
> +            case NetworkType:
> +                madeDirectory = !(rootless_mkdir_datavault(sandboxPath.data(), 0700, "WebKitNetworkingSandbox"));
> +                break;
> +            case StorageType:
> +                madeDirectory = !(rootless_mkdir_datavault(sandboxPath.data(), 0700, "WebKitStorageSandbox"));
> +                break;
> +            case PluginType:
> +                madeDirectory = !(rootless_mkdir_datavault(sandboxPath.data(), 0700, "WebKitPluginSandbox"));
> +                break;

style nit: This would be cleaner if you just has a `const char*` variable you assigned to in the switch, then, after the switch, you can call:
madeDirectory = !(rootless_mkdir_datavault(sandboxPath.data(), 0700, name));

> Source/WebKit/Shared/mac/ChildProcessMac.mm:464
> +                CRASH();

We usually use RELEASE_ASSERT_NOT_REACHED(). However, if you move to an enum class here, you can remove this default case. If someone adds a new value to that enum class, it'd become a compile error if they don't handle it here.

[Daniel Bates]

Comment on attachment 342781 [details]
WIP

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:275
> +        // Compute the sandbox header size.

This function is getting very long. Can we extract more code into functions?

[Igalia-pontevedra EWS]

Comment on attachment 342781 [details]
WIP

Attachment 342781 [details] did not pass gtk-wk2-ews (gtk-wk2):
Output: http://webkit-queues.webkit.org/results/8193520

New failing tests:
media/video-ended-seek-crash.html
media/video-restricted-invisible-autoplay-allowed-when-visible.html

[Igalia-pontevedra EWS]

Created attachment 342796 [details]
Archive of layout-test-results from ltilve-gtk-wk2-ews for gtk-wk2

The attached test failures were seen while running run-webkit-tests on the gtk-wk2-ews.
Bot: ltilve-gtk-wk2-ews  Port: gtk-wk2  Platform: Linux-4.16.0-0.bpo.1-amd64-x86_64-with-debian-9.4

[Ben Richards]

Created attachment 342947 [details]
cache sandbox

[Saam Barati]

Comment on attachment 342947 [details]
cache sandbox

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

My high level comments are:
- let's figure out what our strategy is for OS updates / changes to the sandbox file. I think the latter is handled by memcmp the file itself. But we should lay out our strategy perhaps in a comment and convince ourselves it's correct.
- that security bug I spotted w.r.t the directory being made before we try to make it. Let's figure out a way to validate a directory is a datavault/change it to being a datavault.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:76
> +    // OOPS: build in versioning based on webkit binary.

What's our plan here for OS/webkit updates?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:208
> +    

style: remove whitespace

> Source/WebKit/Shared/mac/ChildProcessMac.mm:229
> +    auto sandboxHeader = Vector<uint8_t>(headerSize.unsafeGet());

Style nit: I think we usually write this as:

Vector<uint8_t> sandboxHeader(headerSize.unsafeGet());

> Source/WebKit/Shared/mac/ChildProcessMac.mm:256
> +static inline String &&getSandboxDirectory(ChildProcess::ProcessType processType)

"String &&g" => "String&& g"

> Source/WebKit/Shared/mac/ChildProcessMac.mm:286
> +    sandboxDirectory.append("/com.apple.WebKit.WebKitSandbox");

Why don't we also do the above when !APPLE_INTERNAL?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:292
> +static inline String &&getSandboxFilename(const SandboxInfo &info)

"String &&" => "String&& "
"SandboxInfo &" => "SandboxInfo& "

> Source/WebKit/Shared/mac/ChildProcessMac.mm:322
> +static SandboxProfile *compileAndCacheSandboxProfile(const SandboxInfo &info)

"SandboxProfile *" => "SandboxProfile* "
"SandboxInfo &" => "SandboxInfo& "

It's worth reading our style guidelines here:
https://webkit.org/code-style-guidelines/

> Source/WebKit/Shared/mac/ChildProcessMac.mm:325
> +    bool hasSandboxDirectory = FileSystem::fileIsDirectory(info.directoryPath, FileSystem::ShouldFollowSymbolicLinks::Yes);
> +    if (!hasSandboxDirectory) {

I just realized a security bug here:
We don't verify that the directory is a datavault. So, if somebody makes this directory before launching this particular WebKit process for the first time, they can write mkdir their own sandbox directory without it being a data vault, and we'd happily use that directory.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:510
> +        verboseLog("could not apply cached sandbox");

Should be a WTFLogAlways I think. I guess there remains a question of if we should crash here or simply return false, and recompute the profile file. It seems like we could end up here if this file on disk were corrupted somehow. I think it's probably better to just recompile than to CRASH forever.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:637
> +        dataLogLn("Apply time: ", (endTime - startTime).milliseconds());

let's get rid of this or make it a verboseLog

[Ben Richards]

Created attachment 342973 [details]
cache sandbox

Fixed some bugs and style issues, added OS versioning to CachedSandboxHeader, and double check that cache folder is a data vault (and handle issue if it isn't).

[Geoffrey Garen]

I think you need to conditionalize so that folks can build even if they don't have the Apple internal SDK.

You can use the preprocessor __has_include approach or the USE(APPLE_INTERNAL_SDK) approach. Both appear elsewhere in the project.

    /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -x objective-c++ -arch x86_64 -fmessage-length=0 -fdiagnostics-show-note-include-stack -fmacro-backtrace-limit=0 -std=gnu++1y -stdlib=libc++ -fobjc-weak -gmodules -Wno-trigraphs -fno-exceptions -fno-rtti -fpascal-strings -Os -fno-common -Werror -Wno-missing-field-initializers -Wmissing-prototypes -Wunreachable-code -Wno-implicit-atomic-properties -Wno-arc-repeated-use-of-weak -Wnon-virtual-dtor -Wno-overloaded-virtual -Wno-exit-time-destructors -Wduplicate-method-match -Wno-missing-braces -Wparentheses -Wswitch -Wunused-function -Wno-unused-label -Wno-unused-parameter -Wunused-variable -Wunused-value -Wempty-body -Wuninitialized -Wno-unknown-pragmas -Wno-shadow -Wno-four-char-constants -Wno-conversion -Wconstant-conversion -Wint-conversion -Wbool-conversion -Wenum-conversion -Wsign-compare -Wno-shorten-64-to-32 -Wnewline-eof -Wno-selector -Wno-strict-selector-match -Wundeclared-selector -Wno-deprecated-implementations -Wno-c++11-extensions -DNDEBUG -DENABLE_3D_TRANSFORMS -DENABLE_APPLE_PAY -DENABLE_APPLICATION_MANIFEST -DENABLE_ATTACHMENT_ELEMENT -DENABLE_AVF_CAPTIONS -DENABLE_CACHE_PARTITIONING -DENABLE_CHANNEL_MESSAGING -DENABLE_CONTENT_FILTERING -DENABLE_CSS_ANIMATIONS_LEVEL_2 -DENABLE_CSS_BOX_DECORATION_BREAK -DENABLE_CSS_COMPOSITING -DENABLE_CSS_SCROLL_SNAP -DENABLE_CSS_SELECTORS_LEVEL4 -DENABLE_CSS_TRAILING_WORD -DENABLE_CURSOR_VISIBILITY -DENABLE_DASHBOARD_SUPPORT -DENABLE_DATACUE_VALUE -DENABLE_EXPERIMENTAL_FEATURES -DENABLE_FILTERS_LEVEL_2 -DENABLE_FTL_JIT -DENABLE_FULLSCREEN_API -DENABLE_GAMEPAD -DENABLE_GEOLOCATION -DENABLE_ICONDATABASE -DENABLE_INDEXED_DATABASE -DENABLE_INDEXED_DATABASE_IN_WORKERS -DENABLE_INTERSECTION_OBSERVER -DENABLE_INTL -DENABLE_INTL_NUMBER_FORMAT_TO_PARTS -DENABLE_INTL_PLURAL_RULES -DENABLE_JS_ASYNC_ITERATION -DENABLE_KEYBOARD_CODE_ATTRIBUTE -DENABLE_KEYBOARD_KEY_ATTRIBUTE -DENABLE_LEGACY_CSS_VENDOR_PREFIXES -DENABLE_LEGACY_CUSTOM_PROTOCOL_MANAGER -DENABLE_LEGACY_ENCRYPTED_MEDIA -DENABLE_MATHML -DENABLE_MEDIA_CONTROLS_SCRIPT -DENABLE_MEDIA_SOURCE -DENABLE_MEDIA_STREAM -DENABLE_METER_ELEMENT -DENABLE_MOUSE_CURSOR_SCALE -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SUBTLE_CRYPTO -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_USERSELECT_ALL -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_RTC -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DENABLE_MANUAL_SANDBOXING -DHAVE_CORE_PREDICTION -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -DFRAMEWORK_NAME=WebKit -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -fasm-blocks -fstrict-aliasing -Wprotocol -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.12 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/PrivateHeaders/ForwardingHeaders -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2 -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/WebKitAdditions -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/WebKitAdditions -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/webrtc -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat-security -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wno-unused-parameter -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Carbon.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Quartz.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/PrivateFrameworks -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/WebKit2Prefix-clprtzqrovjqlrcaddkbduispkbz/WebKit2Prefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/ChildProcessMac.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/ChildProcessMac.dia -c /Volumes/Data/EWS/WebKit/Source/WebKit/Shared/Mac/ChildProcessMac.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/ChildProcessMac.o
/Volumes/Data/EWS/WebKit/Source/WebKit/Shared/mac/ChildProcessMac.mm:43:9: fatal error: 'rootless.h' file not found
#import <rootless.h>
        ^
1 error generated.

[Ben Richards]

Created attachment 342997 [details]
cache sandbox

wrapped #import <rootless.h> in #if USE(APPLE_INTERNAL_SDK)

[Ben Richards]

Created attachment 343006 [details]
cache sandbox

[Ben Richards]

Created attachment 343012 [details]
cache sandbox

[Ben Richards]

Created attachment 343014 [details]
cache sandbox

[EWS Watchlist]

Comment on attachment 343014 [details]
cache sandbox

Attachment 343014 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8243414

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 343025 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

Comment on attachment 343014 [details]
cache sandbox

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

> Source/WebKit/Configurations/WebContent-OSX.entitlements:7
> +	<key>com.apple.rootless.storage.WebKitWebContentSandbox</key>

Because this is a restricted entitlement, giving it to an executable makes it crash on launch, as you can see in the test results. Restricted entitlements may only be given when the WK_USE_RESTRICTED_ENTITLEMENTS build setting is set to YES. For the Web Content service, you can specify restricted entitlements in WebContent-OSX-restricted.entitlements, which is only used when that setting is YES. For the other processes, if they currently don’t require any non-restricted entitlements, you may be able to just make setting CODE_SIGN_ENTITLEMENTS conditional on WK_USE_RESTRICTED_ENTITLEMENTS.

[EWS Watchlist]

Comment on attachment 343014 [details]
cache sandbox

Attachment 343014 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8246557

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 343051 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ben Richards]

Created attachment 343063 [details]
cache sandbox

Moved restricted entitlements to <process name>-restricted.entitlements. Set up conditional entitlements for network and storage processes based on WK_USE_RESTRICTED_ENTITLEMENTS

[Saam Barati]

Comment on attachment 343063 [details]
cache sandbox

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:320
> +        bool madeDirectory = !(rootless_mkdir_datavault(sandboxPath.data(), 0700, storageClass));

Don't we want to make this conditional on APPLE_INTERNAL?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:343
> +    if (rootless_check_datavault_flag(directoryPath.data(), storageClass)) {

ditto

[EWS Watchlist]

Comment on attachment 343063 [details]
cache sandbox

Attachment 343063 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/8249983

New failing tests:
imported/w3c/web-platform-tests/FileAPI/blob/Blob-constructor.html

[EWS Watchlist]

Created attachment 343073 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Ben Richards]

Created attachment 343094 [details]
cache sandbox

Exposed some libsandbox functions so they can be used in the non internal builds

[Ben Richards]

Created attachment 343110 [details]
cache sandbox

[Geoffrey Garen]

Comment on attachment 343110 [details]
cache sandbox

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

> Source/WebKit/Configurations/NetworkService.xcconfig:35
> +NETWORK_ENTITLEMENTS_RESTRICTED_NO = Configurations/Network-OSX.entitlements;

FWIW, I think comment #53 was trying to explain that this value can be literally empty. I'm OK with specifying a dummy file too. That might be a nice thing to leave behind for future programmers who need to add unrestricted entitlements.

> Source/WebKit/Configurations/WebContentService.xcconfig:-35
> -CODE_SIGN_ENTITLEMENTS_COCOA_TOUCH_NO = Configurations/WebContent-OSX.entitlements;

I'm skeptical about this change. My reading of process-webcontent-entitlements.sh, which is admittedly limited, is that it intends to merge WebContent-OSX-restricted.entitlements with WebContent-OSX.entitlements. But if you remove this specification here, will the merge still happen? Put another way, if you build with this patch, does the WebContent service still have the com.apple.security.cs.allow-jit entitlement?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:80
> +// CachedFileHeader <- sizeof(CachedFileHeader) bytes

Should be CachedSandboxHeader

> Source/WebKit/Shared/mac/ChildProcessMac.mm:265
> +    String sandboxDirectory = darwinUserCacheDir; // OOPS: Do we want to just use the one w/ client identifier appended to it?

I think we can remove this OOPS.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:283
>  #else
> -    NSBundle *webKit2Bundle = [NSBundle bundleForClass:NSClassFromString(@"WKView")];
> +    sandboxDirectory.append("/com.apple.WebKit.WebKitSandbox");
> +#endif // USE(APPLE_INTERNAL_SDK)

Let's apply the same directory structure for apple internal builds and not. It's not to avoid differences if we can. (The one difference we can't avoid is whether this directory structure is protected by data vault technology. That difference is OK.)

> Source/WebKit/Shared/mac/ChildProcessMac.mm:295
> +#if !(USE(APPLE_INTERNAL_SDK))
> +    sandboxFile.append(info.initializationParameters.userDirectorySuffix());
> +    sandboxFile.append('+');
>  #endif

Same comment about directory structure and file naming.

[Ben Richards]

Created attachment 343126 [details]
cache sandbox

[Daniel Bates]

Comment on attachment 343126 [details]
cache sandbox

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

> Source/WTF/wtf/spi/darwin/SandboxSPI.h:53
> +    size_t size, available;

Please put each variable on its own line.

> Source/WebKit/ChangeLog:28
> +        (WebKit::getFileContents):
> +        (WebKit::getProcessStorageClass):
> +        (WebKit::getSandboxHeader):
> +        (WebKit::getSandboxDirectory):
> +        (WebKit::getSandboxFilename):

These functions do not take out parameters. So, they should not be prefixed with "get".

> Source/WebKit/Shared/mac/ChildProcessMac.mm:188
> +static inline constexpr const char* getProcessStorageClass(ChildProcess::ProcessType type)

static inline constexpr => constexpr

[Ben Richards]

Created attachment 343354 [details]
cache sandbox

tried to clean things up a bit paying closer attention to the webkit style guide

[EWS Watchlist]

Attachment 343354 [details] did not pass style-queue:


ERROR: Source/WTF/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebKit/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Richards]

Created attachment 343356 [details]
cache sandbox

fixed changelog

[Saam Barati]

Comment on attachment 343356 [details]
cache sandbox

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:342
> +        // Directory isn't a datavault, let's convert it to one
> +        if (rootless_convert_to_datavault(directoryPath.data(), storageClass)) {

What are the semantics here? Does this delete all files inside of this directory? If not, I think we have the same vulnerability as before. I sort of think this code should delete this directory and loop back around. The attack I'm thinking of here is you plant a cached sandbox file before launching safari. Safari will convert it to a data vault, but at that point, you may have already planted a bogus cached sandbox.

[Ben Richards]

Comment on attachment 343356 [details]
cache sandbox

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

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:342
>> +        if (rootless_convert_to_datavault(directoryPath.data(), storageClass)) {
> 
> What are the semantics here? Does this delete all files inside of this directory? If not, I think we have the same vulnerability as before. I sort of think this code should delete this directory and loop back around. The attack I'm thinking of here is you plant a cached sandbox file before launching safari. Safari will convert it to a data vault, but at that point, you may have already planted a bogus cached sandbox.

I agree. I was going to say that since this function is called from compileAndCacheSandboxProfile, a bogus file would get overwritten by rename anyways. But if rename fails then that won't be true. I also need to add a check in applyCachedSandbox to verify that the directory is a data vault before applying any cached sandbox.

[EWS Watchlist]

Comment on attachment 343356 [details]
cache sandbox

Attachment 343356 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8307542

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 343456 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 343356 [details]
cache sandbox

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

> Source/WebKit/Configurations/NetworkService.xcconfig:36
> +NETWORK_ENTITLEMENTS_RESTRICTED_NO = Configurations/Network-OSX.entitlements;
> +NETWORK_ENTITLEMENTS_RESTRICTED_YES = Configurations/Network-OSX-restricted.entitlements;

We have gotten into the habit of prefixing WebKit-defined build settings (with the exception of our ENABLE_ ones) with WK_, for clarity and collision-avoidance.

[Ben Richards]

Created attachment 343554 [details]
cache sandbox

I think this is pretty close to being able to be committed

[Saam Barati]

Comment on attachment 343554 [details]
cache sandbox

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:348
> +            CRASH();

This is an easy way to mount a DOS attack against Safari. Let's just avoid the crash here and remove the directory. So this can just all go into a loop IMO. Loop until it's a datavault

> Source/WebKit/Shared/mac/ChildProcessMac.mm:356
> +            CRASH();

ditto. No need to crash here. I think we can just remove the old directory and move on with our life.

[EWS Watchlist]

Comment on attachment 343554 [details]
cache sandbox

Attachment 343554 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8341007

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 343574 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ben Richards]

Created attachment 343651 [details]
Patch

[Ben Richards]

Created attachment 343655 [details]
Patch

[Saam Barati]

Comment on attachment 343655 [details]
Patch

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

This looks really close to done, and looks really solid, just a few comments and maybe a couple of bugs.

> Source/WTF/ChangeLog:3
> +        Added trace points for sandbox initialization

This line should be the title of your bugzilla and you should put this description below the bug "Reviewed by ..." line. (See other changelog entries)

> Source/WebKit/ChangeLog:3
> +        Added sandbox caching so that we don't have to spend 30+ ms compiling the sandbox on every process launch

ditto

> Source/WebKit/Scripts/process-plugin-entitlements.sh:2
> +#!/bin/sh
> +set -e

What is this file doing? I think part is better reviewed by somebody who understands our build system.

> Source/WebKit/Scripts/process-plugin-entitlements.sh:11
> +        if (( ${TARGET_MAC_OS_X_VERSION_MAJOR} >= 101400 )); then

What is this doing? What do we do for older OSs?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:59
> +#define VERBOSE_LOGGING false

why not `static constexpr bool = false`? This can also just be a hardcoded constant in the `verboseLog` function

> Source/WebKit/Shared/mac/ChildProcessMac.mm:75
> +    char osVersion[10];

os version string is guaranteed to be strlen()<=9?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:84
> +// [SandboxBuiltin] optional. Present if CachedSandboxHeader::sanboxBuiltinSize is not UINT_MAX. If present, sandboxBuiltinSize bytes.

Probably worth specifying here if this contains the null terminating byte at the end or not.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:162
> +template<typename... Types>
> +void verboseLog(const Types&... values)
> +{
> +    dataLogLnIf(VERBOSE_LOGGING, values...);
> +}

Do you think we still need this? Was it helpful in you writing this patch?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:219
> +        auto& name = headerPieces.last();

This is wrong. You're grabbing a reference to a Cstring that may move in the line 220 append call. Vector::append may reallocate storage. Maybe just do the math to headerSize here before appending?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:380
> +        WTFLogAlways("%s: \"%s\" is using a reserved path. Attempting to move it.", getprogname(), directoryPath.data(), directoryPath.data());
> +        String movedDirectory = info.directoryPath + ".Old";
> +        while (FileSystem::fileIsDirectory(movedDirectory, FileSystem::ShouldFollowSymbolicLinks::No)) {
> +            // Keep trying until we create the directory
> +            movedDirectory = info.directoryPath + ".Old+" + randomString();
> +        }
> +        FileSystem::makeAllDirectories(movedDirectory);
> +        
> +        // Now let's move everything to the new directory
> +        Vector<String> directoryContents = FileSystem::listDirectory(info.directoryPath, "*");
> +        for (const auto& file : directoryContents) {
> +            String fileName = FileSystem::pathGetFileName(file);
> +            String newPath = FileSystem::pathByAppendingComponent(movedDirectory, fileName);
> +            if (!FileSystem::moveFile(file, newPath)) {
> +                WTFLogAlways("%s: \"%s\" could not be moved and must be deleted", getprogname(), FileSystem::fileSystemRepresentation(file).data());
> +                FileSystem::deleteFile(file);
> +            }
> +        }

Why isn't this just doing rename on the directory? Seems like you have a TOCTOU bug on line 369.

I feel like this entire function should just be something like this:

while (true) {
    if (directory is already is a data vault is true)
        break;
    if (try to make a data vault directory is true)
        break;
     rename directory with random suffix
}

> Source/WebKit/Shared/mac/ChildProcessMac.mm:409
> +    // Make sure the data vault/directory we want to cache in is good to go

style nit: this comment doesn't add anything that isn't described by your function name below

> Source/WebKit/Shared/mac/ChildProcessMac.mm:456
> +        if (i)
> +            tempFileString.append(String::number(i));

I wonder if we should use your randomString function here instead, so someone can't make us iterate for a long time.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:475
> +            // According to POSIX complience requirements, this should not only rename the temporary

complience => compliance

Also worth linking to where you found this info

> Source/WebKit/Shared/mac/ChildProcessMac.mm:486
> +    // Clean up

style nit: this comment doesn't add anything

> Source/WebKit/Shared/mac/ChildProcessMac.mm:588
> +    const static NSBundle* webKit2Bundle = [NSBundle bundleForClass:NSClassFromString(@"WKWebView")];

I think our style for ObjC code is to have the pointer be "Type *ident" instead of our normal style of "Type* ident"

> Source/WebKit/Shared/mac/ChildProcessMac.mm:676
> +    char temporaryDirectory[PATH_MAX + 1];

why +1 here?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:687
> +#if WK_API_ENABLED
> +    NSBundle* webKit2Bundle = [NSBundle bundleForClass:NSClassFromString(@"WKWebView")];
> +#else
> +    NSBundle* webKit2Bundle = [NSBundle bundleForClass:NSClassFromString(@"WKView")];
> +#endif

We do this type of thing twice, maybe we should make a function for it?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:738
> +    NSEvent* event = [NSEvent otherEventWithType:NSEventTypeApplicationDefined location:NSMakePoint(0, 0) modifierFlags:0 timestamp:0.0 windowNumber:0 context:nil subtype:0 data1:0 data2:0];

I think the previous style is correct here

[Ben Richards]

Comment on attachment 343655 [details]
Patch

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

>> Source/WebKit/Scripts/process-plugin-entitlements.sh:2
>> +set -e
> 
> What is this file doing? I think part is better reviewed by somebody who understands our build system.

I copied most of this code from Source/WebKit/Scripts/process-webcontent-entitlements.sh. It merges in the restricted entitlements in Plugin-OSX-restricted.entitlements dynamically based on whether WK_USE_RESTRICTED_ENTITLEMENTS is YES. The setup for this and WebContent are almost identical

>> Source/WebKit/Scripts/process-plugin-entitlements.sh:11
>> +        if (( ${TARGET_MAC_OS_X_VERSION_MAJOR} >= 101400 )); then
> 
> What is this doing? What do we do for older OSs?

This is just to be consistent with Source/WebKit/Scripts/process-webcontent-entitlements.sh. The alternative is we need to change the restriction in process-webcontent-entitlements.sh (which I assume was put there for a reason) or make two new files, one for WebContent and one for Plugin, that only hold their sandbox entitlement and merge those dynamically without the restriction.

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:75
>> +    char osVersion[10];
> 
> os version string is guaranteed to be strlen()<=9?

I think the strlen is always 7 from what I've seen but left it as 10 incase the version number grows in the future. Each time we load this or compare we use a length checked function to prevent overflow so this should be safe.

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:380
>> +        }
> 
> Why isn't this just doing rename on the directory? Seems like you have a TOCTOU bug on line 369.
> 
> I feel like this entire function should just be something like this:
> 
> while (true) {
>     if (directory is already is a data vault is true)
>         break;
>     if (try to make a data vault directory is true)
>         break;
>      rename directory with random suffix
> }

I don't think we should care too much if a directory with the same name as movedDirectory is created between 365 and 369. This is really just a half assed attempt to avoid deleting someone's files if they really didn't mean for them to end up there. The important thing here is that even if a directory is pre placed with the same name as the datavault we want to create, we can first convert it to a data vault to prevent further tampering and then move what we can out and delete the reset. This should avoid any potential bugs where we get stuck in that rename loop somehow

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:676
>> +    char temporaryDirectory[PATH_MAX + 1];
> 
> why +1 here?

I've seen people add +1 just to account for null termination but either works

[Ben Richards]

Created attachment 343771 [details]
Patch

[Saam Barati]

Comment on attachment 343771 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:431
> +        fileContents.append(sandboxProfile->builtin, cachedHeader.builtinSize);

This function expects you to pass in a length that *does not* include the null terminating byte. However, above, you do +1.

I'd vote for not storing the terminating byte on disk since we're already storing the length. Storing the byte is superfluous.

[Saam Barati]

Comment on attachment 343771 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:59
> +#define VERBOSE_LOGGING false

remove this

[Ben Richards]

Created attachment 343774 [details]
Patch

[Ben Richards]

Created attachment 343776 [details]
Patch

[Ben Richards]

applied Saam's suggestions

[EWS Watchlist]

Comment on attachment 343776 [details]
Patch

Attachment 343776 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8367936

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 343804 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Saam Barati]

Comment on attachment 343776 [details]
Patch

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

r=me
This patch looks good to me, but I think it's worth getting someone else's eyes on it too.

Let's also discuss in person if we want to commit it now or hold off a bit.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:240
> +static inline String sandboxParentDirectory()

Might be worth a comment saying we've already set the DIRHELPER_USER_DIR_SUFFIX environment variable here. Or you could have a debug assert w/ getenv

> Source/WebKit/Shared/mac/ChildProcessMac.mm:301
> +    constexpr size_t alphaNumRange = sizeof(alphaNum) - 1;

strlen here seems more intuitive.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:302
> +    String string;

This should use StringBuilder and return the builder.toString() at the end.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:305
> +        string.append(alphaNum[rand() % alphaNumRange]);

We should probably use WTF::randomNumber() here just for consictency.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:335
> +    if (errno == EEXIST) {

will this ever be EAGAIN?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:355
> +        FileSystem::makeAllDirectories(movedDirectory);

Maybe this should be part of the loop since this can fail?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:364
> +                FileSystem::deleteFile(file);

Should we verify it got deleted? Seems like a vulnerability if it doesn't

[Geoffrey Garen]

Comment on attachment 343776 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:366
> +        // Let's convert it to a data vault then move out their files
> +        if (rootless_convert_to_datavault(directoryPath.data(), storageClass)) {
> +            WTFLogAlways("%s: Sandbox directory couldn't be created, errno: %d", getprogname(), errno);
> +            return false;
> +        }
> +        
> +        // Create a new directory to move old files to
> +        WTFLogAlways("%s: \"%s\" is using a reserved path. Attempting to move it.", getprogname(), directoryPath.data(), directoryPath.data());
> +        String movedDirectory = info.directoryPath + ".Old";
> +        while (FileSystem::fileIsDirectory(movedDirectory, FileSystem::ShouldFollowSymbolicLinks::No)) {
> +            // Keep trying until we create the directory
> +            movedDirectory = info.directoryPath + ".Old+" + randomString();
> +        }
> +        FileSystem::makeAllDirectories(movedDirectory);
> +        
> +        // Now let's move everything to the new directory
> +        Vector<String> directoryContents = FileSystem::listDirectory(info.directoryPath, "*");
> +        for (const auto& file : directoryContents) {
> +            String fileName = FileSystem::pathGetFileName(file);
> +            String newPath = FileSystem::pathByAppendingComponent(movedDirectory, fileName);
> +            if (!FileSystem::moveFile(file, newPath)) {
> +                WTFLogAlways("%s: \"%s\" could not be moved and must be deleted", getprogname(), FileSystem::fileSystemRepresentation(file).data());
> +                FileSystem::deleteFile(file);
> +            }
> +        }

This behavior seems a bit whacky. Why do we believe that some other app might make a folder named "WebKit's data vault for sandboxes" or whatever? If we do have such an unlikely collision, why is it OK to move someone else's files?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:402
> +    // Create a sandbox profile that must be freed by the caller of this function
> +    char* error = nullptr;
> +    CString profilePath = FileSystem::fileSystemRepresentation(info.profilePath);
> +    if (profilePath.isNull())
> +        return nullptr;
> +    SandboxProfile* sandboxProfile = sandbox_compile_file(profilePath.data(), info.sandboxParameters, &error);

Instead of a comment, you can use std::shared_ptr with a custom deleter in order to guarantee correct lifetime behavior.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:463
> +    // Find a temporary filename to write to
> +    FileSystem::PlatformFileHandle tempHandle;
> +    String tempFileString;
> +    CString tempFilePath;
> +    while (true) {
> +        tempFileString = info.directoryPath + "/tempfile+" + randomString();
> +        tempFilePath = FileSystem::fileSystemRepresentation(tempFileString);
> +        if (tempFilePath.isNull())
> +            return nullptr;
> +        tempHandle = open(tempFilePath.data(), (O_WRONLY | O_CREAT | O_EXCL), 0666);
> +        if (tempHandle != FileSystem::invalidPlatformFileHandle)
> +            break;
> +        if (errno != EEXIST)
> +            return nullptr;
> +    }
> +    ASSERT(tempHandle != FileSystem::invalidPlatformFileHandle);
> +
> +    // Attempt to rename the temporary file to its proper name (this operation is atomic)
> +    bool didRenameFile = false;
> +    int writeSize = FileSystem::writeToFile(tempHandle, bitwise_cast<const char*>(fileContents.data()), safeCast<int>(fileContents.size()));
> +    if (writeSize == safeCast<int>(fileContents.size())) {
> +        CString sandboxFilePath = FileSystem::fileSystemRepresentation(info.filePath);
> +        if (!sandboxFilePath.isNull()) {
> +            // According to POSIX compliance requirements, this should not only rename the temporary
> +            // file but also overwrite any existing file with the same name atomically.
> +            // See http://pubs.opengroup.org/onlinepubs/9699919799/ for more information.
> +            if (!rename(tempFilePath.data(), sandboxFilePath.data()))
> +                didRenameFile = true;
> +        }
> +    }
> +
> +    FileSystem::closeFile(tempHandle);
> +    if (!didRenameFile)
> +        FileSystem::deleteFile(tempFileString);

This can be a helper function.

[Saam Barati]

Comment on attachment 343776 [details]
Patch

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

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:366
>> +        }
> 
> This behavior seems a bit whacky. Why do we believe that some other app might make a folder named "WebKit's data vault for sandboxes" or whatever? If we do have such an unlikely collision, why is it OK to move someone else's files?

I agree this is whacky, but what would our other option here be? Here are the options I think we have:
- move the directory
- delete the directory
- crash
- do nothing

move the directory: this seems like the best option, since it at least doesn't delete files. I think probabilistically, the only time there will be a file here is if someone is trying to attack webkit

delete the directory: I also think this is a good option for the same reason above: I don't think we'll go down this code path unless someone is trying to attack webkit

crash: This seems bad. This will make it easy to DOS webkit/safari

do nothing: We can't do this because it's a security vulnerability

(In reply to Saam Barati from comment #91)
> Comment on attachment 343776 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343776&action=review
> 
> >> Source/WebKit/Shared/mac/ChildProcessMac.mm:366
> >> +        }
> > 
> > This behavior seems a bit whacky. Why do we believe that some other app might make a folder named "WebKit's data vault for sandboxes" or whatever? If we do have such an unlikely collision, why is it OK to move someone else's files?
> 
> I agree this is whacky, but what would our other option here be? Here are
> the options I think we have:
> - move the directory
> - delete the directory
> - crash
> - do nothing

Is there also an option to take a safe but slow path of simply not using the cache?

[Saam Barati]

Comment on attachment 343776 [details]
Patch

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

>>>> Source/WebKit/Shared/mac/ChildProcessMac.mm:366
>>>> +        }
>>> 
>>> This behavior seems a bit whacky. Why do we believe that some other app might make a folder named "WebKit's data vault for sandboxes" or whatever? If we do have such an unlikely collision, why is it OK to move someone else's files?
>> 
>> I agree this is whacky, but what would our other option here be? Here are the options I think we have:
>> - move the directory
>> - delete the directory
>> - crash
>> - do nothing
>> 
>> move the directory: this seems like the best option, since it at least doesn't delete files. I think probabilistically, the only time there will be a file here is if someone is trying to attack webkit
>> 
>> delete the directory: I also think this is a good option for the same reason above: I don't think we'll go down this code path unless someone is trying to attack webkit
>> 
>> crash: This seems bad. This will make it easy to DOS webkit/safari
>> 
>> do nothing: We can't do this because it's a security vulnerability
> 
> Is there also an option to take a safe but slow path of simply not using the cache?

Yeah good point. This is another option. We can fall back to what we do today.

The downside I see with doing that is we will then have a path of code that's only taken when people try to pwn webkit. Perhaps we can force this mode in our tests, but it still feels a bit precarious to rely on the safety of our sandbox being applied properly in mostly untested code.

[Saam Barati]

I'd also like to say that my r+ is contingent on other folks being ok with your build system changes. Your changes seem reasonable to me, but I'm not knowledgable enough in that code to say definitively.

[Ben Richards]

Created attachment 344130 [details]
Patch

[Daniel Bates]

Comment on attachment 344130 [details]
Patch

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

I have not finished reviewing this patch. I want to see another iteration.

> Source/WebKit/Shared/ChildProcess.h:49
> +        WebContentType,

The suffix “Type” is unnecessary on each enumerator given that these are scoped to ProcessType. Please remove the suffix.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:57
>  #include <HIServices/ProcessesPriv.h>

I know you did not write this code. We should change this line to use #import.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:58
> +#endif // USE(APPLE_INTERNAL_SDK)

Please remove the inline comment as the code block it is annotating is small and can fit in a single screen. Annotating the end of a macro is most beneficial when the code block spans more than one screen.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:102
> +    const SandboxInitializationParameters &initializationParameters;

The & should be in the left since this variables has a C++ data type. You also put the & on the wrong side in the constructor signature below.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:104
> +    SandboxInfo(const SandboxInitializationParameters &parameters)

Do we usually put functions of a struct at the bottom? I was under the impression that we use the same form as for classes and put functions at the top and variables at the bottom.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:105
> +        : initializationParameters(parameters)

Can we use universal initialization syntax?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:110
> +static constexpr uint32_t CachedSandboxVersionNumber = 0;

Please remove the “static”. Constant expressions have internal linkage.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:167
> +    FileSystem::PlatformFileHandle handle = openFile(path, FileSystem::FileOpenMode::Read);

I suggest that we use the convenience class FileHandle, <https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/FileHandle.h#L36>, instead of FileSystem to get a handle to a file. The former wraps a platform file handle and will automatically close the file on destruction. Then we can remove closeFileOnExit below.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:177
> +    if (!FileSystem::getFileSize(handle, fileSize))

Does this provide a measurable performance improvement? If not, I suggest we remove it and use the file reading logic below handle the case of an empty file.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:183
> +    int bytesRead = FileSystem::readFromFile(handle, contents.data(), safeCast<size_t>(fileSize));

Is it necessary to compute the file size in advance to read the entire file? Can we read fixed size chunks until FileHandle::read() told us there are no more bytes? The benefit of this approach is that we avoid an unnecessary stat’ing of the FileSystem as well as simplify the code in this function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:213
> +    for (size_t i = 0; i < initializationParameters.count(); ++i) {

I take it we cannot use a C++11 ranged-based for-loop to iterate over the parameters. We should cache count() in a local to avoid re-computing it on each iteration.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:217
> +        if (name.isNull() || value.isNull())

How can we ever have a null name or null value? It seems like this would represent a programming mistake and hence we should use an assertion to catch this assuming it can even happen based on how the code is structured.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:235
> +        return std::nullopt;

This line does not conform to the WebKit Code Style Guidelines. A return statement should not be part of any else clause.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:241
> +        header.append(piece.data(), piece.length());

Although unlikely to cause an issue on most platforms, this code assumes that sizeof(uint8_t) == sizeof(char). One way to remove this assumption is to standardize on uint8_t for the headerPieces.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:244
> +    header.append(bitwise_cast<uint8_t *>(profileContents.data()), profileContents.size());

Although unlikely to cause an issue on most platforms, this code assumes that sizeof(uint8_t) == sizeof(char). One way to remove this assumption is to read the file directly into a uint8_t buffer.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:252
> +    char darwinUserCacheDir[PATH_MAX + 1];

As pointed out before, is the “+ 1” needed? If POSIX? PATH_MAX includes the null terminator then we should not add “+ 1” here.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:254
> +        char temp[PATH_MAX + 1];

Ditto

> Source/WebKit/Shared/mac/ChildProcessMac.mm:255
> +        if (!confstr(_CS_DARWIN_USER_CACHE_DIR, temp, sizeof(temp))) {

We are underutilizing the return value of confstr(). It returns the size of the value written into the buffer. If this return value > sizeof(temp) then we know it truncated the value and should handle this case appropriately. Additionally we can use the knowledge of the size of the buffer to invoke the String() constructor that takes a buffer and length and avoid having String call strlen().

> Source/WebKit/Shared/mac/ChildProcessMac.mm:263
> +    return darwinUserCacheDir;

This is suboptimal because String will need to compute the length of the buffer. The confstr() API returns the size of the buffer. We should pass this info when constructing the string.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:268
> +    String directory = sandboxParentDirectory();

We should use StringBuilder to build this string efficiently and avoid unnecessary malloc()s and computing the length of string literals.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:285
> +    // Add .OpenSource suffix so that non-internal builds don't try to access a data vault used by system Safari

Nit: Missing a period at the end of this sentence.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:294
> +    String sandboxFile = info.directoryPath;

We should use StringBuilder to efficiently build the string in this function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:308
> +constexpr size_t compileTimeStrlen(const char *str)

This is unnecessary. Please use WTF_ARRAY_SIZE() and remove this function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:315
> +    constexpr char alphaNum[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";

Maybe a better number for this would be alphaNumericScanSet?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:316
> +    constexpr size_t alphaNumRange = compileTimeStrlen(alphaNum);

The word “range” is typically used to represent a pair of numbers to describe a section of content. We typically use “length” when talking about the. Inner of characters in a string.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:318
> +    

I do not see the need for this empty line as I do not feel it improves the readability of this code. Please remove.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:327
> +    // First we need to ensure that at least the parent directory exists

This comment saids what the code does. Either explain why or remove this comment.

[Daniel Bates]

Comment on attachment 344130 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:412
> +    String tempFileString;

We should use StringBuilder to build up the string in this function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:419
> +        tempHandle = open(tempFilePath.data(), (O_WRONLY | O_CREAT | O_EXCL), 0666);

Can we use the convenience class FileHandle here? We may need to teach it how to create a file with a certain permission mode.

[Saam Barati]

Comment on attachment 344130 [details]
Patch

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

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:183
>> +    int bytesRead = FileSystem::readFromFile(handle, contents.data(), safeCast<size_t>(fileSize));
> 
> Is it necessary to compute the file size in advance to read the entire file? Can we read fixed size chunks until FileHandle::read() told us there are no more bytes? The benefit of this approach is that we avoid an unnecessary stat’ing of the FileSystem as well as simplify the code in this function.

is that atomic?

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:308
>> +constexpr size_t compileTimeStrlen(const char *str)
> 
> This is unnecessary. Please use WTF_ARRAY_SIZE() and remove this function.

also, "char *" => "char* "

[Daniel Bates]

Comment on attachment 344130 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:453
> +    // Create a sandbox profile that must be freed by the caller of this function

I’m confused. We are returning a smart pointer class.. So, the caller does not need to do anything. Please remove this comment.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:464
> +    // Begin building our file to cache

This comments saids what the code does. Either explain why or please remove this comment.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:473
> +    // Make sure to set header OS version

Ditto

> Source/WebKit/Shared/mac/ChildProcessMac.mm:475
> +    if (sysctlbyname("kern.osversion", cachedHeader.osVersion, &osVersionSize, NULL, 0))

NULL => nullptr

> Source/WebKit/Shared/mac/ChildProcessMac.mm:478
> +    // Write contents into a temporary vector

This comment saids what the code codes. Please remove this comment.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:509
> +        return false;

This line does not conform our code style guidelines.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:511
> +    // Make sure cachedSandboxContents aren't too small to contain CachedSandboxHeader

Why? Or remove this comment.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:515
> +    // Read CachedSandboxHeader from top of file

Ditto.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:519
> +    // Verify cached sandbox was compiled for the same OS version

Ditto.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:520
> +    char *osVersionBuffer;

* on the wrong side.

[Daniel Bates]

Comment on attachment 344130 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:90
> +// SandboxHeader <- CachedSandboxHeader::sandboxHeaderSize bytes

Did you mean CachedSandboxHeader::headerSize?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:91
> +// [SandboxBuiltin] optional. Present if CachedSandboxHeader::sanboxBuiltinSize is not UINT_MAX. If present, sandboxBuiltinSize bytes (not including null termination).

Did you mean CachedSandboxHeader::builtinSize?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:92
> +// SandboxData <- CachedSandboxHeader::sandboxDataSize bytes

Did you mean CachedSandboxHeader::dataSize?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:239
> +    Vector<uint8_t> header(headerSize.unsafeGet());

We should assert that headerPieces.size() is a multiple of two.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:459
> +    if (!sandboxProfile) {

I am assuming that error is non-null only if sandboxProfile is null. That is, it is never possible for sandboxProfile to be null and error be non-null.

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:475
>> +    if (sysctlbyname("kern.osversion", cachedHeader.osVersion, &osVersionSize, NULL, 0))
> 
> NULL => nullptr

What is the purpose of storing the OS version? I see the value of storing our own version number (for future-proofing) and storing the version of libsandbox the sandbox was compiled with (in case a binary incompatible change is made to libsandbox).

> Source/WebKit/Shared/mac/ChildProcessMac.mm:481
> +    fileContents.append(info.header.data(), info.header.size());

This is weird. We converted the value stored into info.header from char to unt8_t (in sandboxHeader()) only to convert it back to char here. Can we just keep the header as a CString?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:517
> +    memcpy(&cachedSandboxHeader, cachedSandboxContents.data(), sizeof(CachedSandboxHeader));

This assumes that endianness does not change. This is likely acceptable since all this effort is a performance optimization. Regardless, we should add a comment to explain that endianness does not matter because this effort is all part of a performance optimization and we will re-compile the sandbox if needed.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:545
> +    const char* sandboxHeaderPointer = cachedSandboxContents.data() + sizeof(CachedSandboxHeader);
> +    const char* sandboxBuiltinPointer = sandboxHeaderPointer + cachedSandboxHeader.headerSize;
> +    const char* sandboxDataPointer = haveBuiltin ? sandboxBuiltinPointer + cachedSandboxHeader.builtinSize : sandboxBuiltinPointer;

Is there any way to write the computation of expectedFileSize in terms of pointer arithmetic? On another note, I suggest we change the datatype of sandboxDataPointer to be unsigned char* to avoid the need to case it as such on line 563.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:566
> +        WTFLogAlways("Could not apply cached sandbox");

I suggest we take a similar approach as when we called sandbox_apply() in applySandbox() and include the errno in the logged message on failure to help debugging?

[Ben Richards]

Comment on attachment 344130 [details]
Patch

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

>>> Source/WebKit/Shared/mac/ChildProcessMac.mm:183
>>> +    int bytesRead = FileSystem::readFromFile(handle, contents.data(), safeCast<size_t>(fileSize));
>> 
>> Is it necessary to compute the file size in advance to read the entire file? Can we read fixed size chunks until FileHandle::read() told us there are no more bytes? The benefit of this approach is that we avoid an unnecessary stat’ing of the FileSystem as well as simplify the code in this function.
> 
> is that atomic?

I don't know if this change would be atomic. If it's not atomic then there's a chance here that the contents will be corrupted if another process overwrites the file which could cause the process reading the file to recompile the sandbox unnecessarily. This may be ok though since this type of collision should be very rare

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:239
>> +    Vector<uint8_t> header(headerSize.unsafeGet());
> 
> We should assert that headerPieces.size() is a multiple of two.

The isNull check on name and value on line 217 should guarantee this

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:263
>> +    return darwinUserCacheDir;
> 
> This is suboptimal because String will need to compute the length of the buffer. The confstr() API returns the size of the buffer. We should pass this info when constructing the string.

realpath will expand symbolic links and resolve '/./' and '/../' references so unless we want to remove that call then we will still have to let the String constructor call strlen

[Ben Richards]

Comment on attachment 344130 [details]
Patch

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

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:545
>> +    const char* sandboxDataPointer = haveBuiltin ? sandboxBuiltinPointer + cachedSandboxHeader.builtinSize : sandboxBuiltinPointer;
> 
> Is there any way to write the computation of expectedFileSize in terms of pointer arithmetic? On another note, I suggest we change the datatype of sandboxDataPointer to be unsigned char* to avoid the need to case it as such on line 563.

expectedFileSize must be computed using the metadata stored in cachedSandboxHeader as a way to verify that cached sandbox is valid.

[Daniel Bates]

Comment on attachment 344130 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:445
> +    // Find a temporary filename to write to
> +    FileSystem::PlatformFileHandle tempHandle;
> +    String tempFileString;
> +    CString tempFilePath;
> +    while (true) {
> +        tempFileString = info.directoryPath + "/tempfile+" + randomString();
> +        tempFilePath = FileSystem::fileSystemRepresentation(tempFileString);
> +        if (tempFilePath.isNull())
> +            return nullptr;
> +        tempHandle = open(tempFilePath.data(), (O_WRONLY | O_CREAT | O_EXCL), 0666);
> +        if (tempHandle != FileSystem::invalidPlatformFileHandle)
> +            break;
> +        if (errno != EEXIST)
> +            return nullptr;
> +    }
> +    ASSERT(tempHandle != FileSystem::invalidPlatformFileHandle);
> +    
> +    // Attempt to rename the temporary file to its proper name (this operation is atomic)
> +    bool didRenameFile = false;
> +    int writeSize = FileSystem::writeToFile(tempHandle, bitwise_cast<const char*>(fileContents.data()), safeCast<int>(fileContents.size()));
> +    if (writeSize == safeCast<int>(fileContents.size())) {
> +        CString sandboxFilePath = FileSystem::fileSystemRepresentation(info.filePath);
> +        if (!sandboxFilePath.isNull()) {
> +            // According to POSIX compliance requirements, this should not only rename the temporary
> +            // file but also overwrite any existing file with the same name atomically.
> +            // See http://pubs.opengroup.org/onlinepubs/9699919799/ for more information.
> +            if (!rename(tempFilePath.data(), sandboxFilePath.data()))
> +                didRenameFile = true;
> +        }
> +    }
> +    
> +    FileSystem::closeFile(tempHandle);
> +    if (!didRenameFile)
> +        FileSystem::deleteFile(tempFileString);
> +    
> +    return didRenameFile;

Ben and I talked about this some more today. It seems sufficient to use FileSystem::openAndLockFile() to open the destination cached sandbox file directly and then write the compiled sandbox definition into it. Together with using the non-blocking form of flock() when reading the cached sandbox file (*), this approach will be multi-processing safe and has less moving parts.

(*) If one process detects the presence of a lock then it seems sufficient to fallback to using the non-optimized code path of compiling the sandbox. Eventually the process that acquired the file lock will write the compiled sandbox definition to the cache file and subsequent processes will be able to make use of it.

[Geoffrey Garen]

> > This behavior seems a bit whacky. Why do we believe that some other app might make a folder named "WebKit's data vault for sandboxes" or whatever? If we do have such an unlikely collision, why is it OK to move someone else's files?
> 
> I agree this is whacky, but what would our other option here be? Here are
> the options I think we have:
> - move the directory
> - delete the directory
> - crash
> - do nothing
> 
> move the directory: this seems like the best option, since it at least
> doesn't delete files. I think probabilistically, the only time there will be
> a file here is if someone is trying to attack webkit

I still don't like moving the files because it's a weird outcome, and there's no reason to believe that whoever put those files there will continue to function correctly after we move them.

Either we believe that we've picked a reasonably unique directory name or we don't. If yes, we can consider it ours for all practical purposes. If no, we can't use it at all. (This problem applies to all WebKit storage, BTW.)

Realistically speaking, we're talking about a path such as "~/Library/Caches/com.apple.WebKit.WebContent.Sandbox", right? I think it's OK to say we own that path. That's the purpose of reverse domain name strings.

> delete the directory: I also think this is a good option for the same reason
> above: I don't think we'll go down this code path unless someone is trying
> to attack webkit

Yup, let's do this.

> crash: This seems bad. This will make it easy to DOS webkit/safari

Yup, this is bad. Crash on launch forever.

> do nothing: We can't do this because it's a security vulnerability

I think the best version of "do nothing" would mean "just don't cache the sandbox". That might be OK.

But I don't see any need for it. I believe we own this directory name.

[Geoffrey Garen]

> I think the best version of "do nothing" would mean "just don't cache the
> sandbox". That might be OK.

(I guess this is Comment 92.)

[Ben Richards]

Created attachment 344805 [details]
Patch

[EWS Watchlist]

Comment on attachment 344805 [details]
Patch

Attachment 344805 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8512692

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344832 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ryosuke Niwa]

Comment on attachment 344805 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        (3) In order to to create process specific data vaults, each process now has their own <process name>-OSX-sandbox.entitlements

Nit: In order to *to*

> Source/WebKit/ChangeLog:16
> +        (4) The sandbox entitlements file for the Network, WebContent and Plugin services are loaded dynamically

We should explain why we need to load these entitlements dynamically.

> Source/WebCore/platform/FileHandle.cpp:126
> +    if (m_shouldLock && *this)

I think this code warrants a comment saying that unlockAndCloseFile requires the file handle to be valid.

> Source/WebCore/platform/mac/FileSystemMac.mm:52
> +bool FileSystem::deleteNonEmptyDirectory(const String& path)

I think we can have this in FileSystem.cpp instead since there is no Darwin specific code here.

> Source/WebCore/platform/mac/FileSystemMac.mm:54
> +    std::queue<String> files;

Why don't we use Deque instead?

> Source/WebCore/platform/mac/FileSystemMac.mm:59
> +        String removed = WTFMove(files.front());
> +        files.pop();

Then you can use takeFirst here.

> Source/WebCore/platform/mac/FileSystemMac.mm:60
> +        

Nit: whitespace.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:108
> +    : sandboxParameters { sandbox_create_params() }
> +    , initializationParameters { parameters }

Nit: Needs to be indented.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:213
> +static std::optional<CString> sandboxHeader(const SandboxInitializationParameters& initializationParameters, const SandboxInfo& info)

We need more descriptive name like like setSandboxParametersAndSerializedHeader?
or setAndSerializeSandboxParameters?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:219
> +        

Nit: Whitespace here and the rest of the function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:236
> +        profileContents = String::adopt(WTFMove(*contents));

Why don't we do builder.append(contents.data(), contents.length()) here?
like.
auto contents = fileContents(info.profilePath);
if (!contents)
    return std::nullopt;
builder.append(contents.data(), contents.length());

> Source/WebKit/Shared/mac/ChildProcessMac.mm:551
> +    SandboxInfo info(sandboxParameters);

Why not { sandboxParameters, parameters.processType, etc... }?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:641
> +    String libraryPath = path;

Seems like this variable is not used anywhere.

[Ryosuke Niwa]

Comment on attachment 344805 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        (3) In order to to create process specific data vaults, each process now has their own <process name>-OSX-sandbox.entitlements

Nit: In order to *to*

> Source/WebKit/ChangeLog:16
> +        (4) The sandbox entitlements file for the Network, WebContent and Plugin services are loaded dynamically

We should explain why we need to load these entitlements dynamically.

> Source/WebCore/platform/FileHandle.cpp:126
> +    if (m_shouldLock && *this)

I think this code warrants a comment saying that unlockAndCloseFile requires the file handle to be valid.

> Source/WebCore/platform/mac/FileSystemMac.mm:52
> +bool FileSystem::deleteNonEmptyDirectory(const String& path)

I think we can have this in FileSystem.cpp instead since there is no Darwin specific code here.

> Source/WebCore/platform/mac/FileSystemMac.mm:54
> +    std::queue<String> files;

Why don't we use Deque instead?

> Source/WebCore/platform/mac/FileSystemMac.mm:59
> +        String removed = WTFMove(files.front());
> +        files.pop();

Then you can use takeFirst here.

> Source/WebCore/platform/mac/FileSystemMac.mm:60
> +        

Nit: whitespace.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:108
> +    : sandboxParameters { sandbox_create_params() }
> +    , initializationParameters { parameters }

Nit: Needs to be indented.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:641
> +    String libraryPath = path;

Seems like this variable is not used anywhere.

[Ryosuke Niwa]

Comment on attachment 344805 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:188
> +    contents.reserveInitialCapacity(chunkSize * 2);

Why don't we call getFileSize and set the buffer size to the right size right away?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:444
> +    // Need to make sure the sandbox directory is a data vault for security reasons.

Probably don't need this comment since it's pretty clear from the semantics of code.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:460
> +    // Read CachedSandboxHeader from top of file. This data may be currupted if the sandbox file was cached on a different

I don't we need a verbose comment like this.
Just mention that "This data may be corrupted if the sandbox file was cached on a different platform with different endianness"

> Source/WebKit/Shared/mac/ChildProcessMac.mm:469
> +    const bool haveBuiltin = (cachedSandboxHeader.builtinSize != std::numeric_limits<uint32_t>::max());

Nit: I don't think we need parenthesis here.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:473
> +    const char* sandboxHeaderPtr = bitwise_cast<char *>(cachedSandboxContents.data()) + sizeof(CachedSandboxHeader);

Let's use Persistence::Encoder & Persistence::Decoder here.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:493
> +    profile.size = cachedSandboxHeader.dataSize;

Why don't we add a ASSERT that sandboxDataPtr + profile.size <= end-of-file pointer.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:562
> +        // Order here is important! Header must be set before calling sandboxFilename(info).

But why? Comments should explain the reasons, not what we need to do.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:566
> +    if (info.header.isNull()) {

Seems like this is just else? Can info.header.isNull be true even when the above if condition is true?

[Ben Richards]

Created attachment 345206 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 345206 [details]
Patch

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

> Source/WebCore/platform/mac/FileSystemMac.mm:33
> +#import <queue>

Nit: Seems unnecessary.

> Source/WebKit/Scripts/process-network-entitlements.sh:11
> +        echo "Adding sandbox entitlements.";

Maybe we should say which process so that it's easier to debug when it fails?

> Source/WebKit/Scripts/process-plugin-entitlements.sh:11
> +        echo "Adding sandbox entitlements.";

Ditto.

> Source/WebKit/Scripts/process-webcontent-entitlements.sh:11
> +        echo "Adding sandbox entitlements.";

Ditto.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:190
> +    long contentSize = 0;

Nit: Why not size_t?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:191
> +    Vector<char> contents;

Can we use uint8_t??

> Source/WebKit/Shared/mac/ChildProcessMac.mm:249
> +static inline String sandboxParentDirectory()
> +{
> +    // DIRHELPER_USER_DIR_SUFFIX is set in initializeSandboxParameters (called before this function).
> +    static char parentDirectory[PATH_MAX];
> +    static size_t parentDirectoryLength;
> +    static bool parentDirectoryIsSet = false;

Instead of storing these static variables, can we just pass the directory path around?
If we must, we should use static NeverDestroyed<String>

> Source/WebKit/Shared/mac/ChildProcessMac.mm:250
> +    

Nit: Whitespace and in the rest of the function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:303
> +    // Add .OpenSource suffix so that non-internal builds don't try to access a data vault used by system Safari.

Probably should say "so that opens source builds" instead.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:317
> +    auto crypto = PAL::CryptoDigest::create(PAL::CryptoDigest::Algorithm::SHA_1);

Let's use SHA-2.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:364
> +        if ((isDirectory && !FileSystem::deleteNonEmptyDirectory(info.directoryPath)) || (!isDirectory && !FileSystem::deleteFile(info.directoryPath)))
> +            return false;

Why don't we just do:
if (isDirectory) {
  if (~)
} else {
  if (~)
}
to improve the readability.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:467
> +    

Nit: Whitespace.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:486
> +    // Begin building up a SandboxProfile instance with the information we've gathered that we can pass to sandbox_apply.

I don't think this comment is necessary.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:561
> +        // If we reach this then there is something wrong with the sandboxParameters, therefore we should crash.

I don't think we need this comment.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:567
> +    // Try to find a cached version of the sandbox and apply it.
> +    if (applyCachedSandbox(info))

Again, we should just rename applyCachedSandbox to tryApplyCachedSandbox and remove this comment.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:570
> +    // We failed to find a valid sandbox on the fs so let's compile and cache one for the next process.

I don't think we need this comment.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:573
> +        // Something went wrong so we need to fall back to the slow but safe method.

Ditto.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:574
> +        return applySandboxSafeButSlow(info);

Instead of saying butSlow just say compileAndApplySandbox or compileAndApplySandboxSlowCase.

[EWS Watchlist]

Comment on attachment 345206 [details]
Patch

Attachment 345206 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8578323

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 345287 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ben Richards]

Created attachment 345311 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 345311 [details]
Patch

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

> Source/WebCore/platform/FileSystem.cpp:374
> +bool deleteNonEmptyDirectory(const String& path)
> +{

Alex says we have a method to do this in NSFileManager. Let's use that and put this in FileSystemCocoa.mm

> Source/WebKit/Shared/mac/ChildProcessMac.mm:311
> +    auto crypto = PAL::CryptoDigest::create(PAL::CryptoDigest::Algorithm::SHA_224);

Use SHA_256?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:315
> +    String readableHash = base64Encode(hash, WTF::Base64URLPolicy);
> +    readableHash.replace('/', '_');

I think we wanna use base64URLEncode so that we don't have to do manual replacement.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:394
> +    if (file.write(cacheFile.data(), cacheFile.size()) != safeCast<int>(cacheFile.size()))

Just return file.write(~) == safeCast... here without a if statement.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:451
> +    if (auto contents = fileContents(info.filePath, true, FileSystem::FileLockMode::Shared))
> +        cachedSandboxContents = WTFMove(*contents);

Better to early return when contents is falsey as in:
auto contents =~
if (!contents || !contents->isEmpty())
    return false;

> Source/WebKit/Shared/mac/ChildProcessMac.mm:480
> +        // Header and cached header do not have the same contents so sandbox parameters must have changed and we need to recompile.

I don't think we need this comment. The code makes it clear what we're checking.

[EWS Watchlist]

Comment on attachment 345311 [details]
Patch

Attachment 345311 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/8581725

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate
apiTests

[EWS Watchlist]

Comment on attachment 345311 [details]
Patch

Attachment 345311 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8583048

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 345327 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Daniel Bates]

Comment on attachment 345311 [details]
Patch

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

Ben and I went over this patch and I noticed some corr corneas issues and other stylistic issues. I would like to see another iteration.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:265
> +    temp.resize(neededBufferSize);

This will destroy any data already written into the buffer because the size of the vector is 0.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:267
> +        RELEASE_ASSERT(temp.size() == confstrWithCheck());

Can this assert be disabled?

[Ben Richards]

Created attachment 345377 [details]
Patch

[EWS Watchlist]

Attachment 345377 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 345377 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:338
> +    auto makeDataVault = [&]() -> bool {

Style nit: can just be
= [&] {

> Source/WebKit/Shared/mac/ChildProcessMac.mm:346
> +    if (makeDataVault())

Why isn’t this just a loop that does:
while (1) {
if (isDataVault()) return true
if (tryMakeDataVault()) return true
tryDeleteDirectory()
}

I don’t think the code you have suffers from a TOCTOU bug, but it’s structured in a way that seems more complicated than needed

[Ben Richards]

Comment on attachment 345377 [details]
Patch

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

>> Source/WebKit/Shared/mac/ChildProcessMac.mm:346
>> +    if (makeDataVault())
> 
> Why isn’t this just a loop that does:
> while (1) {
> if (isDataVault()) return true
> if (tryMakeDataVault()) return true
> tryDeleteDirectory()
> }
> 
> I don’t think the code you have suffers from a TOCTOU bug, but it’s structured in a way that seems more complicated than needed

I'm not sure that we want to use a loop here.
The only reason this structure should fail is because of an attack.
In the case of an attack it's best just to fall back to compileAndApplySandboxSlowCase instead of trying again.

[Ben Richards]

Created attachment 345392 [details]
Patch

[EWS Watchlist]

Attachment 345392 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 345392 [details]
Patch

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

> Source/WebCore/platform/FileSystem.h:104
> +WEBCORE_EXPORT bool deleteNonEmptyDirectory(const String&);

We should probably place this under #if PLATFORM(COCOA) since there's only implementation under COCOA.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:256
> +    auto writeUserCacheDirectoryIntoBuffer = [&] () {

Why does this need to be a lambda? It seems cleaner to have a static function which takes Vector<char>&.
If we did that, we should probably rename this to getUserCacheDirectory (get prefix since it has an out parameter).

> Source/WebKit/Shared/mac/ChildProcessMac.mm:550
> +    CString header;

I don't think we need a separate local variable from the one returned by setAndSerializeSandboxParameters.
Just call setAndSerializeSandboxParameters outside the if like:
auto header = setAndSerializeSandboxParameters(~);
if (!header) {
    ...
    CRASH();
}
filePath = sandboxFilePath(directoryPath, *header);

> Source/WebKit/Shared/mac/ChildProcessMac.mm:572
> +    if (info.profilePath.isEmpty()) {

Why don't we check this condition before constructing SandboxInfo.
In fact, can't we check this condition before calling setAndSerializeSandboxParameters, no?

[Ben Richards]

Created attachment 345405 [details]
Patch for landing

[Daniel Bates]

Comment on attachment 345405 [details]
Patch for landing

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:277
> +    return String(parentDirectory);

Do we need to explicitly call the String constructor?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:299
> +#if !(USE(APPLE_INTERNAL_SDK))

The outermost parentheses are not needed.

[Ben Richards]

Created attachment 345588 [details]
Patch for landing

[Ben Richards]

Created attachment 345598 [details]
Patch for landing

[EWS Watchlist]

Attachment 345598 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 345598 [details]
Patch for landing

Clearing flags on attachment: 345598

Committed r234121: <https://trac.webkit.org/changeset/234121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42521496>

[Dawei Fenton (:realdawei)]

(In reply to WebKit Commit Bot from comment #134)
> Comment on attachment 345598 [details]
> Patch for landing
> 
> Clearing flags on attachment: 345598
> 
> Committed r234121: <https://trac.webkit.org/changeset/234121>

Performance Bots are failing after this revision:
Sample output:
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Perf%29/builds/1282/steps/perf-test/logs/stdio

Running 185 tests
Running Animation/balls.html (1 of 185)
error: Animation/balls.html
com.apple.WebKit.WebContent.Development: Could not apply cached sandbox
FAILED
Finished: 4.475824 s

Running Bindings/append-child.html (2 of 185)
error: Bindings/append-child.html
com.apple.WebKit.WebContent.Development: Could not apply cached sandbox
FAILED
Finished: 6.071629 s

Running Bindings/childNodes-traversal.html (3 of 185)
error: Bindings/childNodes-traversal.html
com.apple.WebKit.WebContent.Development: Could not apply cached sandbox
FAILED
Finished: 6.045380 s

Running Bindings/children-traversal.html (4 of 185)
error: Bindings/children-traversal.html
com.apple.WebKit.WebContent.Development: Could not apply cached sandbox
FAILED
Finished: 5.767535 s

Running Bindings/create-element.html (5 of 185)
error: Bindings/create-element.html
com.apple.WebKit.WebContent.Development: Could not apply cached sandbox
FAILED
Finished: 6.643981 s

Running Bindings/document-implementation.html (6 of 185)
error: Bindings/document-implementation.html
com.apple.WebKit.WebContent.Development: Could not apply cached sandbox
FAILED
Finished: 5.999480 s

[Geoffrey Garen]

Performance bots are important, so let's roll this patch out while we investigate.

[Ryan Haddad]

Reverted r234121 for reason:

Caused perf test failures.

Committed r234167: <https://trac.webkit.org/changeset/234167>

[Brent Fulgham]

(In reply to Geoffrey Garen from comment #138)
> Performance bots are important, so let's roll this patch out while we
> investigate.

This also breaks some plugins because there is code that tries to interpret the plugin source as a path, resulting in sadness.

[Geoffrey Garen]

> This also breaks some plugins because there is code that tries to interpret
> the plugin source as a path, resulting in sadness.

If it's easier, I think it's OK not to do this optimization for plugins.

[Ben Richards]

Created attachment 346440 [details]
Patch for landing

[EWS Watchlist]

Attachment 346440 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 346440 [details]
Patch for landing

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:485
> +    memset(&profile, 0, sizeof(SandboxProfile));

This is not necessary. It is sufficient to use aggregate initialization in he previous line:

SandboxProfile profile = { 0 };

[Ben Richards]

"SandboxProfile profile = { 0 };" throws an compile time error

[Ben Richards]

"error: missing field 'data' initializer [-Werror, -Wmissing-filed-initializers]"

[Daniel Bates]

(In reply to Ben Richards from comment #145)
> "SandboxProfile profile = { 0 };" throws an compile time error
(In reply to Ben Richards from comment #146)
> "error: missing field 'data' initializer [-Werror,
> -Wmissing-filed-initializers]"

How about:

SandboxProfile profile { };

?

[Daniel Bates]

Comment on attachment 346440 [details]
Patch for landing

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:278
> +static size_t getUserCacheDirectory(Vector<char>& buffer)
> +{
> +    size_t result = confstr(_CS_DARWIN_USER_CACHE_DIR, buffer.data(), buffer.size());
> +    if (!result) {
> +        WTFLogAlways("%s: Could not retrieve private cache directory path: %s\n", getprogname(), strerror(errno));
> +        exit(EX_NOPERM);
> +    }
> +    return result;
> +}
> +
> +static inline String sandboxDataVaultParentDirectory()
> +{
> +    // DIRHELPER_USER_DIR_SUFFIX is set in initializeSandboxParameters (called before this function).
> +    char parentDirectory[PATH_MAX];
> +    
> +    Vector<char> temp;
> +    temp.grow(PATH_MAX);
> +    
> +    size_t neededBufferSize = getUserCacheDirectory(temp);
> +    if (neededBufferSize > temp.size()) {
> +        temp.grow(neededBufferSize);
> +        size_t bufferSize = getUserCacheDirectory(temp);
> +        RELEASE_ASSERT(bufferSize == temp.size());
> +    }
> +    if (!realpath(temp.data(), parentDirectory)) {
> +        WTFLogAlways("%s: Could not retrieve private cache directory path: %s\n", getprogname(), strerror(errno));
> +        exit(EX_NOPERM);
> +    }
> +    
> +    return parentDirectory;
> +}

In sandboxDataVaultParentDirectory() we allow temp to grow larger than PATH_MAX only to truncate it to PATH_MAX when we call realpath(3). Moreover a failure of confstr(3) in getUserCacheDirectory() may be indistinguishable from a failure of realpath(3) because we emit the same error message up to strerror(errno). I suggest that we emit a different error message to make the logged failure reason unambiguous. Taking these suggestions into account and removing the unnecessary inline keyword (*) I would replace these functions with the following functions:

static String sandboxDataVaultParentDirectory()
{
    char temp[PATH_MAX];
    size_t length = confstr(_CS_DARWIN_USER_CACHE_DIR, temp, sizeof(temp));
    if (!length) {
        WTFLogAlways("%s: Could not retrieve user cache directory path: %s\n", getprogname(), strerror(errno));
        exit(EX_NOPERM);
    }
    RELEASE_ASSERT(length <= sizeof(temp));
    char resolvedPath[PATH_MAX];
    if (!realpath(temp, resolvedPath, sizeof(resolvedPath)) {
        WTFLogAlways("%s: Could not canonicalize user cache directory path: %s\n", getprogname(), strerror(errno));
        exit(EX_NOPERM);
    }
    return resolvedPath;
}

If we need to support a path longer than PATH_MAX then I suggest we restructure this code such that we call confstr(_CS_DARWIN_USER_CACHE_DIR, nullptr, 0) to get the size of the path, then allocate a CString of that length, then call confstr() again passing the buffer associated with the CString and its length, take advantage of the mac/iOS? support for having realpath() allocate a buffer of the correct length by invoking realpath() with a nullptr for the resolved name buffer and finally return the CString.

(*) Although the keyword inline is only a hint to the compiler we should only give such a hint when we have proven that it has a material performance impact. Even better, we should use the macro ALWAYS_INLINE to force inlining for such proven cases.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:280
> +static inline String sandboxDirectory(ChildProcess::ProcessType processType, const String& parentDirectory)

This should not be inline unless we have proven that it has a material performance impact.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:307
> +static inline String sandboxFilePath(const String& directoryPath, const CString& header)

Ditto.

[Daniel Bates]

(In reply to Daniel Bates from comment #148)
> > Source/WebKit/Shared/mac/ChildProcessMac.mm:278
> [...]
> If we need to support a path longer than PATH_MAX then ... call confstr() again passing the buffer associated with the CString and its length, take advantage of the mac/iOS? support for having ...

Disregard the remark about iOS. There is no iOS code in ChildProcessMac.mm.

[Ben Richards]

Created attachment 346558 [details]
Patch for landing

[EWS Watchlist]

Attachment 346558 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/mac/ChildProcessMac.mm:469:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 346558 [details]
Patch for landing

Clearing flags on attachment: 346558

Committed r234569: <https://trac.webkit.org/changeset/234569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r234569 for reason:

Breaks internal builds.

Committed r234613: <https://trac.webkit.org/changeset/234613>

[Ben Richards]

Created attachment 346743 [details]
Patch

[EWS Watchlist]

Attachment 346743 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 346743 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:62
> +#define CAN_CACHE_COMPILED_SANDBOX __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300 && !PLATFORM(IOSMAC)

We should use USE_ macro instead, and define this to be 1 or 0 based on the condition.
Also, we need to check PLATFORM(MAC) before checking the value of __MAC_OS_X_VERSION_MIN_REQUIRED.

[Ben Richards]

Created attachment 346810 [details]
Patch

[EWS Watchlist]

Attachment 346810 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 346810 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:200
> +    

Nit: Whitespace.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:211
> +    

Nit: Whitespace.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:348
> +        

Nit: Whitespace.

[Ben Richards]

Created attachment 346841 [details]
Patch for landing

[Ben Richards]

Let's let EWS run before cq+

[EWS Watchlist]

Attachment 346841 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 346841 [details]
Patch for landing

Clearing flags on attachment: 346841

Committed r234747: <https://trac.webkit.org/changeset/234747>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 188524

[Simon Fraser (smfr)]

Comment on attachment 346841 [details]
Patch for landing

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

> Source/WebCore/platform/FileHandle.h:64
> +    bool m_shouldLock { false };
> +    OptionSet<FileSystem::FileLockMode> m_lockMode;

Please order these for better packing (bool last)

> Source/WebKit/Shared/mac/ChildProcessMac.mm:288
> +    case ChildProcess::ProcessType::WebContent:
> +        directory.append("/com.apple.WebKit.WebContent.Sandbox");
> +        break;
> +    case ChildProcess::ProcessType::Network:
> +        directory.append("/com.apple.WebKit.Networking.Sandbox");
> +        break;
> +    case ChildProcess::ProcessType::Storage:
> +        directory.append("/com.apple.WebKit.Storage.Sandbox");
> +        break;
> +    case ChildProcess::ProcessType::Plugin:
> +        directory.append("/com.apple.WebKit.Plugin.Sandbox");
> +        break;

Can't you get the com.apple.WebKit.* parts from the bundle identifier? If we extend the number of process types, we should not have to change this code.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:293
> +    // Add .OpenSource suffix so that open source builds don't try to access a data vault used by system Safari.
> +    directory.append(".OpenSource");

Weird.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:519
> +static String sandboxProfilePath(const SandboxInitializationParameters& parameters)
> +{
> +    switch (parameters.mode()) {
> +    case SandboxInitializationParameters::ProfileSelectionMode::UseDefaultSandboxProfilePath:
> +        return [webKit2Bundle() pathForResource:[[NSBundle mainBundle] bundleIdentifier] ofType:@"sb"];
> +    case SandboxInitializationParameters::ProfileSelectionMode::UseOverrideSandboxProfilePath:
> +        return parameters.overrideSandboxProfilePath();
> +    case SandboxInitializationParameters::ProfileSelectionMode::UseSandboxProfile:
> +        return parameters.sandboxProfile();
> +    }
> +}

It's really confusing that the returns string is either a path or an entire profile; the function name doesn't make this clear, and this is what broke plug-ins.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:539
> +    String profilePath { sandboxProfilePath(sandboxInitializationParameters) };

But it's not always a path.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:554
> +        WTFLogAlways("%s: Sandbox parameters are invalid\n", getprogname());
> +        CRASH();

Plug-in processes hit this crash.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:572
> +    if (info.processType == ChildProcess::ProcessType::Plugin)
> +        return compileAndApplySandboxSlowCase(profilePath, sandboxInitializationParameters);

It's wrong to special-case process type here. You should keep some data around to know if you should treat "profilePath" as a path or a profile.

[Geoffrey Garen]

I think the easiest next step here is to reproduce this bug by enabling Flash and visiting a website with a Flash plug-in.

Seems like, in the plug-in case, what we have is a plug-in string and not a plug-in path.

[Saam Barati]

(In reply to Geoffrey Garen from comment #168)
> I think the easiest next step here is to reproduce this bug by enabling
> Flash and visiting a website with a Flash plug-in.
> 
> Seems like, in the plug-in case, what we have is a plug-in string and not a
> plug-in path.

Maybe we should just convert this to use the path method while we’re at it. Maybe there is something I’m missing, but it’s weird we have these two modes. It seems like the “I have a path” mode should work for all processes.

[Ben Richards]

Looks like the DARWIN_USER_TEMP_DIR and DARWIN_USER_CACHE_DIR sandbox parameters are randomized for the plugin process. Since they are randomized the compiled sandbox is going to be different each time so I don't think we can cache the sandbox due to the fact that a new compiled sandbox will be cached every time just filling up the filesystem. If we could remove those parameters then we could cache but I'm not sure that's a good idea.

[Ben Richards]

Created attachment 347144 [details]
Patch for landing

[EWS Watchlist]

Attachment 347144 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ben Richards]

Created attachment 347170 [details]
Patch

[EWS Watchlist]

Attachment 347170 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/FileHandle.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

(In reply to Ben Richards from comment #170)
> Looks like the DARWIN_USER_TEMP_DIR and DARWIN_USER_CACHE_DIR sandbox
> parameters are randomized for the plugin process. Since they are randomized
> the compiled sandbox is going to be different each time so I don't think we
> can cache the sandbox due to the fact that a new compiled sandbox will be
> cached every time just filling up the filesystem. If we could remove those
> parameters then we could cache but I'm not sure that's a good idea.

Makes sense. I agree we shouldn’t cache on the file system here.

[Ryosuke Niwa]

Comment on attachment 347170 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:671
> +    // This must be called before initializeSandboxParameters so that the path does not include the user directory suffix.
> +    // We don't want the user directory suffix because we want all processes of the same type to use the same cache directory.

A comment like this is an indication that this code isn't structured well.
I'd r+ this patch but we really need to do some refactoring once this initial patch lands.

[WebKit Commit Bot]

Comment on attachment 347170 [details]
Patch

Clearing flags on attachment: 347170

Committed r234910: <https://trac.webkit.org/changeset/234910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.