Restrict network process sandbox

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Restrict network process sandbox

[Oliver Hunt]

Created attachment 233931 [details]
Patch

[Sam Weinig]

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?

[Sam Weinig]

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.

[Oliver Hunt]

(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.

[Sam Weinig]

(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.

[Alexey Proskuryakov]

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).

[Oliver Hunt]

Comment on attachment 233931 [details]
Patch

I'm making a few changes that will require re-review (not just reformatting :) )

[Oliver Hunt]

Created attachment 234053 [details]
Patch

[Darin Adler]

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?

[Oliver Hunt]

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.

[Oliver Hunt]

Created attachment 234100 [details]
Patch

[Oliver Hunt]

Committed r170608: <http://trac.webkit.org/changeset/170608>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 134533

[Oliver Hunt]

Committed r170733: <http://trac.webkit.org/changeset/170733>