Bug 219173 – [macOS] Adopt SPI to prevent establishing XPC connections to Launch Services

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Adopt SPI to prevent establishing XPC connections to Launch Services in the WebContent process on macOS.

[Radar WebKit Bug Importer]

<rdar://problem/71595536>

[Per Arne Vollan]

Created attachment 414621 [details]
Patch

[Per Arne Vollan]

Created attachment 414676 [details]
Patch

[Per Arne Vollan]

Created attachment 414678 [details]
Patch

[Per Arne Vollan]

Created attachment 414682 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Not sure I should have reviewed that older patch?

[Per Arne Vollan]

(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!

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 414735 [details]
Patch

[Per Arne Vollan]

Created attachment 415005 [details]
Patch

[Alex Christensen]

How will _LSSetApplicationInformationItem work with this?

[Per Arne Vollan]

(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!

[Alex Christensen]

Comment on attachment 415005 [details]
Patch

Oh I see ENABLE(SET_WEBCONTENT_PROCESS_INFORMATION_IN_NETWORK_PROCESS) now

[Per Arne Vollan]

Comment on attachment 415005 [details]
Patch

Thanks for reviewing!

[EWS]

Committed r270284: <https://trac.webkit.org/changeset/270284>

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