Bug 203419 - [iOS] Fix sandbox violations seen while running layout tests
Summary: [iOS] Fix sandbox violations seen while running layout tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-25 09:04 PDT by Per Arne Vollan
Modified: 2019-10-28 16:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.97 KB, patch)
2019-10-25 09:09 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-10-25 09:04:48 PDT
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' internally.
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.
Comment 1 Per Arne Vollan 2019-10-25 09:09:06 PDT
Created attachment 381930 [details]
Patch
Comment 2 Brent Fulgham 2019-10-25 09:41:36 PDT
Comment on attachment 381930 [details]
Patch

r=me
Comment 3 Per Arne Vollan 2019-10-25 09:47:51 PDT
(In reply to Brent Fulgham from comment #2)
> Comment on attachment 381930 [details]
> Patch
> 
> r=me

Thanks for reviewing!
Comment 4 WebKit Commit Bot 2019-10-25 12:03:08 PDT
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 5 WebKit Commit Bot 2019-10-25 12:03:09 PDT
The commit-queue encountered the following flaky tests while processing attachment 381930 [details]:

The commit-queue is continuing to process your patch.
Comment 6 WebKit Commit Bot 2019-10-25 16:59:54 PDT
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 7 WebKit Commit Bot 2019-10-25 17:00:38 PDT
Comment on attachment 381930 [details]
Patch

Clearing flags on attachment: 381930

Committed r251612: <https://trac.webkit.org/changeset/251612>
Comment 8 WebKit Commit Bot 2019-10-25 17:00:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-10-25 17:01:16 PDT
<rdar://problem/56637619>
Comment 10 Alexey Proskuryakov 2019-10-27 18:43:13 PDT
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?
Comment 11 Per Arne Vollan 2019-10-28 16:58:52 PDT
(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!