Per discussion with :baku on IRC, this is a follow up to bug 1344415 "Privilege escalation/Sandbox escape using PFileSystemRequestConstructor". The fix for 1344415 needs to do some additional checks on the paths received from content and used in FileSystemRequestParent::Initialize() before they are passed to FileSystemUtils::IsDescendantPath() to prevent the ".." trick. Specifically, in the parent, the file picker could be used to select say /Users/john/files-to-upload The child could later send a FileSystemRequestParent request for the path /Users/john/files-to-upload/../my-secret-files And FileSystemUtils::IsDescendantPath would consider the second path to be a descendant of the first which would allow the content process to access files that it should not be allowed to. The content process sandbox will soon block read access to arbitrary files in the home directory.

Ugh, I was afraid of this path parsing business. :( Thanks for catching this!

Comment on attachment 8849630 [details] [diff] [review] file2.patch Review of attachment 8849630 [details] [diff] [review]: ----------------------------------------------------------------- This needs a bit more work. I'm also wondering if we've solved this problem of checking if file A is a descendant of dir B in a reliable way already. I'll ask around about it. The nsIFile routines all use string prefix comparisons of the paths which has me concerned--I don't know all the tricks you can play with paths. It would be great if we had a way to do this that used filesystem system calls to do so without path prefix string comparisons. Not to be done in this bug, but I think it would be nice if we had a facility to do this that relied on OS-level sandboxing. ::: dom/filesystem/FileSystemRequestParent.cpp @@ +107,5 @@ > if (!mozilla::Preferences::GetBool("dom.filesystem.pathcheck.disabled", false)) { > + nsCOMPtr<nsIFile> file; > + nsresult rv = NS_NewLocalFile(mPath, true, getter_AddRefs(file)); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + mContentParent->KillHard("This path is not allowed."); Maybe add a comment that we're killing the process for any type of error returned from NS_NewLocalFile(). The message "This path is not allowed" made me think NS_NewLocalFile was doing some checks on the path, but it doesn't. ::: dom/filesystem/FileSystemSecurity.cpp @@ +61,1 @@ > { I think we should call nsIFile::normalize() on the incoming file here. FYI, on Mac, normalize() results in realpath() being called. From the realpath(3) man page: "The realpath() function resolves all symbolic links, extra ``/'' characters, and references to /./ and /../ in file_name." @@ +68,5 @@ > + nsresult rv = aDirectoryPath->IsDirectory(&isDir); > + MOZ_ASSERT(NS_SUCCEEDED(rv) && isDir); > +#endif > + > + nsTArray<nsCOMPtr<nsIFile>>* paths; I think it makes sense to rename paths, mPaths, and aDirectoryPath now that they're file objects and not strings. @@ +75,5 @@ > mPaths.Put(aId, paths); > + } else { > + for (uint32_t i = 0, len = paths->Length(); i < len; ++i) { > + bool equal = false; > + nsresult rv = paths->ElementAt(i)->Equals(aDirectoryPath, &equal); Just wondering if the nested |rv| definition causes a compiler warning for DEBUG builds. @@ +101,1 @@ > { We should call nsIFile::normalize() on the incoming file here too. @@ +108,5 @@ > } > > for (uint32_t i = 0, len = paths->Length(); i < len; ++i) { > + bool isDescendant = false; > + nsresult rv = paths->ElementAt(i)->Contains(aPath, &isDescendant); nsIFile::contains() relies on string comparisons so we're still vulnerable to the ".." tick without a call to nsIFile::normalize() beforehand. ::: dom/filesystem/FileSystemSecurity.h @@ +41,5 @@ > private: > FileSystemSecurity(); > ~FileSystemSecurity(); > > + nsClassHashtable<nsUint64HashKey, nsTArray<nsCOMPtr<nsIFile>>> mPaths; Rename mPaths now that it's not used to store paths.

Thanks for the review, but we cannot call 'Normalize' because it will kill the child process for each symlink found. We want to support symlinks in Entries API, but if we call nsIFile::Normalize() this will not happen. Think about the scenario where: 1. FilePicker shares /home/foo/a 2. /home/foo/a/bar is a link to /tmp/anotherFile 3. th comparison between /home/foo/a and /tmp/anotherFile will fail if done after ::Normalize(). Instead, what about if we simply check if the paths contain '..'?

(In reply to Andrea Marchesini [:baku] from comment #4) > Thanks for the review, but we cannot call 'Normalize' because it will kill > the child process for each symlink found. > We want to support symlinks in Entries API, but if we call > nsIFile::Normalize() this will not happen. Think about the scenario where: > > 1. FilePicker shares /home/foo/a > 2. /home/foo/a/bar is a link to /tmp/anotherFile > 3. th comparison between /home/foo/a and /tmp/anotherFile will fail if done > after ::Normalize(). Ah, I see. > Instead, what about if we simply check if the paths contain '..'? Yeah, maybe that would be enough. Will have to look into this a bit more. I know the significance of '..' for Linux/UNIX/Mac, but don't know if Windows has the same or other path special cases.

Yes, also windows has the same '..' behavior. I'll submit a new patch with this approach.

Comment on attachment 8850499 [details] [diff] [review] file1.patch r+ One idea we had in our sandboxing standup today was that, since all directory listings are done in the parent, the parent could send an identifier to the child with each filesystem entry. Then, when the child wants to request access, instead of sending a path back to the parent which has to be validated, it would send the identifier back. The parent would have to maintain a set of mappings of identifier->path.

> One idea we had in our sandboxing standup today was that, since all This is a good idea. But instead sending back just the identifier, we need to send back the identifier + the relative path, because, for how the Entries API works, if the user shares '/home/foo', content can ask for '/home/foo/b/a/r'. This is totally allowed.

Comment on attachment 8850499 [details] [diff] [review] file1.patch This is a follow up of bug 1344415. See https://bugzilla.mozilla.org/show_bug.cgi?id=1344415#c39 for more details. Approval Request Comment [Feature/Bug causing the regression]: Bug 1332003 [User impact if declined]: In theory, the content process could send a IPC message in order to have read-only access to any existing file. [Is this code covered by automated tests?]: yes. [Has the fix been verified in Nightly?]: not yet. [Needs manual test from QE? If yes, steps to reproduce]: none [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: Here we check if the path contains '..'. No risks. [String changes made/needed]: none

Comment on attachment 8850499 [details] [diff] [review] file1.patch sec-approval+. We'll want this on any affected branches as well.

Comment on attachment 8850499 [details] [diff] [review] file1.patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1332003 [User impact if declined]: In theory, the content process could send a IPC message in order to have read-only access to any existing file. [Is this code covered by automated tests?]: yes. [Has the fix been verified in Nightly?]: not yet. [Needs manual test from QE? If yes, steps to reproduce]: none [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: No, we just check for '..' in strings. [String changes made/needed]: none

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=86581040&repo=mozilla-inbound

Comment on attachment 8850499 [details] [diff] [review] file1.patch Fix a sec-high. Aurora54+ & Beta53+.

https://hg.mozilla.org/releases/mozilla-aurora/rev/42a43de3d64f https://hg.mozilla.org/releases/mozilla-beta/rev/0c03035155c5

Comment on attachment 8850499 [details] [diff] [review] file1.patch sandboxing fix for esr52

Setting qe-verify- based on Andrea's assessment on manual testing needs (see Comment 13) and the fact that this fix has automated coverage.