Bug 209933

Summary: [iOS] Deny mach lookup access to the runningboard service in the WebContent process
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, eric.carlson, ggaren, jer.noble, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 210066    
Bug Blocks: 226357    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
cdumez: review+
Patch none

Description Per Arne Vollan 2020-04-02 15:20:12 PDT 
On iOS, after <https://trac.webkit.org/changeset/258180/webkit>, mach lookup access to "com.apple.runningboard" can be denied in the WebContent process.
Comment 1 Per Arne Vollan 2020-04-02 15:20:32 PDT 
rdar://problem/56995639
Comment 2 Per Arne Vollan 2020-04-02 15:29:38 PDT 
Created attachment 395311 [details]
Patch
Comment 3 Brent Fulgham 2020-04-03 10:30:51 PDT 
Comment on attachment 395311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395311&action=review

r=me

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:561
> +(deny mach-lookup (with telemetry-backtrace)

Hooray!
Comment 4 Per Arne Vollan 2020-04-03 10:31:28 PDT 
Comment on attachment 395311 [details]
Patch

Thanks for reviewing!
Comment 5 EWS 2020-04-03 10:37:51 PDT 
Committed r259469: <https://trac.webkit.org/changeset/259469>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395311 [details].
Comment 6 Chris Dumez 2020-04-06 10:19:18 PDT 
Comment on attachment 395311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395311&action=review

> Source/WebKit/ChangeLog:9
> +        On iOS, after <https://trac.webkit.org/changeset/258180/webkit>, mach lookup access to "com.apple.runningboard"

How can this be true? We still take a RunningBoard DependencyAssertion in the WebProcess. I suspect this broke media (again).
Comment 7 Chris Dumez 2020-04-06 10:19:37 PDT 
(In reply to Chris Dumez from comment #6)
> Comment on attachment 395311 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395311&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        On iOS, after <https://trac.webkit.org/changeset/258180/webkit>, mach lookup access to "com.apple.runningboard"
> 
> How can this be true? We still take a RunningBoard DependencyAssertion in
> the WebProcess. I suspect this broke media (again).

+ Jer/ Eric
Comment 8 Per Arne Vollan 2020-04-06 11:12:24 PDT 
(In reply to Chris Dumez from comment #6)
> Comment on attachment 395311 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395311&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        On iOS, after <https://trac.webkit.org/changeset/258180/webkit>, mach lookup access to "com.apple.runningboard"
> 
> How can this be true? We still take a RunningBoard DependencyAssertion in
> the WebProcess. I suspect this broke media (again).

I don't think calling 'm_uiProcessDependencyProcessAssertion = makeUnique<DependencyProcessAssertion>(remoteProcessID, "WebContent process dependency on UIProcess"_s)' in the WebContent process will need access to the runningboard service.
Comment 9 Chris Dumez 2020-04-06 11:23:15 PDT 
(In reply to Per Arne Vollan from comment #8)
> (In reply to Chris Dumez from comment #6)
> > Comment on attachment 395311 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=395311&action=review
> > 
> > > Source/WebKit/ChangeLog:9
> > > +        On iOS, after <https://trac.webkit.org/changeset/258180/webkit>, mach lookup access to "com.apple.runningboard"
> > 
> > How can this be true? We still take a RunningBoard DependencyAssertion in
> > the WebProcess. I suspect this broke media (again).
> 
> I don't think calling 'm_uiProcessDependencyProcessAssertion =
> makeUnique<DependencyProcessAssertion>(remoteProcessID, "WebContent process
> dependency on UIProcess"_s)' in the WebContent process will need access to
> the runningboard service.

That would be good news. As long as you have verified that then I guess we're good. Breakage would not be obvious but you would definitely see sandbox violations if things were not working I think.
Comment 10 WebKit Commit Bot 2020-04-06 12:58:16 PDT 
Re-opened since this is blocked by bug 210066
Comment 11 Per Arne Vollan 2020-04-07 14:26:06 PDT 
Created attachment 395739 [details]
Patch
Comment 12 Chris Dumez 2020-04-07 15:28:50 PDT 
This likely needs updating after http://trac.webkit.org/changeset/259674/webkit.
Comment 13 Per Arne Vollan 2020-04-07 15:41:26 PDT 
Created attachment 395752 [details]
Patch
Comment 14 Per Arne Vollan 2020-04-07 15:42:25 PDT 
(In reply to Chris Dumez from comment #12)
> This likely needs updating after
> http://trac.webkit.org/changeset/259674/webkit.

Thanks!
Comment 15 Chris Dumez 2020-04-07 15:44:33 PDT 
Comment on attachment 395752 [details]
Patch

Ok, assuming you have verified that the assertion reliably gets taken at RunningBoard level and does not get invalidated.
Comment 16 Per Arne Vollan 2020-04-07 15:47:08 PDT 
(In reply to Chris Dumez from comment #15)
> Comment on attachment 395752 [details]
> Patch
> 
> Ok, assuming you have verified that the assertion reliably gets taken at
> RunningBoard level and does not get invalidated.

Yes, I have confirmed that the creation of the dependency assertion succeeds, and manually tested the audio playback scenario.

Thanks for reviewing!
Comment 17 Per Arne Vollan 2020-04-07 16:24:46 PDT 
Created attachment 395758 [details]
Patch
Comment 18 EWS 2020-04-07 19:38:48 PDT 
Committed r259700: <https://trac.webkit.org/changeset/259700>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395758 [details].