Bug 220525

Summary: [Cocoa] Network extension sandbox extensions are sometimes issued too late
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch none

Description Per Arne Vollan 2021-01-11 13:46:10 PST
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.
Comment 1 Per Arne Vollan 2021-01-11 13:46:31 PST
<rdar://problem/68443565>
Comment 2 Per Arne Vollan 2021-01-11 14:00:30 PST
Created attachment 417410 [details]
Patch
Comment 3 Darin Adler 2021-01-11 14:08:07 PST
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.
Comment 4 Per Arne Vollan 2021-01-11 14:12:20 PST
Created attachment 417411 [details]
Patch
Comment 5 Per Arne Vollan 2021-01-11 14:13:14 PST
(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!
Comment 6 Darin Adler 2021-01-11 14:20:48 PST
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".
Comment 7 Per Arne Vollan 2021-01-11 14:38:27 PST
(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!
Comment 8 Per Arne Vollan 2021-01-11 15:17:41 PST
Created attachment 417412 [details]
Patch
Comment 9 Per Arne Vollan 2021-01-12 10:08:43 PST
This patch has been verified to fix the issue.
Comment 10 Brent Fulgham 2021-01-13 10:32:38 PST
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).
Comment 11 Per Arne Vollan 2021-01-13 13:13:51 PST
Created attachment 417554 [details]
Patch
Comment 12 Per Arne Vollan 2021-01-13 13:24:15 PST
Created attachment 417556 [details]
Patch
Comment 13 Per Arne Vollan 2021-01-13 13:27:02 PST
(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!
Comment 14 Per Arne Vollan 2021-01-13 13:38:32 PST
Created attachment 417557 [details]
Patch
Comment 15 Brent Fulgham 2021-01-13 15:13:48 PST
Comment on attachment 417557 [details]
Patch

Looks great! Thank you for addressing my earlier comments.
Comment 16 Per Arne Vollan 2021-01-13 15:15:25 PST
Comment on attachment 417557 [details]
Patch

Thanks for reviewing!
Comment 17 EWS 2021-01-13 15:27:23 PST
Committed r271469: <https://trac.webkit.org/changeset/271469>

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