1349276 - (CVE-2017-5454) Paths received by FileSystemRequestParent need to be sanitized before passed to IsDescendantPath

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: file2.patch

Comment 2

[(no longer active)]

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

Comment 3

[Haik Aftandilian [:haik]]

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.

Comment 4

[Andrea Marchesini [:baku]]

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 '..'?

Comment 5

[Haik Aftandilian [:haik]]

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

Comment 6

[Andrea Marchesini [:baku]]

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

Comment 7

[Andrea Marchesini [:baku]]

Attachment: file1.patch

https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileWin.cpp#1480

Comment 8

[Andrea Marchesini [:baku]]

Attachment: file1.patch

Comment 9

[Haik Aftandilian [:haik]]

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.

Comment 10

[Andrea Marchesini [:baku]]

> 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 11

[Andrea Marchesini [:baku]]

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 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8850499 [details] [diff] [review]
file1.patch

sec-approval+.
We'll want this on any affected branches as well.

Comment 13

[Andrea Marchesini [:baku]]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a65b7088f02e

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8850499 [details] [diff] [review]
file1.patch

Fix a sec-high. Aurora54+ & Beta53+.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8850499 [details] [diff] [review]
file1.patch

sandboxing fix for esr52

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/ac40d4a4e414

Comment 20

[Andrei Vaida [:avaida]]

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