Bug 134360 - Restrict network process sandbox
Summary: Restrict network process sandbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on: 134533
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-26 13:43 PDT by Oliver Hunt
Modified: 2014-07-02 16:03 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.82 KB, patch)
2014-06-26 13:54 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (11.97 KB, patch)
2014-06-28 14:44 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (12.04 KB, patch)
2014-06-30 15:11 PDT, Oliver Hunt
sam: review+
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-06-26 13:43:01 PDT
Restrict network process sandbox
Comment 1 Oliver Hunt 2014-06-26 13:54:58 PDT
Created attachment 233931 [details]
Patch
Comment 2 Sam Weinig 2014-06-26 13:59:23 PDT
Comment on attachment 233931 [details]
Patch

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

You've got inconsistent spacing in the profile.

> Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:64
>  {
> +
> +    m_diskCacheDirectory = parameters.diskCacheDirectory;

Extraneous newline.  How is this path validated? What prevents an app from passing in a path it should not have access to?
Comment 3 Sam Weinig 2014-06-26 14:06:35 PDT
Comment on attachment 233931 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:64
>> +    m_diskCacheDirectory = parameters.diskCacheDirectory;
> 
> Extraneous newline.  How is this path validated? What prevents an app from passing in a path it should not have access to?

Never mind about the validation. This is only allowed via an extension.

> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:39
> +;; Utility functions for home directory relative path filters
> +(define (home-regex home-relative-regex)
> +  (regex (string-append #"^/private/var/mobile" home-relative-regex)))
> +
> +(define (home-subpath home-relative-subpath)
> +    (subpath (string-append "/private/var/mobile" home-relative-subpath)))
> +
> +(define (home-literal home-relative-literal)
> +    (literal (string-append "/private/var/mobile" home-relative-literal)))

These seem unused.

> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:43
> +(apple-cookie-access 'with-read-write)

Does this allow any WebKit2 client to access the same cookies? I don't think we want that.
Comment 4 Oliver Hunt 2014-06-26 14:19:19 PDT
(In reply to comment #3)

> 
> > Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:39
> > +;; Utility functions for home directory relative path filters
> > +(define (home-regex home-relative-regex)
> > +  (regex (string-append #"^/private/var/mobile" home-relative-regex)))
> > +
> > +(define (home-subpath home-relative-subpath)
> > +    (subpath (string-append "/private/var/mobile" home-relative-subpath)))
> > +
> > +(define (home-literal home-relative-literal)
> > +    (literal (string-append "/private/var/mobile" home-relative-literal)))
> 
> These seem unused.
>
Fixed
 
> > Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:43
> > +(apple-cookie-access 'with-read-write)
> 
> Does this allow any WebKit2 client to access the same cookies? I don't think we want that.

That's current behaviour, this merely allows us to continue doing what we currently do.
Comment 5 Sam Weinig 2014-06-26 15:48:45 PDT
(In reply to comment #4)
> (In reply to comment #3)
> 
> > 
> > > Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:39
> > > +;; Utility functions for home directory relative path filters
> > > +(define (home-regex home-relative-regex)
> > > +  (regex (string-append #"^/private/var/mobile" home-relative-regex)))
> > > +
> > > +(define (home-subpath home-relative-subpath)
> > > +    (subpath (string-append "/private/var/mobile" home-relative-subpath)))
> > > +
> > > +(define (home-literal home-relative-literal)
> > > +    (literal (string-append "/private/var/mobile" home-relative-literal)))
> > 
> > These seem unused.
> >
> Fixed

Thanks.

> 
> > > Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:43
> > > +(apple-cookie-access 'with-read-write)
> > 
> > Does this allow any WebKit2 client to access the same cookies? I don't think we want that.
> 
> That's current behaviour, this merely allows us to continue doing what we currently do.

Ok. Please file a bug on this.
Comment 6 Alexey Proskuryakov 2014-06-26 23:08:31 PDT
Comment on attachment 233931 [details]
Patch

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

> Source/WebKit2/Resources/SandboxProfiles/ios/com.apple.WebKit.Networking.sb:62
> +;; Access to own cache & temp folders.
> +(allow file-read* file-write*
> +       (extension "com.apple.webkit.read-write"))
> +
> +;; IOKit user clients
> +(allow iokit-open
> +    (iokit-user-client-class "RootDomainUserClient"))

Please fix the profile to use uniform indentation (4 spaces would be nice).
Comment 7 Oliver Hunt 2014-06-27 09:05:21 PDT
Comment on attachment 233931 [details]
Patch

I'm making a few changes that will require re-review (not just reformatting :) )
Comment 8 Oliver Hunt 2014-06-28 14:44:36 PDT
Created attachment 234053 [details]
Patch
Comment 9 Darin Adler 2014-06-30 08:10:07 PDT
Comment on attachment 234053 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:67
> +    SandboxExtension::consumePermanently(parameters.cookieStorageDirectoryExtensionHandle);
> +    m_diskCacheDirectory = parameters.diskCacheDirectory;
> +
> +    if (!m_diskCacheDirectory.isNull()) {
> +        if (!SandboxExtension::consumePermanently(parameters.diskCacheDirectoryExtensionHandle))

Why are we checking the return value from consuming diskCacheDirectoryExtensionHandle yet not doing the same for cookieStorageDirectoryExtensionHandle just above?

Code paragraphing here is peculiar. The code to set m_diskCacheDirectory is grouped with the code to work with cookieStorageDirectoryExtensionHandle instead of with the code for diskCacheDirectoryExtensionHandle. I’d move the blank line or remove it.

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:74
> +    char path[MAXPATHLEN + 1];
> +    if (sandbox_container_path_for_pid(getpid(), path, MAXPATHLEN))
> +        return String();

This is passing the wrong buffer size. It should not pass the size minus one, but rather the entire size. I suggest sizeof(path).

It’s a little strange that processHasContainer above is using std::array but we are opting not to use it here.

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:77
> +    if (!path[0])
> +        return String();

Is there some reason it’s important to return a null string instead of an empty string in this case? If it’s not, I suggest just omitting this special case as the normal case below will work for the empty string.

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:79
> +    path[MAXPATHLEN] = 0;

Not needed. There’s no reason that sandbox_container_path_for_pid would return a non-terminated string and pretend to succeed.

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:80
> +    return String(path);

Since the path is a UTF-8 string, we should not be using the constructor that treats characters as Latin-1.

WebCore’s FileSystem.h code even goes so far as to use CFStringGetFileSystemRepresentation; the corresponding thing to do here would be to use CFStringCreateWithFileSystemRepresentation.

But it might be OK to call String::fromUTF8.

> Source/WebKit2/Shared/mac/SandboxUtilities.h:30
> +#include <wtf/text/WTFString.h>

No need to include this header just to return an object of type WTF::String. Instead, we could include wtf/Forward.h.

> Source/WebKit2/Shared/mac/SandboxUtilities.h:37
> +// Returns an empty string if the process is not in a container

The code seems to return a null string in this case, not an empty string.

We normally use a period at the end of these kinds of comments.

> Source/WebKit2/UIProcess/WebContext.cpp:414
> +    parameters.cookieStorageDirectory = (cookieStorageDirectory());

The extra parentheses here are a bit non-idiomatic. I’d leave them out.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:276
> +        path = [@"~/" stringByStandardizingPath];

Maybe use NSHomeDirectory() instead?
Comment 10 Oliver Hunt 2014-06-30 10:46:37 PDT
Comment on attachment 234053 [details]
Patch

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

Whoops, sorry i should have removed the r? as i was still making changes :)

Thanks for the feedback though :D

>> Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:67
>> +        if (!SandboxExtension::consumePermanently(parameters.diskCacheDirectoryExtensionHandle))
> 
> Why are we checking the return value from consuming diskCacheDirectoryExtensionHandle yet not doing the same for cookieStorageDirectoryExtensionHandle just above?
> 
> Code paragraphing here is peculiar. The code to set m_diskCacheDirectory is grouped with the code to work with cookieStorageDirectoryExtensionHandle instead of with the code for diskCacheDirectoryExtensionHandle. I’d move the blank line or remove it.

Hmmm, i'm looking at the diff and i think what happened is that i introduce the crash while debugging.

>> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:74
>> +        return String();
> 
> This is passing the wrong buffer size. It should not pass the size minus one, but rather the entire size. I suggest sizeof(path).
> 
> It’s a little strange that processHasContainer above is using std::array but we are opting not to use it here.

Yeah, i didn't want to do the extra alloc, but guess this should be hot and i'm doing an unnecessary premature optimisation.

>> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:77
>> +        return String();
> 
> Is there some reason it’s important to return a null string instead of an empty string in this case? If it’s not, I suggest just omitting this special case as the normal case below will work for the empty string.

Ah, ok

>> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:79
>> +    path[MAXPATHLEN] = 0;
> 
> Not needed. There’s no reason that sandbox_container_path_for_pid would return a non-terminated string and pretend to succeed.

Ah, ok, i'm just super paranoid about such things

>> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:80
>> +    return String(path);
> 
> Since the path is a UTF-8 string, we should not be using the constructor that treats characters as Latin-1.
> 
> WebCore’s FileSystem.h code even goes so far as to use CFStringGetFileSystemRepresentation; the corresponding thing to do here would be to use CFStringCreateWithFileSystemRepresentation.
> 
> But it might be OK to call String::fromUTF8.

I'll do CFString.... to be consistent with webcore

>> Source/WebKit2/Shared/mac/SandboxUtilities.h:30
>> +#include <wtf/text/WTFString.h>
> 
> No need to include this header just to return an object of type WTF::String. Instead, we could include wtf/Forward.h.

Even for value returns? a forward decl would not allow the surely? What am i missing?

>> Source/WebKit2/Shared/mac/SandboxUtilities.h:37
>> +// Returns an empty string if the process is not in a container
> 
> The code seems to return a null string in this case, not an empty string.
> 
> We normally use a period at the end of these kinds of comments.

oh right we have empty vs null. Sigh.

Also re periods: i can't grammar

>> Source/WebKit2/UIProcess/WebContext.cpp:414
>> +    parameters.cookieStorageDirectory = (cookieStorageDirectory());
> 
> The extra parentheses here are a bit non-idiomatic. I’d leave them out.

Yeah, whoops left over cruft

>> Source/WebKit2/UIProcess/mac/WebContextMac.mm:276
>> +        path = [@"~/" stringByStandardizingPath];
> 
> Maybe use NSHomeDirectory() instead?

Hmmmm, i knew there was an api like that (couldn't remember the name), but everywhere else was the awful ~ thing. Will switch.
Comment 11 Oliver Hunt 2014-06-30 15:11:45 PDT
Created attachment 234100 [details]
Patch
Comment 12 Oliver Hunt 2014-06-30 15:54:53 PDT
Committed r170608: <http://trac.webkit.org/changeset/170608>
Comment 13 WebKit Commit Bot 2014-07-01 18:18:38 PDT
Re-opened since this is blocked by bug 134533
Comment 14 Oliver Hunt 2014-07-02 16:03:22 PDT
Committed r170733: <http://trac.webkit.org/changeset/170733>