Extend SandboxBroker to allow adding paths

RESOLVED FIXED in Firefox 51

Status

()

Core
Security: Process Sandboxing
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

(Whiteboard: sblc2)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
Currently SandboxBroker allows adding directories, but it is really walking the directory tree at policy creation time and adding all files individually.

This worked for B2G with a static filesystem, but for desktop it's going to be insufficient.

We'll need to whitelist things like /tmp, /usr/lib, /usr/share because of the amount of third party stuff we load into the content process.
(Assignee)

Updated

a year ago
Assignee: nobody → gpascutto
(Assignee)

Updated

a year ago
Whiteboard: sblc2
(Assignee)

Comment 1

a year ago
Created attachment 8774451 [details]
Bug 1288410 - Basic implementation of AddDir and recursive Lookup.

Review commit: https://reviewboard.mozilla.org/r/66904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66904/
https://reviewboard.mozilla.org/r/66904/#review64108

::: security/sandbox/linux/broker/SandboxBroker.cpp:194
(Diff revision 1)
> +  if (stat(aPath, &statBuf) != 0) {
> +    return;
> +  }
> +
> +  if (!S_ISDIR(statBuf.st_mode)) {
> +    return;
> +  }

Does it makes sense to require the directory exists before it is added to the policy? I'm thinking there may be scenarios where the directory isn't created yet, but will be.

::: security/sandbox/linux/broker/SandboxBroker.cpp:210
(Diff revision 1)
> +  if (!mMap.Get(path, &origPerms)) {
> +    origPerms = MAY_ACCESS;
> +  } else {
> +    MOZ_ASSERT(origPerms & MAY_ACCESS);
> +  }
> +  int newPerms = origPerms | aPerms | RECURSIVE;

Why is RECURSIVE always included here?

::: security/sandbox/linux/broker/SandboxBroker.cpp:266
(Diff revision 1)
> +    // passed part starts with something on the whitelist
> +    if (FindInReadable(aPath, whiteListPath) == 0) {
> +      allPerms |= perms;
> +    }
> +  }

We wouldn't want to allow /xyz/foo/bar even if it exists in whiteListPath=/safe/xyz/foo/bar/blah and it seems like that's what we're doing here.

I think we need to start with aPath and then recursively strip off the deepest directory or filename and see if is in the map. i.e., repeatedly strip off the deepest filename or dirname and check the map until we run out of chars in aPath. As in check /xyz/foo/bar, then /xyz/foo, then /xyz, then / until we hit a match or deplete aPath.
(Assignee)

Comment 3

a year ago
Comment on attachment 8774451 [details]
Bug 1288410 - Basic implementation of AddDir and recursive Lookup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66904/diff/1-2/
(Assignee)

Comment 4

a year ago
(In reply to Haik Aftandilian [:haik] from comment #2)
> Does it makes sense to require the directory exists before it is added to
> the policy? I'm thinking there may be scenarios where the directory isn't
> created yet, but will be.

I'm not sure. Giving recursive permission on a directory that doesn't exist
yet sounds strange, I think in most cases (that we've seen?) you would grant 
the permission on the parent.

> > +  int newPerms = origPerms | aPerms | RECURSIVE;
> 
> Why is RECURSIVE always included here?

Because setting that flag (applying the permission recursively) is the point of AddDir, 
when compared to AddTree.

> ::: security/sandbox/linux/broker/SandboxBroker.cpp:266
> (Diff revision 1)
> > +    // passed part starts with something on the whitelist
> > +    if (FindInReadable(aPath, whiteListPath) == 0) {
> > +      allPerms |= perms;
> > +    }
> > +  }
> 
> We wouldn't want to allow /xyz/foo/bar even if it exists in
> whiteListPath=/safe/xyz/foo/bar/blah and it seems like that's what we're
> doing here.
> 
> I think we need to start with aPath and then recursively strip off the
> deepest directory or filename and see if is in the map. i.e., repeatedly
> strip off the deepest filename or dirname and check the map until we run out
> of chars in aPath. As in check /xyz/foo/bar, then /xyz/foo, then /xyz, then
> / until we hit a match or deplete aPath.

This was a bug because the semantics of FindInReadable() are entirely different
compared to Find(). The new patch uses StringBeginsWith() which matches Find(), and
which was intended (i.e. the == 0 was supposed to fix the match to the beginning 
of the string, not compare against true/false).
(Assignee)

Comment 5

a year ago
Created attachment 8777824 [details]
Enable sandbox file broker and log all the things.

Review commit: https://reviewboard.mozilla.org/r/67232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67232/
Attachment #8774451 - Flags: review?(julian.r.hector)
Comment on attachment 8774451 [details]
Bug 1288410 - Basic implementation of AddDir and recursive Lookup.

https://reviewboard.mozilla.org/r/66904/#review66674

See comments...

::: security/sandbox/linux/broker/SandboxBroker.cpp:119
(Diff revision 2)
> +  if (path[0] != '/')
> +    return false;
> +  // No trailing / (but "/" is valid)
> +  if (len > 1 && path[len - 1] == '/')
> +    return false;
> +  // No trailing /..

A trailing '/.' should not be allowed as well.

Requesting access to '/tmp/good/.' also gives a directory file descriptor (see other comment why we don't want this)

::: security/sandbox/linux/broker/SandboxBroker.cpp:214
(Diff revision 2)
> +  }
> +  int newPerms = origPerms | aPerms | RECURSIVE;
> +  if (SandboxInfo::Get().Test(SandboxInfo::kVerbose)) {
> +    SANDBOX_LOG_ERROR("policy for %s: %d -> %d", aPath, origPerms, newPerms);
> +  }
> +  mMap.Put(path, newPerms);

A trailing '/' for |path| should be enforced.

Otherwise this opens up two problems:
First, adding permissions for a directory like AddDir(MAY_READ, "/tmp/good"); (note the missing of the trailing slash) will automatically give permissions for everything in /tmp/ that starts with "good".

So a file like "/tmp/good_but_not_really/key.txt" will be accessible even though the directory /tmp/good_but_not_really/ is not whitelisted.

Second, not enforcing a trailing slash would allow the client to retreive a file descriptor of a directory. Because it can request access to "/tmp/good" and will actually pass the ValidatePath() function (which forbids trailing slashes). With an open directory file descriptor, the client may be able to do a directory traversal (see https://crbug.com/43304, thanks :jld for the link)
Attachment #8774451 - Flags: review?(julian.r.hector) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8777824 - Attachment is obsolete: true

Comment 8

a year ago
mozreview-review
Comment on attachment 8774451 [details]
Bug 1288410 - Basic implementation of AddDir and recursive Lookup.

https://reviewboard.mozilla.org/r/66904/#review67690

Thanks for making the changes. lgtm now.
Attachment #8774451 - Flags: review?(julian.r.hector) → review+
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9f5b652c837
Basic implementation of AddDir and recursive Lookup. r=tedd

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9f5b652c837
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.