Bug 203929 - [iOS] Deny mach lookup access to network extension services in the WebContent sandbox
Summary: [iOS] Deny mach lookup access to network extension services in the WebContent...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-06 16:25 PST by Per Arne Vollan
Modified: 2020-01-09 12:39 PST (History)
6 users (show)

See Also:


Attachments
Patch (13.09 KB, patch)
2019-11-06 16:54 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (12.92 KB, patch)
2019-12-04 17:17 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.33 KB, patch)
2019-12-06 11:50 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (19.12 KB, patch)
2019-12-09 12:06 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.88 KB, patch)
2019-12-10 13:58 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-11-06 16:25:05 PST
The WebContent process' sandbox should deny mach lookup to network extension services.
Comment 1 Per Arne Vollan 2019-11-06 16:54:05 PST
Created attachment 382986 [details]
Patch
Comment 2 Per Arne Vollan 2019-12-04 17:17:25 PST
Created attachment 384867 [details]
Patch
Comment 3 Brent Fulgham 2019-12-04 17:23:21 PST
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.
Comment 4 Alex Christensen 2019-12-04 21:44:59 PST
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.
Comment 5 Per Arne Vollan 2019-12-06 11:50:16 PST
Created attachment 385030 [details]
Patch
Comment 6 Per Arne Vollan 2019-12-06 12:03:17 PST
(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!
Comment 7 Per Arne Vollan 2019-12-06 12:11:09 PST
(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.
Comment 8 Sam Weinig 2019-12-06 18:44:12 PST
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.
Comment 9 Per Arne Vollan 2019-12-09 12:06:15 PST
Created attachment 385178 [details]
Patch
Comment 10 Per Arne Vollan 2019-12-09 16:32:04 PST
(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!
Comment 11 Per Arne Vollan 2019-12-10 13:58:27 PST
Created attachment 385306 [details]
Patch
Comment 12 Brent Fulgham 2019-12-10 14:58:28 PST
Comment on attachment 385306 [details]
Patch

Looks good. r=me
Comment 13 Per Arne Vollan 2019-12-10 15:03:07 PST
(In reply to Brent Fulgham from comment #12)
> Comment on attachment 385306 [details]
> Patch
> 
> Looks good. r=me

Thanks for reviewing, Brent!
Comment 14 WebKit Commit Bot 2019-12-10 15:21:49 PST
Comment on attachment 385306 [details]
Patch

Clearing flags on attachment: 385306

Committed r253351: <https://trac.webkit.org/changeset/253351>
Comment 15 WebKit Commit Bot 2019-12-10 15:21:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-12-10 15:23:00 PST
<rdar://problem/57811107>
Comment 17 Brent Fulgham 2020-01-09 12:39:26 PST
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?