Bug 219173

Summary: [macOS] Adopt SPI to prevent establishing XPC connections to Launch Services
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch none

Description Per Arne Vollan 2020-11-19 11:46:00 PST
Adopt SPI to prevent establishing XPC connections to Launch Services in the WebContent process on macOS.
Comment 1 Radar WebKit Bug Importer 2020-11-19 11:46:20 PST
<rdar://problem/71595536>
Comment 2 Per Arne Vollan 2020-11-19 14:29:49 PST
Created attachment 414621 [details]
Patch
Comment 3 Per Arne Vollan 2020-11-20 08:15:45 PST
Created attachment 414676 [details]
Patch
Comment 4 Per Arne Vollan 2020-11-20 08:31:05 PST
Created attachment 414678 [details]
Patch
Comment 5 Per Arne Vollan 2020-11-20 08:40:02 PST
Created attachment 414682 [details]
Patch
Comment 6 Darin Adler 2020-11-20 11:52:44 PST
Comment on attachment 414676 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessProxy.cpp:280
> +    Optional<WebKit::SandboxExtension::Handle> extension;
> +#if PLATFORM(MAC)

Should add a blank line here to make the formatting match the function below.

> Source/WebKit/WebProcess/WebProcess.h:518
> +    void updateProcessName(Optional<WebKit::SandboxExtension::Handle> = WTF::nullopt);

Why doesn’t this function take an rvalue reference?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:451
> +    if (handle) {

Since Handle already has a null value, we do not need to use Optional<>. If we used just Handle everywhere, not Optional<Handle>, and took out this null check, the code would work fine. SandboxExtension::create already does nothing and returns nullptr if you call it with a null Handle.
Comment 7 Darin Adler 2020-11-20 11:53:18 PST
Not sure I should have reviewed that older patch?
Comment 8 Per Arne Vollan 2020-11-20 14:54:22 PST
(In reply to Darin Adler from comment #6)
> Comment on attachment 414676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414676&action=review
> 
> > Source/WebKit/UIProcess/WebProcessProxy.cpp:280
> > +    Optional<WebKit::SandboxExtension::Handle> extension;
> > +#if PLATFORM(MAC)
> 
> Should add a blank line here to make the formatting match the function below.
> 

Will fix.

> > Source/WebKit/WebProcess/WebProcess.h:518
> > +    void updateProcessName(Optional<WebKit::SandboxExtension::Handle> = WTF::nullopt);
> 
> Why doesn’t this function take an rvalue reference?
> 

I believe it can, will change! 

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:451
> > +    if (handle) {
> 
> Since Handle already has a null value, we do not need to use Optional<>. If
> we used just Handle everywhere, not Optional<Handle>, and took out this null
> check, the code would work fine. SandboxExtension::create already does
> nothing and returns nullptr if you call it with a null Handle.

That is a good point, I will remove the Optional.

Thanks for reviewing!
Comment 9 Per Arne Vollan 2020-11-20 14:55:49 PST
(In reply to Darin Adler from comment #7)
> Not sure I should have reviewed that older patch?

I am currently working on some minor modifications. I will possibly upload another revised patch for review.

Thanks!
Comment 10 Per Arne Vollan 2020-11-20 15:09:39 PST
Created attachment 414735 [details]
Patch
Comment 11 Per Arne Vollan 2020-11-29 23:08:34 PST
Created attachment 415005 [details]
Patch
Comment 12 Alex Christensen 2020-11-30 12:39:45 PST
How will _LSSetApplicationInformationItem work with this?
Comment 13 Per Arne Vollan 2020-11-30 13:02:26 PST
(In reply to Alex Christensen from comment #12)
> How will _LSSetApplicationInformationItem work with this?

These calls will be forwarded to the Networking process.

Thanks for reviewing!
Comment 14 Alex Christensen 2020-11-30 13:32:06 PST
Comment on attachment 415005 [details]
Patch

Oh I see ENABLE(SET_WEBCONTENT_PROCESS_INFORMATION_IN_NETWORK_PROCESS) now
Comment 15 Per Arne Vollan 2020-11-30 22:36:58 PST
Comment on attachment 415005 [details]
Patch

Thanks for reviewing!
Comment 16 EWS 2020-11-30 22:49:26 PST
Committed r270284: <https://trac.webkit.org/changeset/270284>

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