Bug 203929 – [iOS] Deny mach lookup access to network extension services in the WebContent sandbox

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

The WebContent process' sandbox should deny mach lookup to network extension services.

[Per Arne Vollan]

Created attachment 382986 [details]
Patch

[Per Arne Vollan]

Created attachment 384867 [details]
Patch

[Brent Fulgham]

Comment on attachment 384867 [details]
Patch

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

I think this looks correct, but I think it would be beneficial to make this change on macOS, too (at least for 10.15 and newer).

> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:480
> +    (allow mach-lookup (with report) (with telemetry)

Can't we do this on macOS, too? We allow this access on 10.15, too!

> Source/WebKit/Shared/WebProcessCreationParameters.cpp:403
> +#if PLATFORM(IOS)

Seems like this would be useful on macOS.

[Alex Christensen]

Comment on attachment 384867 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        the content filtering code should be moved to the Networking process, but since this is a
> +        bigger undertaking, we can issue extensions in the meantime to strengthen the sandbox.

I don't think moving the content filtering code to the network process would be a terribly large undertaking.  It would just be moving the logic to NetworkResourceLoader.

[Per Arne Vollan]

Created attachment 385030 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 384867 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384867&action=review
> 
> I think this looks correct, but I think it would be beneficial to make this
> change on macOS, too (at least for 10.15 and newer).
> 
> > Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:480
> > +    (allow mach-lookup (with report) (with telemetry)
> 
> Can't we do this on macOS, too? We allow this access on 10.15, too!
> 
> > Source/WebKit/Shared/WebProcessCreationParameters.cpp:403
> > +#if PLATFORM(IOS)
> 
> Seems like this would be useful on macOS.

I have enabled this for macOS as well in the latest patch.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 384867 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384867&action=review
> 
> > Source/WebKit/ChangeLog:12
> > +        the content filtering code should be moved to the Networking process, but since this is a
> > +        bigger undertaking, we can issue extensions in the meantime to strengthen the sandbox.
> 
> I don't think moving the content filtering code to the network process would
> be a terribly large undertaking.  It would just be moving the logic to
> NetworkResourceLoader.

That's very good news! I think it would be good to move the code to the Networking process, as well as issue an extension, since this will also be useful in the Networking process.

[Sam Weinig]

Comment on attachment 385030 [details]
Patch

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

> Source/WebCore/platform/cocoa/NetworkExtensionContentFilter.h:65
> +    WEBCORE_EXPORT static Optional<bool> m_hasConsumedSandboxExtensions;

I think Optional<bool> here obfuscates the meaning a bit. I think you would be better off with a three state enum.

[Per Arne Vollan]

Created attachment 385178 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 385030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385030&action=review
> 
> > Source/WebCore/platform/cocoa/NetworkExtensionContentFilter.h:65
> > +    WEBCORE_EXPORT static Optional<bool> m_hasConsumedSandboxExtensions;
> 
> I think Optional<bool> here obfuscates the meaning a bit. I think you would
> be better off with a three state enum.

I have update the patch to use an enum.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 385306 [details]
Patch

[Brent Fulgham]

Comment on attachment 385306 [details]
Patch

Looks good. r=me

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #12)
> Comment on attachment 385306 [details]
> Patch
> 
> Looks good. r=me

Thanks for reviewing, Brent!

[WebKit Commit Bot]

Comment on attachment 385306 [details]
Patch

Clearing flags on attachment: 385306

Committed r253351: <https://trac.webkit.org/changeset/253351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57811107>

[Brent Fulgham]

Should have a version check in here for this:

SandboxExtension::createHandleForMachLookup("com.apple.nesessionmanager.content-filter", WTF::nullopt, handle);

Because prior to 10.15 this was "com.apple.nesessionmanager" without the content-filter extension?