|Summary:||REGRESSION (r206410): Sandbox violations beneath WebProcessProxy::platformIsBeingDebugged|
|Severity:||Normal||CC:||ap, darin, sam|
|Version:||WebKit Nightly Build|
Description mitz 2016-10-23 23:58:49 PDT
<rdar://problem/28728735> After <https://trac.webkit.org/r206410> (for bug 162234), a sandboxed UI process may commit a sandbox violation when a Web Content process becomes unresponsive, as the former is not allowed to check whether the latter is being debugged.
Comment 1 mitz 2016-10-24 00:03:02 PDT
Created attachment 292588 [details] Only check the Web process if the UI process is being g debugged
Comment 2 Darin Adler 2016-10-24 09:58:01 PDT
Comment on attachment 292588 [details] Only check the Web process if the UI process is being g debugged View in context: https://bugs.webkit.org/attachment.cgi?id=292588&action=review > Source/WebKit2/UIProcess/Cocoa/WebProcessProxyCocoa.mm:140 > + if (!processIsBeingDebugged(getpid())) > + return false; > + > + return processIsBeingDebugged(processIdentifier()); I like using short-circuit boolean algebra for this kind of thing, and I also believe this needs a why comment since the name of the function and implementation don’t match in what might be a surprising way. Something like this, only more accurate and better worded and maybe even broken into multiple lines: // Checking our own process first and then the target process means we won’t try something we don’t have permissions for in the production, sandboxed case. return processIsBeingDebugged(getpid()) && processIsBeingDebugged(processIdentifier());
Comment 3 Alexey Proskuryakov 2016-10-24 10:00:12 PDT
Comment on attachment 292588 [details] Only check the Web process if the UI process is being g debugged View in context: https://bugs.webkit.org/attachment.cgi?id=292588&action=review > Source/WebKit2/ChangeLog:11 > + (WebKit::WebProcessProxy::platformIsBeingDebugged): If the UI process is not currently being Why is this right? I almost never debug both processes, so at least as far as I'm concerned, this completely breaks the intent of the original patch. Also, why debugging the UI process signifies that it's sandbox is open?
Comment 4 Alexey Proskuryakov 2016-10-24 10:01:43 PDT
Darin's r+ got reverted accidentally, but I think that my questions are valid.
Comment 5 Darin Adler 2016-10-24 10:15:24 PDT
I am not in any hurry to r+ this; I’m sure Dan thought it through and if he writes a comment like the one I requested it will likely help the rest of us understand his thinking.
Comment 6 mitz 2016-10-24 18:03:12 PDT
Comment on attachment 292588 [details] Only check the Web process if the UI process is being g debugged Thanks for your comments, Alexey. This mechanism isn’t going to be as useful as I wish it were, but I think it’s still worth keeping. I am going to write a patch that uses WebKit::processIsSandboxed() on the UI process, which is a much better way to predict if checking whether checking the state of another process will succeed.
Comment 7 Alexey Proskuryakov 2016-10-24 19:47:30 PDT
I guess another way to predict if this will work is to perform a dry run using sandbox_check. Of course, that check may break in the future if internal implementation of this sysctl changes.