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

@@ -343,7 +343,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;
@@ -662,17 +662,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;

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);