REGRESSION (r206410): Sandbox violations beneath WebProcessProxy::platformIsBeingDebugged

• Component: WebKit2
• Status: RESOLVED FIXED

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

Created attachment 292588 [details]
Only check the Web process if the UI process is being g debugged

[Darin Adler]

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());

[Alexey Proskuryakov]

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?

[Alexey Proskuryakov]

Darin's r+ got reverted accidentally, but I think that my questions are valid.

[Darin Adler]

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

[Alexey Proskuryakov]

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.

Created attachment 292720 [details]
Patch

Committed <https://trac.webkit.org/r207807>.