New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2010
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug-Security

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Mac sandbox allows calls to stat() on arbitrary paths.

Project Member Reported by dcheng@chromium.org, May 1 2010

Issue description

In the renderer, set a breakpoint at 'WebCore::DragController::dragOperation'. Step into 
containsURL()->asURL()->hasValidURL(), and change the return value to false. Step until 
file_util::PathExists is called.

On Windows:
GetFileAttributes() fails with ERROR_ACCESS_DENIED.

On OS X:
stat64() returns 0.

I would expect it to fail uniformly in the sandbox or succeed uniformly.
 

Comment 1 by mark@chromium.org, May 3 2010

Labels: Mstone-6
Status: Assigned
Jeremy, can you make sure the sandbox is defending us against the stat call? I don't 
think it's feasible to match the exact failure mode, which will make this a WontFix 
provided the sandbox is blocking fs access.
I tried a simple test by adding a few lines to renderer_main.cc:
CHECK(!file_util::PathExists("/etc/passwd"));
CHECK(file_util::PathExists("/truffulatrees"));

The renderer will crash with either line in renderer_main.cc, so it seems like the OS X 
sandbox is allowing limited filesystem access.
This is the consequence of the (allow file-read-metadata) rule in src/chrome/common.sb .

This rule is needed so we can traverse symlinks (needed by system libraries).

Please let me know how much of an issue this is and I'll see if I can tighten this down any.

Test program:
#include <sandbox.h>
#include <stdio.h>
#include <sys/stat.h>

int main(int argc, const char * argv[]) {
  char* err;
  sandbox_init(
      "(version 1)(deny default)(allow file-read-metadata)(debug deny)",
      0,
      &err);
  if (err != NULL) {
    printf(err);
    sandbox_free_error(err);
  }

  struct stat foo;
  int ret = stat("/etc/passwd", &foo);
  printf("stat /etc/passwd returned %d %d\n", ret, S_ISREG(foo.st_mode));
  return 0; 
}
So just to be clear, the Mac sandbox currently **allows** the stat call.
FYI: This can be tightened down by changing the rule to something like:
(allow file-read-metadata (regex "<something>"))
I took some time looking at this today.  There are a bunch of places where this trips up the OS i.e. not a quick fix.

Could you advise in terms of priority?

Comment 7 by jsc...@chromium.org, May 17 2010

I'm trying to determine the security impact of this. It appears that a compromised OS X 
renderer can read attributes (size, date, owner, etc.) of files outside the sandbox. 
However, it doesn't sound like the data itself can be read. Is that correct? Also, why 
is this flagged as affecting Windows?

Comment 8 by dcheng@google.com, May 17 2010

Labels: -OS-Windows
It's an inconsistency between Windows and Mac (POSIX?) platforms. When I filed the 
issue, I wasn't sure if it was intentionally disallowed on Windows, as the renderer was 
trying to do these stat calls, so I flagged it for both Windows and Mac.

I'm trying to remove these calls from WebKit now though, so I guess it doesn't make 
sense to keep the Windows flag anymore.

Comment 9 by jsc...@chromium.org, May 17 2010

Labels: SecSeverity-Low
Okay, sounds like a low severity issue then.
Labels: -Mstone-6 Mstone-X
Labels: -Mstone-X Mstone-7
While not an ideal solution changing the line:

(allow file-read-metadata)  ; 10.5.6

to these 2 lines causes Chrome to perform correctly on 10.6.3:

(allow file-read-metadata (regex #"^/System($|/)"))  ; 10.5.6
(allow file-read-metadata (regex #"^/Library($|/)"))  ; 10.6
I forgot to mention it also cause the stat and other system calls to perform as desired as well.
Summary: Mac sandbox allows calls to stat() on arbitrary paths.
Labels: Restrict-View-SecurityTeam
Labels: -Mstone-7 Mstone-8
The fix for this is going to need some bake time on the dev channel so this isn't going to make it into mstone:7
@jeremy - It sounds like this has been fixed. If so, please close it out. Otherwise, can we get an estimate on when it will be fixed?
Labels: -Mstone-8 Mstone-9
jschuh: no, sadly this isn't fixed yet.
Punting to mstone:9 per laforge's mail.
Status: Started
Labels: -Mstone-9 Mstone-10
Moving P2s with an owner to Mstone 10, bring back to M9 if you think it's important and you don't have higher priority work.
Status: Fixed
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=63884

------------------------------------------------------------------------
r63884 | jeremy@chromium.org | Tue Oct 26 06:41:39 PDT 2010

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/utility.sb?r1=63884&r2=63883&pathrev=63884
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/sandbox_mac_diraccess_unittest.mm?r1=63884&r2=63883&pathrev=63884
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/sandbox_mac.mm?r1=63884&r2=63883&pathrev=63884
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/renderer.sb?r1=63884&r2=63883&pathrev=63884
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/common.sb?r1=63884&r2=63883&pathrev=63884

Mac: block ability to stat arbitrary files in the Sandbox.

This change removes the ability to stat any file on disk and instead only allows stating files to which we have read access.

The complication with removing the ability to stat an arbitrary path is that without extra work you get into a situation where you can stat a leaf directory, but not it's parent. e.g. stat("/foo/bar") succeeds while stat("/foo") fails with errno == EPERM.

The only place we currently run into this is the utility process where the file system is off limits except for one directory.

This causes problems in 2 places:
1) DirectoryExists() works it's way from / down to the leaf directory stating each directory as it goes.
2) The extension installation code calls realpath() which fails if it can't stat parent directories.

The fix for the above is to explictly allow stating parent directories.  We achieve this in the sandbox code by adding a function which generates the appropriate sandbox syntax.

This CL also contains unit tests for the above functionality and re-enables it [ bug 56765 , the underlying issue appears to be unrelated to the test and previously fixed].

BUG= 42989 ,  56765 
TEST=Chrome should continue to render web pages correctly, installing extensions and themes should continue to work on OS X.

Review URL: http://codereview.chromium.org/4044002
------------------------------------------------------------------------
Labels: Verifier-Deepakg
Verified label updated by AutoAllocator, contact AmolK or KrisR for details
Status: FixUnreleased
Thanks Jeremy. Given that this is low severity we won't merge and will just pick it up in the m9 cycle.
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Type-Security
Labels: SecImpacts-Stable
Batch update.
Labels: -Restrict-View-SecurityNotify
Lifting view restrictions.
Status: Fixed
Project Member

Comment 31 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 32 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-Internals -SecSeverity-Low -Mstone-10 -Type-Security -SecImpacts-Stable Security-Severity-Low Security-Impact-Stable Cr-Internals M-10 Type-Bug-Security
Project Member

Comment 33 by bugdroid1@chromium.org, Mar 13 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Project Member

Comment 34 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-Low Security_Severity-Low
Project Member

Comment 35 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Labels: hasTestcase
Project Member

Comment 37 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 38 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic

Sign in to add a comment