Bug 203419

Created attachment 381930 [details] Patch

Comment on attachment 381930 [details] Patch r=me

(In reply to Brent Fulgham from comment #2) > Comment on attachment 381930 [details] > Patch > > r=me Thanks for reviewing!

The commit-queue encountered the following flaky tests while processing attachment 381930 [details]: imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/integrity.html bug 203394 (author: ysuzuki@apple.com) The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 381930 [details]: The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 381930 [details]: imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/integrity.html bug 203394 (author: ysuzuki@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 381930 [details] Patch Clearing flags on attachment: 381930 Committed r251612: <https://trac.webkit.org/changeset/251612>

All reviewed patches have been landed. Closing bug.

<rdar://problem/56637619>

Comment on attachment 381930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381930&action=review > Source/WebKit/ChangeLog:11 > + Deny mach lookup to 'com.apple.logd' and 'com.apple.logd.events' and suppress logs, since these are > + believed to be unneeded in the WebContent process. Allow sysctl write to 'vm.footprint_suspend'. > + Deny mach lookup to 'com.apple.system.notification_center' and suppress logs, since allowing this > + is not believed to be needed in the WebContent process. Where can stack traces for these violations be seen? It seems quite counter-intuitive that we do not need these connections. Can you document these decisions in the associated radar, if they cannot be documented here? Right now, there is nearly zero paper trail for why these dangerous changes were made. > Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:482 > +(with-filter (system-attribute apple-internal) > + (allow sysctl-read sysctl-write > + (sysctl-name "vm.footprint_suspend"))) "system-attribute apple-internal" is not what it seems. This change allows vm.footprint_suspend on some AppleInternal installs, but not on others, and it's unlikely that this is what you were after. You can see <rdar://problem/55853605> for some discussion of the differences. But also, footprint is a publicly shipping tool. What specifically about vm.footprint_suspend makes it AppleInternal only?

(In reply to Alexey Proskuryakov from comment #10) > Comment on attachment 381930 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=381930&action=review > > > Source/WebKit/ChangeLog:11 > > + Deny mach lookup to 'com.apple.logd' and 'com.apple.logd.events' and suppress logs, since these are > > + believed to be unneeded in the WebContent process. Allow sysctl write to 'vm.footprint_suspend'. > > + Deny mach lookup to 'com.apple.system.notification_center' and suppress logs, since allowing this > > + is not believed to be needed in the WebContent process. > > Where can stack traces for these violations be seen? It seems quite > counter-intuitive that we do not need these connections. > > Can you document these decisions in the associated radar, if they cannot be > documented here? Right now, there is nearly zero paper trail for why these > dangerous changes were made. > You are absolutely right. I have reverted most of these changes in https://bugs.webkit.org/show_bug.cgi?id=203505, except for access to 'com.apple.logd.events', which is not being looked up during layout tests. > > Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:482 > > +(with-filter (system-attribute apple-internal) > > + (allow sysctl-read sysctl-write > > + (sysctl-name "vm.footprint_suspend"))) > > "system-attribute apple-internal" is not what it seems. This change allows > vm.footprint_suspend on some AppleInternal installs, but not on others, and > it's unlikely that this is what you were after. You can see > <rdar://problem/55853605> for some discussion of the differences. > > But also, footprint is a publicly shipping tool. What specifically about > vm.footprint_suspend makes it AppleInternal only? See comment in <rdar://problem/56637619>. Thanks for reviewing!