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

Issue 43304 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2010
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security
M-5

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

[MD audit] Linux sandbox escape

Reported by scarybea...@gmail.com, May 5 2010

Issue description

Mark Dowd noted that there's a sandbox escape on the Linux build (and 
likely all OS_POSIX systems).

It is caused by the interaction of the renderer message 
ViewHostMsg_DatabaseOpenFile and chroot()-based sandboxing. In OS_POSIX-
based systems, the browser sends a directory file descriptor over the 
browser<->renderer IPC channel. Unfortunately, this enables easy escaping 
of the sandbox via:
fchdir(dir_fd);
chdir("..")

I confirmed the escape by editing chrome/common/database_util.cc to include 
this modified chunk:

...
#elif defined(OS_POSIX)
  if (dir_handle) {
    int fd = dir_handle_rv.fd;
    fchdir(fd);
    for (int i = 0; i < 100; ++i) chdir("..");
    chdir("tmp");
    creat("chrome.0wned", 0600);
    *dir_handle = dir_handle_rv.fd;
  }
...

And then when accessing an HTML5 web db (e.g. http://cevans-
app.appspot.com/static/webdb.html; then type "select * from pants") there 
is indeed a /tmp/chrome.0wned file created.

(I was careful to make sure I ran with the sandbox; I set:
export CHROME_DEVEL_SANDBOX=/opt/google/chrome/chrome-sandbox
and used /proc to verify the root of the renderer process was in a chroot.
 
Adam, Evan, any interest in tackling? Seems like we want to nail this one before 
stable.

Comment 2 by evan@chromium.org, May 5 2010

+database man

Comment 3 by agl@chromium.org, May 5 2010

Status: Started
Dumi is actually DB man.

Comment 5 by agl@chromium.org, May 5 2010

After tracing this file descriptor for a dozen or more functions, it appears that it's 
only used to fsync the directory in third_party/sqlite/src/os_unix.c. That use is 
already guarded by a test for dirfd >= 0, so why don't we just return -1 in the first 
place?

Comment 6 by agl@chromium.org, May 5 2010

Indeed, http://webkit.org/demos/sticky-notes/index.html seems to work just fine 
without the directory fd.

dumi: any reason not to just remove this?


diff --git a/chrome/browser/renderer_host/database_dispatcher_host.cc 
b/chrome/browser/renderer_host/data
index 201b5c1..1e86f1b 100644
--- a/chrome/browser/renderer_host/database_dispatcher_host.cc
+++ b/chrome/browser/renderer_host/database_dispatcher_host.cc
@@ -176,6 +176,9 @@ void DatabaseDispatcherHost::DatabaseOpenFile(const string16& 
vfs_file_name,
       }
   }
 
+  close(target_dir_handle);
+  target_dir_handle = -1;
+
   ViewHostMsg_DatabaseOpenFile::WriteReplyParams(
       reply_msg,
 #if defined(OS_WIN)

Comment 7 by dumi@chromium.org, May 6 2010

Sorry I haven't responded to this earlier...

Looking at the sqlite code, it seems to me that we need dirfd only if sqlite was 
compiled with SQLITE_NO_SYNC. I don't think we do that (I don't see this option in 
third_party/sqlite/sqlite.gyp), so it should be OK to get rid of this file 
descriptor.

I'll take care of that, but as a short term fix, I think Adam's patch works. Adam: 
before you submit/merge it, can you please run all LayoutTests/storage/ tests in 
test_shell? storage/open-database-over-quota.html should be the only one that fails.

Comment 8 by agl@chromium.org, May 7 2010

Change submitted (without any links to this bug or indications)

http://src.chromium.org/viewvc/chrome?view=rev&revision=46700
Labels: MergeNeeded
Status: FixUnreleased
Ok. I can handle the merge to 375 once this has seen light of day in a dev channel.
Thanks!
Labels: OS-Linux
Labels: -MergeNeeded NeedsMerge
Labels: -Pri-0 Pri-1
Merge 46700 - Don't bother returning the directory descriptor for SQLite.

(Reviewed by dumi.)

TBR=agl@chromium.org
BUG= 43304 
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48057
Labels: -NeedsMerge
Adding Willchan's comments from merge

willchan   	
Happened to see this diff as it scrolled by. Doesn't close() have to be
HANDLE_EINTR() ...


Comment 15 by agl@chromium.org, May 24 2010

Although technically close could return EINTR, it's actually impossible on Linux. So 
technically it should have HANDLE_EINTR wrapped around it, it doesn't matter. I'll fix 
it on trunk.

Comment 16 by agl@chromium.org, May 24 2010

Actually, I won't fix it on trunk because it's been refactored already and the close 
is gone.
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=48057 

------------------------------------------------------------------------
r48057 | inferno@chromium.org | 2010-05-24 11:16:26 -0700 (Mon, 24 May 2010) | 8 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/375/src/chrome/browser/renderer_host/database_dispatcher_host.cc?r1=48057&r2=48056

Merge 46700 - Don't bother returning the directory descriptor for SQLite.

(Reviewed by dumi.)

TBR=agl@chromium.org
BUG= 43304 

Review URL: http://codereview.chromium.org/2166003
------------------------------------------------------------------------

Comment 18 by dumi@chromium.org, May 25 2010

http://code.google.com/p/chromium/issues/detail?id=43489 has the links to all CLs that 
removed the dirfd parameter from everywhere in WebKit and Chromium.
Labels: -Restrict-View-SecurityTeam
Status: Fixed
Fixed in 5.0.373.70
Labels: Type-Security
Labels: SecImpacts-Stable
Batch update.
Project Member

Comment 22 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 23 by bugdroid1@chromium.org, Mar 10 2013

Labels: -SecSeverity-High -Mstone-5 -Type-Security -SecImpacts-Stable M-5 Security-Impact-Stable Security-Severity-High Type-Bug-Security
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 11 2013

Labels: -Area-Undefined
Project Member

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

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

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

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

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

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

Comment 28 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 29 by sheriffbot@chromium.org, Oct 1 2016

Labels: Restrict-View-SecurityNotify
Project Member

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

Labels: -Restrict-View-SecurityNotify
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