commit b4d1d2223d7fed2bd88b06fa4e0f65936618d417
Author: Jed Davis <jld(a)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);