Bug 220525 – [Cocoa] Network extension sandbox extensions are sometimes issued too late

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

[Per Arne Vollan]

Currently, Network extension sandbox extensions are sent to the WebContent process as part of the load parameters, but this is too late in some cases.

[Per Arne Vollan]

<rdar://problem/68443565>

[Per Arne Vollan]

Created attachment 417410 [details]
Patch

[Darin Adler]

Comment on attachment 417410 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:5024
> +        return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager"_s }, WTF::nullopt, managerHandle);

What is “managerHandle”? Doesn’t look like this compiles.

[Per Arne Vollan]

Created attachment 417411 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #3)
> Comment on attachment 417410 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417410&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:5024
> > +        return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager"_s }, WTF::nullopt, managerHandle);
> 
> What is “managerHandle”? Doesn’t look like this compiles.

Good catch! I have uploaded another patch.

Thanks for reviewing!

[Darin Adler]

Comment on attachment 417411 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        Currently, Network extension sandbox extensions are sent to the WebContent process as part of the load parameters, but this is too late in some cases.
> +        In these cases, the extensions can be sent along with the DidReceivePolicyDecision message.

Given this I would have expected to see code *moving* but instead this patch has code *added*.

> Source/WebKit/UIProcess/WebPageProxy.cpp:5027
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
> +        return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager"_s }, WTF::nullopt);
> +#else
> +        return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager.content-filter"_s }, WTF::nullopt);
> +#endif

Would have liked to see an even tighter version of this that uses a local variable and has less code inside #if with "constexpr ASCIILiteral".

[Per Arne Vollan]

(In reply to Darin Adler from comment #6)
> Comment on attachment 417411 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417411&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        Currently, Network extension sandbox extensions are sent to the WebContent process as part of the load parameters, but this is too late in some cases.
> > +        In these cases, the extensions can be sent along with the DidReceivePolicyDecision message.
> 
> Given this I would have expected to see code *moving* but instead this patch
> has code *added*.
> 

Initially, I thought the extensions that are sent with the load parameters were needed, since the message with load parameters is normally sent before the policy decision message, but after reconsidering, I now think this can be removed, since the policy decision message should in all cases be handled in the WebContent process before the Network Extension services are being accessed. I will update the patch accordingly.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:5027
> > +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
> > +        return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager"_s }, WTF::nullopt);
> > +#else
> > +        return SandboxExtension::createHandlesForMachLookup({ "com.apple.nehelper"_s, "com.apple.nesessionmanager.content-filter"_s }, WTF::nullopt);
> > +#endif
> 
> Would have liked to see an even tighter version of this that uses a local
> variable and has less code inside #if with "constexpr ASCIILiteral".

Will fix!

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 417412 [details]
Patch

[Per Arne Vollan]

This patch has been verified to fix the issue.

[Brent Fulgham]

Comment on attachment 417412 [details]
Patch

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

I think this is good, but would be improved by refactoring slightly.

> Source/WebKit/UIProcess/WebPageProxy.cpp:302
> +#endif

You won't need this Cocoa-specific include if you leave the implementation in WebPageProxyCocoa.mm and use a stub in WebPageProxy.cpp for non-Cocoa clients.

> Source/WebKit/UIProcess/WebPageProxy.cpp:5018
> +static SandboxExtension::HandleArray createNetworkExtensionsSandboxExtensions(WebProcessProxy& process)

I think this would be cleaner if this method stayed in WebPageProxyCocoa.mm, with a stub in WebPageProxy.cpp for non-Cocoa cases.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:156
>  

Then you could just move the below code into a method body instead of deleting it here.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:346
> +#endif

You could avoid adding this cocoa-specific header here, if you moved the Sandbox consumption to WebPageCocoa.mm, which already knows about content filtering and sandbox stuff.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3375
> +#endif

I think this would be cleaner with a new method stub in this file called "consumeSandboxExtensions" that is a no-op for non-Cocoa builds. Then the real implementation could live in WebPageCocoa.mm, which already knows about all the important things and no new headers need to be added anywhere.

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:-90
> -#endif

... and this could just be moved to a new method (well, I guess the conditionals wouldn't be needed anymore in the new code path).

[Per Arne Vollan]

Created attachment 417554 [details]
Patch

[Per Arne Vollan]

Created attachment 417556 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #10)
> Comment on attachment 417412 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417412&action=review
> 
> I think this is good, but would be improved by refactoring slightly.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:302
> > +#endif
> 
> You won't need this Cocoa-specific include if you leave the implementation
> in WebPageProxyCocoa.mm and use a stub in WebPageProxy.cpp for non-Cocoa
> clients.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:5018
> > +static SandboxExtension::HandleArray createNetworkExtensionsSandboxExtensions(WebProcessProxy& process)
> 
> I think this would be cleaner if this method stayed in WebPageProxyCocoa.mm,
> with a stub in WebPageProxy.cpp for non-Cocoa cases.
> 
> > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:156
> >  
> 
> Then you could just move the below code into a method body instead of
> deleting it here.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:346
> > +#endif
> 
> You could avoid adding this cocoa-specific header here, if you moved the
> Sandbox consumption to WebPageCocoa.mm, which already knows about content
> filtering and sandbox stuff.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3375
> > +#endif
> 
> I think this would be cleaner with a new method stub in this file called
> "consumeSandboxExtensions" that is a no-op for non-Cocoa builds. Then the
> real implementation could live in WebPageCocoa.mm, which already knows about
> all the important things and no new headers need to be added anywhere.
> 
> > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:-90
> > -#endif
> 
> ... and this could just be moved to a new method (well, I guess the
> conditionals wouldn't be needed anymore in the new code path).

These improvements have been addressed in the latest patch.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 417557 [details]
Patch

[Brent Fulgham]

Comment on attachment 417557 [details]
Patch

Looks great! Thank you for addressing my earlier comments.

[Per Arne Vollan]

Comment on attachment 417557 [details]
Patch

Thanks for reviewing!

[EWS]

Committed r271469: <https://trac.webkit.org/changeset/271469>

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