Bug 132963 – [iOS] Enable sandboxing for the database process

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Enable sandboxing for the database process on ios

[Oliver Hunt]

Created attachment 231530 [details]
Patch

[Tim Horton]

Comment on attachment 231530 [details]
Patch

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

> Source/WebKit2/DatabaseProcess/ios/DatabaseProcessIOS.mm:34
>  #import <WebCore/LocalizedStrings.h>
> +#import <WebCore/FileSystem.h>

sorting

> Source/WebKit2/DatabaseProcess/ios/DatabaseProcessIOS.mm:54
> +    NSBundle *webkit2Bundle = [NSBundle bundleForClass:NSClassFromString(@"WKView")];

WKView is not long for this world, doesn’t seem like a great choice.

> Source/WebKit2/DatabaseProcess/ios/com.apple.WebKit.DatabasesIOS.sb.in:28
> +(allow file-read* file-write* (extension âcom.apple.YourProject.database_container_dirâ))

smart quotes? YourProject?

[Oliver Hunt]

Comment on attachment 231530 [details]
Patch

Hmmm, messed up merge/refactoring to tidy.

[Alexey Proskuryakov]

Comment on attachment 231530 [details]
Patch

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

r- because of hardcoded directory path, and because it doesn't look like this code is suitable in production builds.

I expected that you will need to modify WebKit.xcconfig to omit the profile from other platforms (and to fix my mistake with "com.apple.WebKit.DatabaseProcess.sb").

> Source/WebKit2/DatabaseProcess/ios/DatabaseProcessIOS.mm:58
> +    NSArray *paths = NSSearchPathForDirectoriesInDomains(NSLibraryDirectory, NSUserDomainMask, YES);
> +    NSString *libraryDirectory = [paths objectAtIndex:0];
> +    String databasePath = [libraryDirectory stringByAppendingPathComponent:@"WebKit/Databases"];

The database directory is not hardcoded to be this one - there is even a user default.

There is already code in WebContext::ensureDatabaseProcess() that creates an extension for this directory, which simply needs to be consumed when initialization message is received. The only change may be that we need to change extension class for iOS.

> Source/WebKit2/DatabaseProcess/ios/com.apple.WebKit.DatabasesIOS.sb.in:26
> +(deny default (with partial-symbolication))
> +(allow system-audit file-read-metadata)

You didn't include common.sb or removed-dev-nodes.sb, how did you determine that they are not needed?

> Source/WebKit2/Shared/ios/ChildProcessIOS.mm:35
> +#import <WebCore/SystemVersionMac.h>

Is this used in the iOS implementation?

> Source/WebKit2/Shared/ios/ChildProcessIOS.mm:55
> +extern "C" OSStatus SetApplicationIsDaemon(Boolean isDaemon);

This is not used on iOS (likely doesn't exist either).

> Source/WebKit2/Shared/ios/ChildProcessIOS.mm:70
> +void ChildProcess::initializeSandbox(const ChildProcessInitializationParameters& parameters, SandboxInitializationParameters& sandboxParameters)

I don't see how code in this function is necessary, given that launchd will initialize sandbox for us. This is only for local testing, isn't it?

> Source/WebKit2/Shared/ios/ChildProcessIOS.mm:72
> +    NSBundle *webkit2Bundle = [NSBundle bundleForClass:NSClassFromString(@"WKView")];

What Tim said.

[Oliver Hunt]

Created attachment 231727 [details]
Patch

[Oliver Hunt]

Created attachment 231790 [details]
Patch

[Darin Adler]

Comment on attachment 231790 [details]
Patch

Patch didn’t include the new file, ChildProcessIOS.mm

[Oliver Hunt]

Created attachment 231805 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231805 [details]
Patch

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

> Source/WebKit2/DatabaseProcess/com.apple.WebKit.Databases.sb.in:25
> +#include "ios/com.apple.WebKit.DatabasesIOS.sb.in"

I think that the database process profile needs to go directly to where we want it to be on iOS, which is a different place than on OS X. So, merging them like this will be problematic.

> Source/WebKit2/DatabaseProcess/ios/DatabaseProcessIOS.mm:54
> +    // Need to overide the default, because service has a different bundle ID.
> +    NSBundle *webkit2Bundle = [NSBundle bundleForClass:NSClassFromString(@"WKView")];
> +    sandboxParameters.setOverrideSandboxProfilePath([webkit2Bundle pathForResource:@"com.apple.WebKit.Databases" ofType:@"sb"]);

I still don't understand why this is needed, I thought we agreed that we didn't need any code to enter the sandbox.

[Alexey Proskuryakov]

DerivedSources.make has a rule to process the three profiles for Mac from .sb.in. We can put iOS versions in a directory(directories?) that is not in VPATH, if there is a name conflict - there is no need to preprocess them, as they won't need any ifdefs.

Then we need to install the profiles into /usr/local/share/sandbox/embedded/profiles/builtin (will need to discuss whether we need a build alias as suggested). I don't know how that's done, but Xcode must have a  way.

For local testing, the profiles can be put in another special place, and edited even without rebuilding or installing roots.

[Oliver Hunt]

Created attachment 231835 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231835 [details]
Patch

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

> Source/WebKit2/Shared/ios/ChildProcessIOS.mm:43
> +#define ENABLE_MANUAL_SANDBOXING 0

Can any of the above includes be under this ifdef?

[WebKit Commit Bot]

Comment on attachment 231835 [details]
Patch

Clearing flags on attachment: 231835

Committed r169176: <http://trac.webkit.org/changeset/169176>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.