Commit 31227250 authored by Jed Davis's avatar Jed Davis Committed by Georg Koppen
Browse files

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
parent 7668da91

security/sandbox/linux/broker/SandboxBroker.cpp

+2 −12
Original line number Diff line number Diff line
@@ -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;

security/sandbox/linux/gtest/TestBroker.cpp

+11 −4
Original line number Diff line number Diff line
@@ -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);