[webkit-reviews] review denied: [Bug 218188] [macOS] Avoid calling confstr before entering the sandbox in the WebContent process : [Attachment 415009] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Per Arne Vollan
<pvollan at apple.com>'s request for review:
Bug 218188: [macOS] Avoid calling confstr before entering the sandbox in the
WebContent process
https://bugs.webkit.org/show_bug.cgi?id=218188

Attachment 415009: Patch

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




--- Comment #10 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 415009
  --> https://bugs.webkit.org/attachment.cgi?id=415009
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:266
> +    auto url = [[NSFileManager defaultManager]
URLForDirectory:NSCachesDirectory inDomain:NSLocalDomainMask
appropriateForURL:nullptr create:NO error:nullptr];

This is not /S/L/C, but ~/L/C, correct? Why is it ok to ignore the error? Is
there any way in which the result of this function can be affected by the UI
process?

I think that switching from the Darwin user cache directory to ~/L/C is a
pretty big deal. I do not know the details well, but there is a number of
behaviors that could be and probably are different between these - how
CacheDelete works with them, what happens for backups, reboots or software
updates.

IIRC the largest difference is that the Darwin user directory is always on the
boot (data) volume, whereas ~ can be anywhere, even on NFS, and those locations
are vulnerable to various attacks. So the proposed change reduces security for
people with network home directories.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:658
> +    setenv("TMPDIR", temporaryDirectory.utf8().data(), 1);

One big reason why we needed to change what confstr returns was that underlying
frameworks could use it to decide where to put their temporary and cache files.
Setting TMPDIR is not enough to tell underlying frameworks what to do.

I didn't analyze EWS failures carefully, but they seem to be about this at the
first glance.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:663
> +    sandboxParameters.addPathParameter("DARWIN_USER_TEMP_DIR",
temporaryDirectory);
> +    String userCacheDirectory =
parameters.extraInitializationData.get("user-cache-dir"_s);
> +    sandboxParameters.addPathParameter("DARWIN_USER_CACHE_DIR",
userCacheDirectory);

This looks like we are still using paths from UI process to configure the
sandbox? What if the UI process sends "/" in the user-cache-dir parameter? Then
the Web process will have read-write access to the whole file system (modulo
Unix restrictions and maybe TCC), and a malicious UI process will be able to
drive it to do anything.

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:130
> +	   launchOptions.extraInitializationData.add("user-dir"_s, pwd.pw_dir);

This is a misnomer. User directory is what's returned by confstr, e.g.
/var/folders/64/n4pdvp4j2x5cnggcgjf2wzgc0000gn/0. Home directory is what comes
from directory services, /Users/username when unmodified.

You can see the former by running "getconf DARWIN_USER_DIR", and the latter
using dscl in command line, or from a context menu in Users preference pane.