163879 2016-10-23 23:58:49 -0700 REGRESSION (r206410): Sandbox violations beneath WebProcessProxy::platformIsBeingDebugged 2016-10-24 22:25:44 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=162234 InRadar P2 Normal --- 1 mitz mitz ap darin sam oldest_to_newest 1243621 0 mitz 2016-10-23 23:58:49 -0700 <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. 1243623 1 292588 mitz 2016-10-24 00:03:02 -0700 Created attachment 292588 Only check the Web process if the UI process is being g debugged 1243736 2 292588 darin 2016-10-24 09:58:01 -0700 Comment on attachment 292588 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()); 1243737 3 292588 ap 2016-10-24 10:00:12 -0700 Comment on attachment 292588 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? 1243739 4 ap 2016-10-24 10:01:43 -0700 Darin's r+ got reverted accidentally, but I think that my questions are valid. 1243748 5 darin 2016-10-24 10:15:24 -0700 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. 1244007 6 292588 mitz 2016-10-24 18:03:12 -0700 Comment on attachment 292588 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. 1244052 7 ap 2016-10-24 19:47:30 -0700 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. 1244104 8 292720 mitz 2016-10-24 22:17:22 -0700 Created attachment 292720 Patch 1244109 9 mitz 2016-10-24 22:25:44 -0700 Committed <https://trac.webkit.org/r207807>. 292588 2016-10-24 00:03:02 -0700 2016-10-24 18:03:12 -0700 Only check the Web process if the UI process is being g debugged bug-163879-20161023235919.patch text/plain 2112 mitz SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwNzczNykKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDE2LTEwLTIzICBEYW4gQmVy bnN0ZWluICA8bWl0ekBhcHBsZS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjIwNjQxMCk6 IFNhbmRib3ggdmlvbGF0aW9ucyBiZW5lYXRoIFdlYlByb2Nlc3NQcm94eTo6cGxhdGZvcm1Jc0Jl aW5nRGVidWdnZWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE2Mzg3OQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMjg3Mjg3MzU+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQ29jb2Ev V2ViUHJvY2Vzc1Byb3h5Q29jb2EubW06CisgICAgICAgIChXZWJLaXQ6OnByb2Nlc3NJc0JlaW5n RGVidWdnZWQpOiBGYWN0b3JlZCBvdXQgb2YgdGhlIGJlbG93IGZ1bmN0aW9uLgorICAgICAgICAo V2ViS2l0OjpXZWJQcm9jZXNzUHJveHk6OnBsYXRmb3JtSXNCZWluZ0RlYnVnZ2VkKTogSWYgdGhl IFVJIHByb2Nlc3MgaXMgbm90IGN1cnJlbnRseSBiZWluZworICAgICAgICAgIGRlYnVnZ2VkLCBh dm9pZCBjaGVja2luZyBpZiB0aGUgV2ViIHByb2Nlc3MgaXMgYmVpbmcgZGVidWdnZWQuCisKIDIw MTYtMTAtMjMgIE1pY2hhZWwgQ2F0YW56YXJvICA8bWNhdGFuemFyb0BpZ2FsaWEuY29tPgogCiAg ICAgICAgIFtHVEtdIFJlbW92ZSBETyBOT1QgTU9ESUZZIGhlYWRlcnMgZnJvbSBmaWxlcyB0aGF0 IGFyZSBubyBsb25nZXIgYXV0b2dlbmVyYXRlZApJbmRleDogU291cmNlL1dlYktpdDIvVUlQcm9j ZXNzL0NvY29hL1dlYlByb2Nlc3NQcm94eUNvY29hLm1tCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X ZWJLaXQyL1VJUHJvY2Vzcy9Db2NvYS9XZWJQcm9jZXNzUHJveHlDb2NvYS5tbQkocmV2aXNpb24g MjA3NzIyKQorKysgU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0NvY29hL1dlYlByb2Nlc3NQcm94 eUNvY29hLm1tCSh3b3JraW5nIGNvcHkpCkBAIC0xMjEsMTAgKzEyMSwxMCBAQCBSZWZQdHI8T2Jq Q09iamVjdEdyYXBoPiBXZWJQcm9jZXNzUHJveHk6CiAgICAgcmV0dXJuIE9iakNPYmplY3RHcmFw aDo6Y3JlYXRlKE9iakNPYmplY3RHcmFwaDo6dHJhbnNmb3JtKG9iamVjdEdyYXBoLnJvb3RPYmpl Y3QoKSwgVHJhbnNmb3JtZXIoKSkuZ2V0KCkpOwogfQogCi1ib29sIFdlYlByb2Nlc3NQcm94eTo6 cGxhdGZvcm1Jc0JlaW5nRGVidWdnZWQoKSBjb25zdAorc3RhdGljIGJvb2wgcHJvY2Vzc0lzQmVp bmdEZWJ1Z2dlZChwaWRfdCBwaWQpCiB7CiAgICAgc3RydWN0IGtpbmZvX3Byb2MgaW5mbzsKLSAg ICBpbnQgbWliW10gPSB7IENUTF9LRVJOLCBLRVJOX1BST0MsIEtFUk5fUFJPQ19QSUQsIHByb2Nl c3NJZGVudGlmaWVyKCkgfTsKKyAgICBpbnQgbWliW10gPSB7IENUTF9LRVJOLCBLRVJOX1BST0Ms IEtFUk5fUFJPQ19QSUQsIHBpZCB9OwogICAgIHNpemVfdCBzaXplID0gc2l6ZW9mKGluZm8pOwog ICAgIGlmIChzeXNjdGwobWliLCBXVEZfQVJSQVlfTEVOR1RIKG1pYiksICZpbmZvLCAmc2l6ZSwg bnVsbHB0ciwgMCkgPT0gLTEpCiAgICAgICAgIHJldHVybiBmYWxzZTsKQEAgLTEzMiw0ICsxMzIs MTIgQEAgYm9vbCBXZWJQcm9jZXNzUHJveHk6OnBsYXRmb3JtSXNCZWluZ0RlYgogICAgIHJldHVy biBpbmZvLmtwX3Byb2MucF9mbGFnICYgUF9UUkFDRUQ7CiB9CiAKK2Jvb2wgV2ViUHJvY2Vzc1By b3h5OjpwbGF0Zm9ybUlzQmVpbmdEZWJ1Z2dlZCgpIGNvbnN0Cit7CisgICAgaWYgKCFwcm9jZXNz SXNCZWluZ0RlYnVnZ2VkKGdldHBpZCgpKSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOworCisgICAg cmV0dXJuIHByb2Nlc3NJc0JlaW5nRGVidWdnZWQocHJvY2Vzc0lkZW50aWZpZXIoKSk7Cit9CisK IH0K 292720 2016-10-24 22:17:22 -0700 2016-10-24 22:20:25 -0700 Patch bug-163879-20161024221337.patch text/plain 1808 mitz SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwNzgwNSkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDE2LTEwLTI0ICBEYW4gQmVy bnN0ZWluICA8bWl0ekBhcHBsZS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjIwNjQxMCk6 IFNhbmRib3ggdmlvbGF0aW9ucyBiZW5lYXRoIFdlYlByb2Nlc3NQcm94eTo6cGxhdGZvcm1Jc0Jl aW5nRGVidWdnZWQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE2Mzg3OQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMjg3Mjg3MzU+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQ29jb2Ev V2ViUHJvY2Vzc1Byb3h5Q29jb2EubW06CisgICAgICAgIChXZWJLaXQ6OldlYlByb2Nlc3NQcm94 eTo6cGxhdGZvcm1Jc0JlaW5nRGVidWdnZWQpOiBDaGVjayBpZiB0aGUgY3VycmVudCBwcm9jZXNz LCB3aGljaCBpcworICAgICAgICAgIHRoZSBVSSBwcm9jZXNzLCBpcyBzYW5kYm94ZWQgYmVmb3Jl IHRyeWluZyB0byBmaW5kIG91dCBpZiB0aGUgV2ViIHByb2Nlc3MgaXMgYmVpbmcKKyAgICAgICAg ICBkZWJ1Z2dlZC4KKwogMjAxNi0xMC0yMSAgQWxleCBDaHJpc3RlbnNlbiAgPGFjaHJpc3RlbnNl bkB3ZWJraXQub3JnPgogCiAgICAgICAgIFVSTDo6cG9ydCBzaG91bGQgcmV0dXJuIE9wdGlvbmFs PHVpbnQxNl90PgpJbmRleDogU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0NvY29hL1dlYlByb2Nl c3NQcm94eUNvY29hLm1tCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9D b2NvYS9XZWJQcm9jZXNzUHJveHlDb2NvYS5tbQkocmV2aXNpb24gMjA3ODAzKQorKysgU291cmNl L1dlYktpdDIvVUlQcm9jZXNzL0NvY29hL1dlYlByb2Nlc3NQcm94eUNvY29hLm1tCSh3b3JraW5n IGNvcHkpCkBAIC0yNyw2ICsyNyw3IEBACiAjaW1wb3J0ICJXZWJQcm9jZXNzUHJveHkuaCIKIAog I2ltcG9ydCAiT2JqQ09iamVjdEdyYXBoLmgiCisjaW1wb3J0ICJTYW5kYm94VXRpbGl0aWVzLmgi CiAjaW1wb3J0ICJXS0Jyb3dzaW5nQ29udGV4dENvbnRyb2xsZXJJbnRlcm5hbC5oIgogI2ltcG9y dCAiV0tCcm93c2luZ0NvbnRleHRIYW5kbGVJbnRlcm5hbC5oIgogI2ltcG9ydCAiV0tUeXBlUmVm V3JhcHBlci5oIgpAQCAtMTIzLDYgKzEyNCwxMCBAQCBSZWZQdHI8T2JqQ09iamVjdEdyYXBoPiBX ZWJQcm9jZXNzUHJveHk6CiAKIGJvb2wgV2ViUHJvY2Vzc1Byb3h5OjpwbGF0Zm9ybUlzQmVpbmdE ZWJ1Z2dlZCgpIGNvbnN0CiB7CisgICAgLy8gSWYgdGhlIFVJIHByb2Nlc3MgaXMgc2FuZGJveGVk LCBpdCBjYW5ub3QgZmluZCBvdXQgd2hldGhlciBvdGhlciBwcm9jZXNzZXMgYXJlIGJlaW5nIGRl YnVnZ2VkLgorICAgIGlmIChwcm9jZXNzSXNTYW5kYm94ZWQoZ2V0cGlkKCkpKQorICAgICAgICBy ZXR1cm4gZmFsc2U7CisKICAgICBzdHJ1Y3Qga2luZm9fcHJvYyBpbmZvOwogICAgIGludCBtaWJb XSA9IHsgQ1RMX0tFUk4sIEtFUk5fUFJPQywgS0VSTl9QUk9DX1BJRCwgcHJvY2Vzc0lkZW50aWZp ZXIoKSB9OwogICAgIHNpemVfdCBzaXplID0gc2l6ZW9mKGluZm8pOwo=