Bug 1335329 - Improve handling of mkdir() on preexisting directories in Linux sandbox file broker. r=gcp
authorJed Davis <jld@mozilla.com>
Thu, 02 Feb 2017 11:56:21 -0700
changeset 341034 53023771039ea8c799ac1b5a382f987066aeebfe
parent 341033 9a087f3a376703d2b5d0eb7dd22dcdf748fe49f5
child 341035 22651b5d537d8f6d0c09a2148f4c1017ab0a368e
push id86615
push userkwierso@gmail.com
push dateTue, 07 Feb 2017 01:52:08 +0000
treeherdermozilla-inbound@f0453084d86e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgcp
bugs1335329
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1335329 - Improve handling of mkdir() on preexisting directories in Linux sandbox file broker. r=gcp If the path given doesn't have write+create permissions in the broker policy, but does have MAY_ACCESS (i.e., if checking for its existence with lstat() or access() would be allowed), then check for its existence and fail with EEXIST the way the the real mkdir() would. Note that mkdir() fails with EEXIST even the existing file isn't a directory, including if it's a broken symlink. MozReview-Commit-ID: 13Cwnq1nRrw
security/sandbox/linux/broker/SandboxBroker.cpp
security/sandbox/linux/gtest/TestBroker.cpp
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -668,17 +668,24 @@ SandboxBroker::ThreadMain(void)
       case SANDBOX_FILE_MKDIR:
         if (permissive || AllowOperation(W_OK | X_OK, perms)) {
           if (mkdir(pathBuf, req.mFlags) == 0) {
             resp.mError = 0;
           } else {
             resp.mError = -errno;
           }
         } else {
-          AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
+          struct stat sb;
+          // This doesn't need an additional policy check because
+          // MAY_ACCESS is required to even enter this switch statement.
+          if (lstat(pathBuf, &sb) == 0) {
+            resp.mError = -EEXIST;
+          } else {
+            AuditDenial(req.mOp, req.mFlags, perms, pathBuf);
+          }
         }
         break;
 
       case SANDBOX_FILE_UNLINK:
         if (permissive || AllowOperation(W_OK, perms)) {
           if (unlink(pathBuf) == 0) {
             resp.mError = 0;
           } else {
--- a/security/sandbox/linux/gtest/TestBroker.cpp
+++ b/security/sandbox/linux/gtest/TestBroker.cpp
@@ -316,16 +316,20 @@ TEST_F(SandboxBrokerTest, Mkdir)
 
   ASSERT_EQ(0, mkdir("/tmp/blublu", 0600))
     << "Creating dir /tmp/blublu failed.";
   EXPECT_EQ(0, Access("/tmp/blublu", F_OK));
   // Not whitelisted target path
   EXPECT_EQ(-EACCES, Mkdir("/tmp/nope", 0600))
     << "Creating dir without MAY_CREATE succeed.";
   EXPECT_EQ(0, rmdir("/tmp/blublu"));
+  EXPECT_EQ(-EEXIST, Mkdir("/proc/self", 0600))
+    << "Creating uncreatable dir that already exists didn't fail correctly.";
+  EXPECT_EQ(-EEXIST, Mkdir("/dev/zero", 0600))
+    << "Creating uncreatable dir over preexisting file didn't fail correctly.";
 
   PrePostTestCleanup();
 }
 
 TEST_F(SandboxBrokerTest, Rename)
 {
   PrePostTestCleanup();