Bug 133415 – Add a few more process specific sandbox profiles

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Add a few more process specific sandbox profiles

[Oliver Hunt]

Created attachment 232312 [details]
Patch

[Oliver Hunt]

<rdar://problem/17086128>

[Alexey Proskuryakov]

Comment on attachment 232312 [details]
Patch

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

> Source/WebKit2/ChangeLog:12
> +        These profiles are not perfect yet, but allow basic
> +        browsing to work by being a little looser than we'd like 
> +        them to be eventually.

I only made a few comments on the profiles, we'll need a deeper review eventually.

> Source/WebKit2/NetworkProcess/ios/NetworkProcessIOS.mm:60
> +    // Need to overide the default, because service has a different bundle ID.

Typo: should be override.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:46
> +			name = WebKit2SandBoxProfiles;
> +			productName = WebKit2SandBoxProfiles;

"Sandbox" is a word, so it shouldn't be capitalized like two words. Please correct.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:1939
> +			dstPath = /usr/local/share/sandbox/embedded/profiles/builtin;
> +			dstSubfolderSpec = 0;
> +			files = (
> +				A7AADA1619395CD9003EA1C7 /* com.apple.WebKit.DatabasesIOS.sb in CopyFiles */,
> +				A7AADA1719395CED003EA1C7 /* com.apple.WebKit.NetworkProcessIOS.sb in CopyFiles */,
> +				A7AADA1819395CF6003EA1C7 /* com.apple.WebProcessIOS.sb in CopyFiles */,
> +			);

Don't the copied files need names that match bundle IDs? Otherwise, I don't see how launched will manage to find them.

Not sure if we have .Development variants on iOS.

Or does the arbitrary name of profile go somewhere in Info.plist or entitlements?

> Source/WebKit2/WebProcess/cocoa/WebProcessCocoa.mm:249
> +#if PLATFORM(IOS)
> +    sandboxParameters.setOverrideSandboxProfilePath([webkit2Bundle pathForResource:@"com.apple.WebProcessIOS" ofType:@"sb"]);
> +#else

Shouldn't this be under a MANUAL ifdef?

> Source/WebKit2/WebProcess/com.apple.WebProcessIOS.sb:118
> +;; Various services required by AppKit and other frameworks

There is no AppKit on iOS.

> Source/WebKit2/WebProcess/com.apple.WebProcessIOS.sb:158
> +(allow mach-lookup
> +       (global-name "com.apple.ocspd")
> +       (global-name "com.apple.securityd"))
> +
> +;; CoreFoundation. We don't import com.apple.corefoundation.sb, because it allows unnecessary access to pasteboard.
> +(allow mach-lookup
> +    (global-name-regex #"^com.apple.distributed_notifications")
> +    (global-name "com.apple.CoreServices.coreservicesd"))

It would be nice to have uniform indentation in the newly added profiles (I prefer WebKit style with four spaces).

> Source/WebKit2/WebProcess/com.apple.WebProcessIOS.sb:179
> +(allow file-read* file-write* (container-subpath "/Library/Caches"))
> +(allow file-issue-extension)

These look very strange to me. Secondary processes have no idea about where container is.

How does (allow file-issue-extension) relate to AppSandbox extension rules above?

> Source/WebKit2/WebProcess/com.apple.WebProcessIOS.sb:183
> +;; FIXME: Should be removed once <rdar://problem/16329087> is fixed.
> +(deny file-write-xattr (xattr "com.apple.quarantine") (with no-log))

There is no quarantine on iOS, so this shouldn't be needed.

> Source/WebKit2/com.apple.WebKit.NetworkProcessIOS.sb:66
> +;; Access to client's cache folder & re-vending to CFNetwork.
> +(allow file-read* file-write*
> +       (extension "com.apple.nsurlstorage.extension-cache"))
> +
> +(allow file-issue-extension
> +    (require-all
> +        (extension-class "com.apple.nsurlstorage.extension-cache")
> +        (extension "com.apple.nsurlstorage.extension-cache")))

I don't understand how this works. We don't have any code that gives this extension to NetworkProcess, so it won't be able to access the cache, will it?

> Source/WebKit2/com.apple.WebKit.NetworkProcessIOS.sb:93
> +; FIXME: Should be removed once <rdar://problem/16329087> is fixed.
> +(deny file-write-xattr (xattr "com.apple.quarantine") (with no-log))

This is not needed.

[Oliver Hunt]

Created attachment 232331 [details]
Patch

Lets restructure all the ios profiles into something sane. Still waiting for build to complete.

[Oliver Hunt]

Created attachment 232333 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 232333 [details]
Patch

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

> Source/WebKit2/ChangeLog:23
> +        * Resources/SandboxProfiles/ios/com.apple.WebKit.Databases.sb: Added.
> +        * Resources/SandboxProfiles/ios/com.apple.WebKit.NetworkProcess.sb: Added.
> +        * Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb: Added.

We still need clarity on whether naming is important.

If it is important, then "com.apple.WebKit.NetworkProcess.sb" is wrong - bundle identifier for this process is "com.apple.WebKit.Networking", not "com.apple.WebKit.NetworkProcess". And I'm still unsure whether there are .Development variants, could you please check?

> Source/WebKit2/NetworkProcess/ios/NetworkProcessIOS.mm:60
> +    // Need to overide the default, because service has a different bundle ID.

Still "overide".

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:46
> +			productName = WebKit2SandBoxProfiles;

Please fix capitalization here, too.

[Oliver Hunt]

Created attachment 232345 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 232345 [details]
Patch

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

Still r=me

> Source/WebKit2/DatabaseProcess/ios/DatabaseProcessIOS.mm:55
>      // Need to overide the default, because service has a different bundle ID.

overide

[Oliver Hunt]

Committed r169533: <http://trac.webkit.org/changeset/169533>