[tbb-commits] [tor-browser/tor-browser-52.3.0esr-7.5-2] Bug 1386279 - Renovate Linux sandbox file broker handling of access(). r=gcp

[gk at torproject.org]

commit 018cc6d1fd6751a21bb46aa1b9afc7ca96a42c8c
Author: Jed Davis <jld at mozilla.com>
Date:   Tue Aug 8 18:02:31 2017 -0600

    Bug 1386279 - Renovate Linux sandbox file broker handling of access(). r=gcp
    
    1. X_OK is now allowed, and is limited only by the MAY_ACCESS permission.
    
    2. The actual access() syscall is now used, if access is granted by the
    broker policy.  This fixed bug 1382246, which explains the background.
    
    MozReview-Commit-ID: 926429PlBnL
    
    --HG--
    extra : rebase_source : 6ae54c4c25e1389fa3af75b0bdf727323448294a
---
 security/sandbox/linux/broker/SandboxBroker.cpp | 14 ++------------
 security/sandbox/linux/gtest/TestBroker.cpp     | 15 +++++++++++----
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/security/sandbox/linux/broker/SandboxBroker.cpp b/security/sandbox/linux/broker/SandboxBroker.cpp
index d79656c3407f..56215b89b232 100644
--- a/security/sandbox/linux/broker/SandboxBroker.cpp
+++ b/security/sandbox/linux/broker/SandboxBroker.cpp
@@ -306,7 +306,7 @@ AllowOperation(int aReqFlags, int aPerms)
 static bool
 AllowAccess(int aReqFlags, int aPerms)
 {
-  if (aReqFlags & ~(R_OK|W_OK|F_OK)) {
+  if (aReqFlags & ~(R_OK|W_OK|X_OK|F_OK)) {
     return false;
   }
   int needed = 0;
@@ -569,17 +569,7 @@ SandboxBroker::ThreadMain(void)
 
       case SANDBOX_FILE_ACCESS:
         if (permissive || AllowAccess(req.mFlags, perms)) {
-          // This can't use access() itself because that uses the ruid
-          // and not the euid.  In theory faccessat() with AT_EACCESS
-          // would work, but Linux doesn't actually implement the
-          // flags != 0 case; glibc has a hack which doesn't even work
-          // in this case so it'll ignore the flag, and Bionic just
-          // passes through the syscall and always ignores the flags.
-          //
-          // Instead, because we've already checked the requested
-          // r/w/x bits against the policy, just return success if the
-          // file exists and hope that's close enough.
-          if (stat(pathBuf, (struct stat*)&respBuf) == 0) {
+          if (access(pathBuf, req.mFlags) == 0) {
             resp.mError = 0;
           } else {
             resp.mError = -errno;
diff --git a/security/sandbox/linux/gtest/TestBroker.cpp b/security/sandbox/linux/gtest/TestBroker.cpp
index a311e181a9b1..bf5e58acbc40 100644
--- a/security/sandbox/linux/gtest/TestBroker.cpp
+++ b/security/sandbox/linux/gtest/TestBroker.cpp
@@ -133,6 +133,8 @@ SandboxBrokerTest::GetPolicy() const
   policy->AddPath(MAY_READ | MAY_WRITE, "/tmp", AddAlways);
   policy->AddPath(MAY_READ | MAY_WRITE | MAY_CREATE, "/tmp/blublu", AddAlways);
   policy->AddPath(MAY_READ | MAY_WRITE | MAY_CREATE, "/tmp/blublublu", AddAlways);
+  // This should be non-writable by the user running the test:
+  policy->AddPath(MAY_READ | MAY_WRITE, "/etc", AddAlways);
 
   return Move(policy);
 }
@@ -206,6 +208,14 @@ TEST_F(SandboxBrokerTest, Access)
   EXPECT_EQ(-EACCES, Access("/proc/self", R_OK));
 
   EXPECT_EQ(-EACCES, Access("/proc/self/stat", F_OK));
+
+  EXPECT_EQ(0, Access("/tmp", X_OK));
+  EXPECT_EQ(0, Access("/tmp", R_OK|X_OK));
+  EXPECT_EQ(0, Access("/tmp", R_OK|W_OK|X_OK));
+  EXPECT_EQ(0, Access("/proc/self", X_OK));
+
+  EXPECT_EQ(0, Access("/etc", R_OK|X_OK));
+  EXPECT_EQ(-EACCES, Access("/etc", W_OK));
 }
 
 TEST_F(SandboxBrokerTest, Stat)
@@ -257,10 +267,7 @@ TEST_F(SandboxBrokerTest, Chmod)
   close(fd);
   // Set read only. SandboxBroker enforces 0600 mode flags.
   ASSERT_EQ(0, Chmod("/tmp/blublu", S_IRUSR));
-  // SandboxBroker doesn't use real access(), it just checks against
-  // the policy. So it can't see the change in permisions here.
-  // This won't work:
-  // EXPECT_EQ(-EACCES, Access("/tmp/blublu", W_OK));
+  EXPECT_EQ(-EACCES, Access("/tmp/blublu", W_OK));
   statstruct realStat;
   EXPECT_EQ(0, statsyscall("/tmp/blublu", &realStat));
   EXPECT_EQ((mode_t)S_IRUSR, realStat.st_mode & 0777);