[webkit-reviews] review denied: [Bug 184991] We should cache the compiled sandbox profile in a data vault : [Attachment 342947] cache sandbox

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Ben Richards
<benton_richards at apple.com>'s request for review:
Bug 184991: We should cache the compiled sandbox profile in a data vault
https://bugs.webkit.org/show_bug.cgi?id=184991

Attachment 342947: cache sandbox

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




--- Comment #44 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 342947
  --> https://bugs.webkit.org/attachment.cgi?id=342947
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