Bug 132963 - [iOS] Enable sandboxing for the database process
Summary: [iOS] Enable sandboxing for the database process
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
Depends on:
Reported: 2014-05-15 11:49 PDT by Oliver Hunt
Modified: 2014-05-21 13:52 PDT (History)
3 users (show)

See Also:

Patch (18.70 KB, patch)
2014-05-15 12:12 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (13.01 KB, patch)
2014-05-19 15:45 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (13.00 KB, patch)
2014-05-20 13:37 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (33.41 KB, patch)
2014-05-20 18:38 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (20.87 KB, patch)
2014-05-21 12:25 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2014-05-15 11:49:44 PDT
Enable sandboxing for the database process on ios
Comment 1 Oliver Hunt 2014-05-15 12:12:43 PDT
Created attachment 231530 [details]
Comment 2 Tim Horton 2014-05-15 12:18:36 PDT
Comment on attachment 231530 [details]

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>


> 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?
Comment 3 Oliver Hunt 2014-05-15 12:25:57 PDT
Comment on attachment 231530 [details]

Hmmm, messed up merge/refactoring to tidy.
Comment 4 Alexey Proskuryakov 2014-05-15 12:42:04 PDT
Comment on attachment 231530 [details]

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.
Comment 5 Oliver Hunt 2014-05-19 15:45:36 PDT
Created attachment 231727 [details]
Comment 6 Oliver Hunt 2014-05-20 13:37:38 PDT
Created attachment 231790 [details]
Comment 7 Darin Adler 2014-05-20 15:46:46 PDT
Comment on attachment 231790 [details]

Patch didn’t include the new file, ChildProcessIOS.mm
Comment 8 Oliver Hunt 2014-05-20 18:38:50 PDT
Created attachment 231805 [details]
Comment 9 Alexey Proskuryakov 2014-05-20 20:47:56 PDT
Comment on attachment 231805 [details]

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.
Comment 10 Alexey Proskuryakov 2014-05-20 21:03:02 PDT
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.
Comment 11 Oliver Hunt 2014-05-21 12:25:25 PDT
Created attachment 231835 [details]
Comment 12 Alexey Proskuryakov 2014-05-21 12:55:21 PDT
Comment on attachment 231835 [details]

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

> Source/WebKit2/Shared/ios/ChildProcessIOS.mm:43

Can any of the above includes be under this ifdef?
Comment 13 WebKit Commit Bot 2014-05-21 13:52:54 PDT
Comment on attachment 231835 [details]

Clearing flags on attachment: 231835

Committed r169176: <http://trac.webkit.org/changeset/169176>
Comment 14 WebKit Commit Bot 2014-05-21 13:52:57 PDT
All reviewed patches have been landed.  Closing bug.