Bug 220358

<rdar://problem/57616019>

Created attachment 417079 [details] Patch

Comment on attachment 417079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417079&action=review > Source/WebKit/ChangeLog:11 > + Reset user directory suffix before getting sandbox data vault directory. We do not want to include the user directory suffix, > + since the compiled sandbox should be shared by all WebKit processes of the same type. Also, creating the data vault directory > + can fail under some circumstances if the user directory suffix is not empty. I do not understand what "all WebKit processes of the same type" means. Surely WebKit processes for different applications have different sandbox profiles? Also, I do not see how this can help <rdar://problem/57616019>, which is about trying to use an incorrect cache directory path, with a '/' instead of a '+'. How do we end up trying to use the slash?

(In reply to Alexey Proskuryakov from comment #3) > Comment on attachment 417079 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417079&action=review > > > Source/WebKit/ChangeLog:11 > > + Reset user directory suffix before getting sandbox data vault directory. We do not want to include the user directory suffix, > > + since the compiled sandbox should be shared by all WebKit processes of the same type. Also, creating the data vault directory > > + can fail under some circumstances if the user directory suffix is not empty. > > I do not understand what "all WebKit processes of the same type" means. > Surely WebKit processes for different applications have different sandbox > profiles? > The different WebKit process types I am thinking of is GPU, WebContent, and Networking. I believe each WebKit process type can share the same cached sandbox profile, which this patch ensures by having each type store their profile at the same location. This should be a performance optimization. > Also, I do not see how this can help <rdar://problem/57616019>, which is > about trying to use an incorrect cache directory path, with a '/' instead of > a '+'. How do we end up trying to use the slash? I believe we end up trying to use the slash, because the slash is present in the user directory suffix. This patch fixes this and the crash by resetting the suffix, making sure confstr returns the expected result. Thanks for reviewing!

> I believe each WebKit process type can share the same cached sandbox profile, which this patch ensures by having each type store their profile at the same location. This cannot work today because they all have different variables applied when compiling. Are you talking about a future goal? > I believe we end up trying to use the slash, because the slash is present in the user directory suffix. This patch fixes this and the crash by resetting the suffix, making sure confstr returns the expected result. Where does the unusual suffix come from? The crash happens in Networking process, so I don't understand what could possibly set it. Also, the suffix for networking process always has a '+' in it, which I don't see in the crash log. This is very mysterious.

(In reply to Alexey Proskuryakov from comment #5) > > I believe each WebKit process type can share the same cached sandbox profile, which this patch ensures by having each type store their profile at the same location. > > This cannot work today because they all have different variables applied > when compiling. Are you talking about a future goal? > That is a good point. Can the sandbox parameters be different for different apps hosting WebKit, though? Is there any way the embedding application can have any influence on the sandbox parameters? If not, I think we should be able to share the cached sandboxes between various embedding apps. > > I believe we end up trying to use the slash, because the slash is present in the user directory suffix. This patch fixes this and the crash by resetting the suffix, making sure confstr returns the expected result. > > Where does the unusual suffix come from? The crash happens in Networking > process, so I don't understand what could possibly set it. Also, the suffix > for networking process always has a '+' in it, which I don't see in the > crash log. This is very mysterious. Yes, this is very strange. I am currently looking into why we see the unusual user directory suffix. Thanks for reviewing!

> Can the sandbox parameters be different for different > apps hosting WebKit, though? They are expected to be different, e.g. each one has its own DARWIN_USER_CACHE_DIR; ENABLE_SANDBOX_MESSAGE_FILTER can be on or off. > Is there any way the embedding application can > have any influence on the sandbox parameters? The bundle ID of the embedding application definitely does. It is OK for it to have other kinds of influence as long as we understand that the input can be malicious, and handle it defensively before enabling the sandbox.

(In reply to Alexey Proskuryakov from comment #7) > > Can the sandbox parameters be different for different > > apps hosting WebKit, though? > > They are expected to be different, e.g. each one has its own > DARWIN_USER_CACHE_DIR; ENABLE_SANDBOX_MESSAGE_FILTER can be on or off. > > > Is there any way the embedding application can > > have any influence on the sandbox parameters? > > The bundle ID of the embedding application definitely does. It is OK for it > to have other kinds of influence as long as we understand that the input can > be malicious, and handle it defensively before enabling the sandbox. On second thought, I think the current patch is correct, since it does not cause WebKit processes to share profiles. It only makes them end up in the same folder for various apps, e.g. in the subfolder com.apple.WebKit.WebContent.Sandbox for the WebContent process. This is already usually the case, but this patch makes this consistent, also when the WebKit process has a user directory suffix set. What do you think?

I have verified locally that confstr correctly handles multiple updates of the user directory suffix with the function _set_user_dir_suffix().

Comment on attachment 417079 [details] Patch R=me, based on Per Arne's latest testing.

Comment on attachment 417079 [details] Patch This wasn't even up for review...

Committed r271331: <https://trac.webkit.org/changeset/271331> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417079 [details].

Re-opened since this is blocked by bug 220487

Comment on attachment 417079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417079&action=review > Source/WebCore/PAL/pal/spi/cocoa/CoreServicesSPI.h:28 > +extern "C" { FYI, there are BEGIN_EXTERN_C and END_EXTERN_C macros in one of the WTF compiler headers, so I think we should be using those going forward.

(In reply to David Kilzer (:ddkilzer) from comment #14) > Comment on attachment 417079 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417079&action=review > > > Source/WebCore/PAL/pal/spi/cocoa/CoreServicesSPI.h:28 > > +extern "C" { > > FYI, there are BEGIN_EXTERN_C and END_EXTERN_C macros in one of the WTF > compiler headers, so I think we should be using those going forward. Will fix! Thanks for reviewing!

Created attachment 417446 [details] Patch

Created attachment 417447 [details] Patch

Comment on attachment 417447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417447&action=review > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:658 > + _set_user_dir_suffix(FileSystem::fileSystemRepresentation(sandboxParameters.userDirectorySuffix()).data()); Does this put cache and temporary directories under the original suffix? Wouldn't that just crash for the same reason? Also, I'm beginning to think that WebKit auxiliary processes should completely ignore the UI process suffix, not just for data vault. We never put cache and temporary files under UI process control, or otherwise implicitly inherit anything from UI process, and I don't see why we'd do that for processes that set the suffix. Perhaps we should talk to owners of this one to see what they are trying to achieve.

(In reply to Alexey Proskuryakov from comment #18) > Comment on attachment 417447 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417447&action=review > > > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:658 > > + _set_user_dir_suffix(FileSystem::fileSystemRepresentation(sandboxParameters.userDirectorySuffix()).data()); > > Does this put cache and temporary directories under the original suffix? > Wouldn't that just crash for the same reason? > This puts cache and temporary directory directly under the top level cache or temporary directory. For example, for the Networking process running under MiniBrowser, this will set the suffix to 'com.apple.WebKit.Networking+org.webkit.MiniBrowser', and the cache folder will be at '/var/folders/dd/wjn_0cz5593389p25ybpbml00000gn/C/com.apple.WebKit.Networking+org.webkit.MiniBrowser'. This will not crash, since 'com.apple.WebKit.Networking+org.webkit.MiniBrowser' is a valid user directory suffix, which the system handles successfully. This change does actually not change behavior, it just replaces a setenv with _set_user_dir_suffix. > Also, I'm beginning to think that WebKit auxiliary processes should > completely ignore the UI process suffix, not just for data vault. We never > put cache and temporary files under UI process control, or otherwise > implicitly inherit anything from UI process, and I don't see why we'd do > that for processes that set the suffix. Perhaps we should talk to owners of > this one to see what they are trying to achieve. I think this patch makes us completely ignore the user directory suffix we inherit from the UI process, unless confstr is called before we reset the user directory suffix. Currently, there is no confstr call before the reset. This path is always taken on macOS, and makes sure we have an empty user directory suffix until we choose to set it ourselves with the above call to '_set_user_dir_suffix(FileSystem::fileSystemRepresentation(sandboxParameters.userDirectorySuffix()).data())', which will be the user directory suffix for the rest of the process lifetime. We could consider resetting the user directory suffix even earlier to guard against confstr calls being added before we reset the user directory suffix. What do you think?

Comment on attachment 417447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417447&action=review > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:705 > + _set_user_dir_suffix(nullptr); I'm still not sure from looking at the code if we can set this twice. This function itself support that, but dirhelper does not, as _dirhelper_init is only executed once, and sets state based forever.

(In reply to Alexey Proskuryakov from comment #20) > Comment on attachment 417447 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417447&action=review > > > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:705 > > + _set_user_dir_suffix(nullptr); > > I'm still not sure from looking at the code if we can set this twice. This > function itself support that, but dirhelper does not, as _dirhelper_init is > only executed once, and sets state based forever. In the debugger, I have verified that results from confstr are correct after multiple updates of the suffix with _set_user_dir_suffix. Thanks for reviewing!

Committed r271417: <https://trac.webkit.org/changeset/271417> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417447 [details].