Summary: | [iOS] Deny mach lookup access to network extension services in the WebContent sandbox | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, commit-queue, ggaren, sam, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | iPhone / iPad | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2019-11-06 16:25:05 PST
Created attachment 382986 [details]
Patch
Created attachment 384867 [details]
Patch
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 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. Created attachment 385030 [details]
Patch
(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! (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 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. Created attachment 385178 [details]
Patch
(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! Created attachment 385306 [details]
Patch
Comment on attachment 385306 [details]
Patch
Looks good. r=me
(In reply to Brent Fulgham from comment #12) > Comment on attachment 385306 [details] > Patch > > Looks good. r=me Thanks for reviewing, Brent! Comment on attachment 385306 [details] Patch Clearing flags on attachment: 385306 Committed r253351: <https://trac.webkit.org/changeset/253351> All reviewed patches have been landed. Closing bug. 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? |