Bug 218188 – [macOS] Avoid calling getpwuid_r before entering the sandbox in the WebContent process

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit Misc.

[Per Arne Vollan]

Calling confstr before entering the sandbox in the WebContent process on macOS leaves behind an open connection to opendirectoryd, which should be avoided.

[Radar WebKit Bug Importer]

<rdar://problem/70679577>

[Per Arne Vollan]

Created attachment 412318 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 412318 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Calling confstr before entering the sandbox in the WebContent process on macOS leaves behind an open connection

Doesn’t this patch create a sandbox escape from UI process? The UI process is also sandboxed, and using any path from it means trusting it.

[Brent Fulgham]

(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 412318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412318&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        Calling confstr before entering the sandbox in the WebContent process on macOS leaves behind an open connection
> 
> Doesn’t this patch create a sandbox escape from UI process? The UI process
> is also sandboxed, and using any path from it means trusting it.

I think trust of the UIProcess is implicit in our design. It vends the Mach connections and other items the WebContent process has to use. It is the process that mediates user input and gates access to privileged things like file locations, camera access, and so forth.

Or maybe I misunderstand your concern?

[Brent Fulgham]

Comment on attachment 412318 [details]
Patch

r=me. The test failures don't seem related to this, but please double-check locally.

[Alexey Proskuryakov]

Comment on attachment 412318 [details]
Patch

We don't fully trust UI process, and have gone through complicated design exercises to avoid such trust!

[Per Arne Vollan]

Created attachment 415009 [details]
Patch

[Per Arne Vollan]

I have uploaded a new patch, which I believe should address the issues with the previous patch.

Thanks for reviewing, all!

[Alexey Proskuryakov]

Comment on attachment 415009 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        Calling confstr before entering the sandbox in the WebContent process on macOS leaves behind an open connection to
> +        opendirectoryd, which should be avoided. Instead, call confstr in the UI process, and pass the results to the
> +        WebContent process as part of the extra initialization data. This patch will move the sandbox data vault directory
> +        to /System/Library/Caches.

Could you please elaborate on how this avoids the problem with the original patch? Looks like we are still getting a path from UI process.

What is the reason for moving the cache to /S/L/C?

[Alexey Proskuryakov]

Comment on attachment 415009 [details]
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.

[Per Arne Vollan]

Created attachment 420863 [details]
Patch

[Geoffrey Garen]

Comment on attachment 420863 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Calling getpwuid_r before entering the sandbox in the WebContent process on macOS leaves behind an open connection to
> +        opendirectoryd, which should be avoided. Instead, call getpwuid_r in the UI process, and pass the results to the

This is the second case I've looked at today where we left open a privileged connection in WebContent. Is there a way to write a test for these kinds of open connections? I'm not sure how we'll defend against regression without a test.

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #10)
> Comment on attachment 415009 [details]
> 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.
> 

Yes, these are very good points. This part has been removed from the latest version of the patch.

> > 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.
> 

This has also been removed from the latest patch.

> 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.
> 

In the latest version of the patch, only the home directory from system binaries will be trusted.

> > 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.

Ah, good point! This has been fixed in the new patch.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #12)
> Comment on attachment 420863 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420863&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        Calling getpwuid_r before entering the sandbox in the WebContent process on macOS leaves behind an open connection to
> > +        opendirectoryd, which should be avoided. Instead, call getpwuid_r in the UI process, and pass the results to the
> 
> This is the second case I've looked at today where we left open a privileged
> connection in WebContent. Is there a way to write a test for these kinds of
> open connections? I'm not sure how we'll defend against regression without a
> test.

Yes, that is a very good point. I think it should be possible to write regression tests for this, although it is probably not trivial, since the sandbox API cannot detect this, AFAIK. I would think there exists API for listing open mach ports, though. Additionally, I think we should look into entering the sandbox much earlier, which would also help defend against this.

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 420863 [details]
Patch

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

>>> Source/WebKit/ChangeLog:9
>>> +        opendirectoryd, which should be avoided. Instead, call getpwuid_r in the UI process, and pass the results to the
>> 
>> This is the second case I've looked at today where we left open a privileged connection in WebContent. Is there a way to write a test for these kinds of open connections? I'm not sure how we'll defend against regression without a test.
> 
> Yes, that is a very good point. I think it should be possible to write regression tests for this, although it is probably not trivial, since the sandbox API cannot detect this, AFAIK. I would think there exists API for listing open mach ports, though. Additionally, I think we should look into entering the sandbox much earlier, which would also help defend against this.
> 
> Thanks for reviewing!

Perhaps you could file a bug to create this new test feature?

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:687
> +    if (homeDirectory.isEmpty()) {

When might we ever need to his this code path? I guess third-party clients might not send the home directory?

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:110
> +    struct passwd* result = 0;

nullptr

[Per Arne Vollan]

Created attachment 421013 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #15)
> Comment on attachment 420863 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420863&action=review
> 
> >>> Source/WebKit/ChangeLog:9
> >>> +        opendirectoryd, which should be avoided. Instead, call getpwuid_r in the UI process, and pass the results to the
> >> 
> >> This is the second case I've looked at today where we left open a privileged connection in WebContent. Is there a way to write a test for these kinds of open connections? I'm not sure how we'll defend against regression without a test.
> > 
> > Yes, that is a very good point. I think it should be possible to write regression tests for this, although it is probably not trivial, since the sandbox API cannot detect this, AFAIK. I would think there exists API for listing open mach ports, though. Additionally, I think we should look into entering the sandbox much earlier, which would also help defend against this.
> > 
> > Thanks for reviewing!
> 
> Perhaps you could file a bug to create this new test feature?
> 

I filed https://bugs.webkit.org/show_bug.cgi?id=222193.

> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:687
> > +    if (homeDirectory.isEmpty()) {
> 
> When might we ever need to his this code path? I guess third-party clients
> might not send the home directory?
> 

All clients will send the home directory, but we will only trust the home directory from system binaries, so this path will be taken when that is not the case.

> > Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:110
> > +    struct passwd* result = 0;
> 
> nullptr

Fixed!

Thanks for reviewing!

[Alexey Proskuryakov]

> All clients will send the home directory, but we will only trust the home directory from system binaries, so this path will be taken when that is not the case.

Why would we trust home directory from system binaries? They can be hacked or confused like anything else, and can have very limited sandbox that's worth escaping via WebKit.

[Alexey Proskuryakov]

I don't think that it's a security distinction we've made before, but even if we did, it seems invalid to me.

[Alexey Proskuryakov]

Also, to be clear, I don't want to mess with r+ flag that's set, but I think that we need to think this through before landing.

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #20)
> Also, to be clear, I don't want to mess with r+ flag that's set, but I think
> that we need to think this through before landing.

Yes, let's discuss implications more before landing.

Thanks for reviewing!

[Brent Fulgham]

We no longer need this change after Bug 227068.

*** This bug has been marked as a duplicate of bug 227068 ***