Bug 172182 - [WK2][iOS] Adopt a whitelist for XPC services
Summary: [WK2][iOS] Adopt a whitelist for XPC services
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 172462
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-16 12:12 PDT by Brent Fulgham
Modified: 2017-05-23 08:57 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.98 KB, patch)
2017-05-18 12:02 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (4.91 KB, patch)
2017-05-18 12:11 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (2.87 KB, patch)
2017-05-22 16:04 PDT, Brent Fulgham
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2017-05-16 12:12:17 PDT
Lock down the sandbox further by denying XPC services access by default, and only permitting connections to things we need to access.
Comment 1 Brent Fulgham 2017-05-16 12:12:28 PDT
<rdar://problem/30669445>
Comment 2 Brent Fulgham 2017-05-18 12:02:30 PDT
Created attachment 310526 [details]
Patch
Comment 3 Brent Fulgham 2017-05-18 12:11:34 PDT
Created attachment 310528 [details]
Patch
Comment 4 Geoffrey Garen 2017-05-18 12:21:31 PDT
Comment on attachment 310528 [details]
Patch

How did we come up with the list of needed services? Might there be services used on unlikely paths that need whitelisting?
Comment 5 Brent Fulgham 2017-05-18 12:32:10 PDT
(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 310528 [details]
> Patch
> 
> How did we come up with the list of needed services? Might there be services
> used on unlikely paths that need whitelisting?

These are based on discussion with the Sandboxing team and the global sandboxes they set up for 3rd-party applications. I did local testing with a handful of iOS units of varying screen size and features, but it was not exhaustive.

I'd like to get this in a build so that we could get larger QC on it in case we need to add anything else.
Comment 6 Sam Weinig 2017-05-18 17:20:55 PDT
Comment on attachment 310528 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +
> +        * Resources/SandboxProfiles/ios/com.apple.WebKit.Databases.sb:
> +        * Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:
> +        * Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:

This could be more fleshed out.

> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.Databases.sb:48
> +;; Various services required by system frameworks
> +(allow mach-lookup
> +    (global-name "com.apple.analyticsd")
> +    (global-name "com.apple.lsd.mapdb"))

Are these related to the XPC deny change?

> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:70
>  
> +(deny iokit-get-properties)
> +(allow iokit-get-properties
> +    (iokit-property-prefix "AGXParameterBufferMaxSize")
> +    (iokit-property-regex #"^AppleJPEG") ; AppleJPEGDriver, EmbeddedCoreMedia
> +    (iokit-property "AppleTV")
> +    (iokit-property "BaseAddressAlignmentRequirement")
> +    (iokit-property "CFBundleIdentifier")
> +    (iokit-property "DisplayPipePlaneBaseAlignment")
> +    (iokit-property "DisplayPipeStrideRequirements")
> +    (iokit-property "ForceSupported")
> +    (iokit-property-regex "^IOGL(|ES)(|Metal)BundleName")
> +    (iokit-property-regex #"^InternalStatistics(|Accm)")
> +    (iokit-property-regex #"^MetalPlugin(Name|ClassName)")
> +    (iokit-property-regex #"^PerformanceStatistics(|Accum)")
> +    (iokit-property "Protocol Characteristics")
> +    (iokit-property "Size")
> +    (iokit-property "compass-calibration")
> +    (iokit-property "display-rotation")
> +    (iokit-property "display-scale"")
> +    (iokit-property "graphic-options")
> +    (iokit-property "gyro-interrupt-calibration")
> +    (iokit-property "hdcp-hoover-protocol")
> +    (iokit-property-regex #"^parser-(options|type)")
> +    (iokit-property "product-id")
> +    (iokit-property "software-behavior")

These don't seem like XPC services.

> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:77
> +    (xpc-service-name-regex #"\.viewservice$")

What is this? It seems scarier than the other two.

> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:125
> +    (global-name "com.apple.analyticsd")
>      (global-name "com.apple.accountsd.accountmanager")
> -    (global-name "com.apple.coremedia.audiodeviceclock"))
> +    (global-name "com.apple.coremedia.audiodeviceclock")
> +    (global-name "com.apple.lsd.mapdb")

How do these relate to the XPC whitelist change?
Comment 7 Brent Fulgham 2017-05-19 14:50:51 PDT
Comment on attachment 310528 [details]
Patch

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

>> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:70
>> +    (iokit-property "software-behavior")
> 
> These don't seem like XPC services.

Yeah, I'll split these out into a separate patch. I was trying to get the iOS and macOS sandboxes in sync.

>> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:77
>> +    (xpc-service-name-regex #"\.viewservice$")
> 
> What is this? It seems scarier than the other two.

Right now we don't block any of these things. The Sandboxing team gave me a list of things they are whitelisting in the global application sandboxes, which included these three items. The others were for things without any plausible relevance to WebContent process, so I didn't include them.

These seemed like the might be used under some code paths, so I whitelisted them to avoid the chance of breaking something.

>> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:125
>> +    (global-name "com.apple.lsd.mapdb")
> 
> How do these relate to the XPC whitelist change?

'analyticsd' is needed in newer OS's to support some logging features. We should be whitelisting this, but I can do it as a separate patch.

'lsd.mapdb' should have been whitelisted a few releases ago, but was overlooked since no one was keeping an eye on things. Blocking it means we are making core services do extra work on iOS (prevents access to a cache)
Comment 8 Sam Weinig 2017-05-19 18:14:08 PDT
(In reply to Brent Fulgham from comment #7)
> Comment on attachment 310528 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310528&action=review
> 
> >> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:70
> >> +    (iokit-property "software-behavior")
> > 
> > These don't seem like XPC services.
> 
> Yeah, I'll split these out into a separate patch. I was trying to get the
> iOS and macOS sandboxes in sync.

Cool.

> 
> >> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:77
> >> +    (xpc-service-name-regex #"\.viewservice$")
> > 
> > What is this? It seems scarier than the other two.
> 
> Right now we don't block any of these things. The Sandboxing team gave me a
> list of things they are whitelisting in the global application sandboxes,
> which included these three items. The others were for things without any
> plausible relevance to WebContent process, so I didn't include them.
> 
> These seemed like the might be used under some code paths, so I whitelisted
> them to avoid the chance of breaking something.

Yeah, but what is #"\.viewservice$ ?
> 
> >> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:125
> >> +    (global-name "com.apple.lsd.mapdb")
> > 
> > How do these relate to the XPC whitelist change?
> 
> 'analyticsd' is needed in newer OS's to support some logging features. We
> should be whitelisting this, but I can do it as a separate patch.

Is it something we want a compromised WebProcess to have access to?

> 
> 'lsd.mapdb' should have been whitelisted a few releases ago, but was
> overlooked since no one was keeping an eye on things. Blocking it means we
> are making core services do extra work on iOS (prevents access to a cache)

Is this cache something we want a compromised WebProcess to have access to? Does it contain all the installed applications, for instance?
Comment 9 Brent Fulgham 2017-05-22 16:04:08 PDT
Created attachment 310945 [details]
Patch
Comment 10 Sam Weinig 2017-05-22 20:03:01 PDT
Comment on attachment 310945 [details]
Patch

Still curious about those questions, but, this looks good. Ice cold, no XPC!
Comment 11 Brent Fulgham 2017-05-23 08:57:09 PDT
Committed r217277: <http://trac.webkit.org/changeset/217277>