Bug 134360

Summary: Restrict network process sandbox
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bunhere, cdumez, commit-queue, gyuyoung.kim, sam, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 134533    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch sam: review+

Oliver Hunt
Reported 2014-06-26 13:43:01 PDT
Restrict network process sandbox
Attachments
Patch (4.82 KB, patch)
2014-06-26 13:54 PDT, Oliver Hunt
no flags
Patch (11.97 KB, patch)
2014-06-28 14:44 PDT, Oliver Hunt
no flags
Patch (12.04 KB, patch)
2014-06-30 15:11 PDT, Oliver Hunt
sam: review+
Oliver Hunt
Comment 1 2014-06-26 13:54:58 PDT
Sam Weinig
Comment 2 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?
Sam Weinig
Comment 3 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.
Oliver Hunt
Comment 4 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.
Sam Weinig
Comment 5 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.
Alexey Proskuryakov
Comment 6 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).
Oliver Hunt
Comment 7 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 :) )
Oliver Hunt
Comment 8 2014-06-28 14:44:36 PDT
Darin Adler
Comment 9 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?
Oliver Hunt
Comment 10 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.
Oliver Hunt
Comment 11 2014-06-30 15:11:45 PDT
Oliver Hunt
Comment 12 2014-06-30 15:54:53 PDT
WebKit Commit Bot
Comment 13 2014-07-01 18:18:38 PDT
Re-opened since this is blocked by bug 134533
Oliver Hunt
Comment 14 2014-07-02 16:03:22 PDT
Note You need to log in before you can comment on or make changes to this bug.