RESOLVED DUPLICATE of bug 227068 218188
[macOS] Avoid calling getpwuid_r before entering the sandbox in the WebContent process
https://bugs.webkit.org/show_bug.cgi?id=218188
Summary [macOS] Avoid calling getpwuid_r before entering the sandbox in the WebConten...
Per Arne Vollan
Reported 2020-10-26 08:11:06 PDT
Calling confstr before entering the sandbox in the WebContent process on macOS leaves behind an open connection to opendirectoryd, which should be avoided.
Attachments
Patch (7.68 KB, patch)
2020-10-26 08:18 PDT, Per Arne Vollan
no flags
Patch (9.32 KB, patch)
2020-11-30 02:10 PST, Per Arne Vollan
no flags
Patch (5.83 KB, patch)
2021-02-18 12:54 PST, Per Arne Vollan
bfulgham: review+
Patch (5.82 KB, patch)
2021-02-19 12:40 PST, Per Arne Vollan
no flags
Radar WebKit Bug Importer
Comment 1 2020-10-26 08:11:33 PDT
Per Arne Vollan
Comment 2 2020-10-26 08:18:54 PDT
Alexey Proskuryakov
Comment 3 2020-10-26 18:28:17 PDT
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
Comment 4 2020-10-27 11:10:00 PDT
(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 5 2020-10-27 13:59:15 PDT
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 6 2020-10-27 17:02:44 PDT
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
Comment 7 2020-11-30 02:10:18 PST
Per Arne Vollan
Comment 8 2020-11-30 02:11:57 PST
I have uploaded a new patch, which I believe should address the issues with the previous patch. Thanks for reviewing, all!
Alexey Proskuryakov
Comment 9 2020-11-30 14:49:46 PST
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 10 2020-12-01 18:04:08 PST
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
Comment 11 2021-02-18 12:54:41 PST
Geoffrey Garen
Comment 12 2021-02-18 12:58:52 PST
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
Comment 13 2021-02-18 13:00:08 PST
(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
Comment 14 2021-02-19 08:57:42 PST
(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 15 2021-02-19 11:17:02 PST
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
Comment 16 2021-02-19 12:40:21 PST
Per Arne Vollan
Comment 17 2021-02-19 13:09:11 PST
(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
Comment 18 2021-02-19 15:19:32 PST
> 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
Comment 19 2021-02-19 15:20:06 PST
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
Comment 20 2021-02-19 15:43:16 PST
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
Comment 21 2021-02-19 15:45:25 PST
(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
Comment 22 2022-02-12 18:49:27 PST
We no longer need this change after Bug 227068. *** This bug has been marked as a duplicate of bug 227068 ***
Note You need to log in before you can comment on or make changes to this bug.