Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1871)

Unified Diff: sandbox/linux/syscall_broker/broker_process_unittest.cc

Issue 761903003: Update from https://crrev.com/306655 (Closed) Base URL: git@github.com:domokit/mojo.git@master
Patch Set: Created 6 years ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « sandbox/linux/syscall_broker/broker_process.cc ('k') | sandbox/linux/tests/unit_tests.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: sandbox/linux/syscall_broker/broker_process_unittest.cc
diff --git a/sandbox/linux/syscall_broker/broker_process_unittest.cc b/sandbox/linux/syscall_broker/broker_process_unittest.cc
index 997667c36ad8a6b1b0dd78be1086054feb711e4c..ee284f210ab3f316c53459e3aa18699d3e2d8da5 100644
--- a/sandbox/linux/syscall_broker/broker_process_unittest.cc
+++ b/sandbox/linux/syscall_broker/broker_process_unittest.cc
@@ -54,11 +54,10 @@ bool NoOpCallback() {
} // namespace
TEST(BrokerProcess, CreateAndDestroy) {
- std::vector<std::string> read_whitelist;
- read_whitelist.push_back("/proc/cpuinfo");
+ std::vector<BrokerFilePermission> permissions;
+ permissions.push_back(BrokerFilePermission::ReadOnly("/proc/cpuinfo"));
- scoped_ptr<BrokerProcess> open_broker(
- new BrokerProcess(EPERM, read_whitelist, std::vector<std::string>()));
+ scoped_ptr<BrokerProcess> open_broker(new BrokerProcess(EPERM, permissions));
ASSERT_TRUE(open_broker->Init(base::Bind(&NoOpCallback)));
ASSERT_TRUE(TestUtils::CurrentProcessHasChildren());
@@ -68,8 +67,8 @@ TEST(BrokerProcess, CreateAndDestroy) {
}
TEST(BrokerProcess, TestOpenAccessNull) {
- const std::vector<std::string> empty;
- BrokerProcess open_broker(EPERM, empty, empty);
+ std::vector<BrokerFilePermission> empty;
+ BrokerProcess open_broker(EPERM, empty);
ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
int fd = open_broker.Open(NULL, O_RDONLY);
@@ -88,17 +87,14 @@ void TestOpenFilePerms(bool fast_check_in_client, int denied_errno) {
const char kRW_WhiteListed[] = "/proc/DOESNOTEXIST3";
const char k_NotWhitelisted[] = "/proc/DOESNOTEXIST4";
- std::vector<std::string> read_whitelist;
- read_whitelist.push_back(kR_WhiteListed);
- read_whitelist.push_back(kR_WhiteListedButDenied);
- read_whitelist.push_back(kRW_WhiteListed);
-
- std::vector<std::string> write_whitelist;
- write_whitelist.push_back(kW_WhiteListed);
- write_whitelist.push_back(kRW_WhiteListed);
+ std::vector<BrokerFilePermission> permissions;
+ permissions.push_back(BrokerFilePermission::ReadOnly(kR_WhiteListed));
+ permissions.push_back(
+ BrokerFilePermission::ReadOnly(kR_WhiteListedButDenied));
+ permissions.push_back(BrokerFilePermission::WriteOnly(kW_WhiteListed));
+ permissions.push_back(BrokerFilePermission::ReadWrite(kRW_WhiteListed));
- BrokerProcess open_broker(
- denied_errno, read_whitelist, write_whitelist, fast_check_in_client);
+ BrokerProcess open_broker(denied_errno, permissions, fast_check_in_client);
ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
int fd = -1;
@@ -243,13 +239,78 @@ TEST(BrokerProcess, OpenOpenFilePermsNoClientCheckNoEnt) {
// expected.
}
-void TestOpenCpuinfo(bool fast_check_in_client) {
+void TestBadPaths(bool fast_check_in_client) {
+ const char kFileCpuInfo[] = "/proc/cpuinfo";
+ const char kNotAbsPath[] = "proc/cpuinfo";
+ const char kDotDotStart[] = "/../proc/cpuinfo";
+ const char kDotDotMiddle[] = "/proc/self/../cpuinfo";
+ const char kDotDotEnd[] = "/proc/..";
+ const char kTrailingSlash[] = "/proc/";
+
+ std::vector<BrokerFilePermission> permissions;
+
+ permissions.push_back(BrokerFilePermission::ReadOnlyRecursive("/proc/"));
+ scoped_ptr<BrokerProcess> open_broker(
+ new BrokerProcess(EPERM, permissions, fast_check_in_client));
+ ASSERT_TRUE(open_broker->Init(base::Bind(&NoOpCallback)));
+ // Open cpuinfo via the broker.
+ int cpuinfo_fd = open_broker->Open(kFileCpuInfo, O_RDONLY);
+ base::ScopedFD cpuinfo_fd_closer(cpuinfo_fd);
+ ASSERT_GE(cpuinfo_fd, 0);
+
+ int fd = -1;
+ int can_access;
+
+ can_access = open_broker->Access(kNotAbsPath, R_OK);
+ ASSERT_EQ(can_access, -EPERM);
+ fd = open_broker->Open(kNotAbsPath, O_RDONLY);
+ ASSERT_EQ(fd, -EPERM);
+
+ can_access = open_broker->Access(kDotDotStart, R_OK);
+ ASSERT_EQ(can_access, -EPERM);
+ fd = open_broker->Open(kDotDotStart, O_RDONLY);
+ ASSERT_EQ(fd, -EPERM);
+
+ can_access = open_broker->Access(kDotDotMiddle, R_OK);
+ ASSERT_EQ(can_access, -EPERM);
+ fd = open_broker->Open(kDotDotMiddle, O_RDONLY);
+ ASSERT_EQ(fd, -EPERM);
+
+ can_access = open_broker->Access(kDotDotEnd, R_OK);
+ ASSERT_EQ(can_access, -EPERM);
+ fd = open_broker->Open(kDotDotEnd, O_RDONLY);
+ ASSERT_EQ(fd, -EPERM);
+
+ can_access = open_broker->Access(kTrailingSlash, R_OK);
+ ASSERT_EQ(can_access, -EPERM);
+ fd = open_broker->Open(kTrailingSlash, O_RDONLY);
+ ASSERT_EQ(fd, -EPERM);
+}
+
+TEST(BrokerProcess, BadPathsClientCheck) {
+ TestBadPaths(true /* fast_check_in_client */);
+ // Don't do anything here, so that ASSERT works in the subfunction as
+ // expected.
+}
+
+TEST(BrokerProcess, BadPathsNoClientCheck) {
+ TestBadPaths(false /* fast_check_in_client */);
+ // Don't do anything here, so that ASSERT works in the subfunction as
+ // expected.
+}
+
+void TestOpenCpuinfo(bool fast_check_in_client, bool recursive) {
const char kFileCpuInfo[] = "/proc/cpuinfo";
- std::vector<std::string> read_whitelist;
- read_whitelist.push_back(kFileCpuInfo);
+ const char kDirProc[] = "/proc/";
- scoped_ptr<BrokerProcess> open_broker(new BrokerProcess(
- EPERM, read_whitelist, std::vector<std::string>(), fast_check_in_client));
+ std::vector<BrokerFilePermission> permissions;
+ if (recursive)
+ permissions.push_back(BrokerFilePermission::ReadOnlyRecursive(kDirProc));
+ else
+ permissions.push_back(BrokerFilePermission::ReadOnly(kFileCpuInfo));
+
+ scoped_ptr<BrokerProcess> open_broker(
+ new BrokerProcess(EPERM, permissions, fast_check_in_client));
ASSERT_TRUE(open_broker->Init(base::Bind(&NoOpCallback)));
int fd = -1;
@@ -293,16 +354,28 @@ void TestOpenCpuinfo(bool fast_check_in_client) {
ASSERT_FALSE(TestUtils::CurrentProcessHasChildren());
}
-// Run the same thing twice. The second time, we make sure that no security
-// check is performed on the client.
+// Run this test 4 times. With and without the check in client
+// and using a recursive path.
TEST(BrokerProcess, OpenCpuinfoWithClientCheck) {
- TestOpenCpuinfo(true /* fast_check_in_client */);
+ TestOpenCpuinfo(true /* fast_check_in_client */, false /* not recursive */);
// Don't do anything here, so that ASSERT works in the subfunction as
// expected.
}
TEST(BrokerProcess, OpenCpuinfoNoClientCheck) {
- TestOpenCpuinfo(false /* fast_check_in_client */);
+ TestOpenCpuinfo(false /* fast_check_in_client */, false /* not recursive */);
+ // Don't do anything here, so that ASSERT works in the subfunction as
+ // expected.
+}
+
+TEST(BrokerProcess, OpenCpuinfoWithClientCheckRecursive) {
+ TestOpenCpuinfo(true /* fast_check_in_client */, true /* recursive */);
+ // Don't do anything here, so that ASSERT works in the subfunction as
+ // expected.
+}
+
+TEST(BrokerProcess, OpenCpuinfoNoClientCheckRecursive) {
+ TestOpenCpuinfo(false /* fast_check_in_client */, true /* recursive */);
// Don't do anything here, so that ASSERT works in the subfunction as
// expected.
}
@@ -311,10 +384,10 @@ TEST(BrokerProcess, OpenFileRW) {
ScopedTemporaryFile tempfile;
const char* tempfile_name = tempfile.full_file_name();
- std::vector<std::string> whitelist;
- whitelist.push_back(tempfile_name);
+ std::vector<BrokerFilePermission> permissions;
+ permissions.push_back(BrokerFilePermission::ReadWrite(tempfile_name));
- BrokerProcess open_broker(EPERM, whitelist, whitelist);
+ BrokerProcess open_broker(EPERM, permissions);
ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
// Check we can access that file with read or write.
@@ -344,13 +417,11 @@ TEST(BrokerProcess, OpenFileRW) {
// SANDBOX_TEST because the process could die with a SIGPIPE
// and we want this to happen in a subprocess.
SANDBOX_TEST(BrokerProcess, BrokerDied) {
- std::vector<std::string> read_whitelist;
- read_whitelist.push_back("/proc/cpuinfo");
+ const char kCpuInfo[] = "/proc/cpuinfo";
+ std::vector<BrokerFilePermission> permissions;
+ permissions.push_back(BrokerFilePermission::ReadOnly(kCpuInfo));
- BrokerProcess open_broker(EPERM,
- read_whitelist,
- std::vector<std::string>(),
- true /* fast_check_in_client */,
+ BrokerProcess open_broker(EPERM, permissions, true /* fast_check_in_client */,
true /* quiet_failures_for_tests */);
SANDBOX_ASSERT(open_broker.Init(base::Bind(&NoOpCallback)));
const pid_t broker_pid = open_broker.broker_pid();
@@ -366,16 +437,16 @@ SANDBOX_TEST(BrokerProcess, BrokerDied) {
SANDBOX_ASSERT(SIGKILL == process_info.si_status);
// Check that doing Open with a dead broker won't SIGPIPE us.
- SANDBOX_ASSERT(open_broker.Open("/proc/cpuinfo", O_RDONLY) == -ENOMEM);
- SANDBOX_ASSERT(open_broker.Access("/proc/cpuinfo", O_RDONLY) == -ENOMEM);
+ SANDBOX_ASSERT(open_broker.Open(kCpuInfo, O_RDONLY) == -ENOMEM);
+ SANDBOX_ASSERT(open_broker.Access(kCpuInfo, O_RDONLY) == -ENOMEM);
}
void TestOpenComplexFlags(bool fast_check_in_client) {
const char kCpuInfo[] = "/proc/cpuinfo";
- std::vector<std::string> whitelist;
- whitelist.push_back(kCpuInfo);
+ std::vector<BrokerFilePermission> permissions;
+ permissions.push_back(BrokerFilePermission::ReadOnly(kCpuInfo));
- BrokerProcess open_broker(EPERM, whitelist, whitelist, fast_check_in_client);
+ BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
// Test that we do the right thing for O_CLOEXEC and O_NONBLOCK.
int fd = -1;
@@ -454,10 +525,10 @@ SANDBOX_TEST_ALLOW_NOISE(BrokerProcess, RecvMsgDescriptorLeak) {
SANDBOX_ASSERT(0 == setrlimit(RLIMIT_NOFILE, &rlim));
static const char kCpuInfo[] = "/proc/cpuinfo";
- std::vector<std::string> read_whitelist;
- read_whitelist.push_back(kCpuInfo);
+ std::vector<BrokerFilePermission> permissions;
+ permissions.push_back(BrokerFilePermission::ReadOnly(kCpuInfo));
- BrokerProcess open_broker(EPERM, read_whitelist, std::vector<std::string>());
+ BrokerProcess open_broker(EPERM, permissions);
SANDBOX_ASSERT(open_broker.Init(base::Bind(&NoOpCallback)));
const int ipc_fd = BrokerProcessTestHelper::GetIPCDescriptor(&open_broker);
@@ -498,16 +569,15 @@ bool WaitForClosedPipeWriter(int reader, int timeout_in_ms) {
// Closing the broker client's IPC channel should terminate the broker
// process.
TEST(BrokerProcess, BrokerDiesOnClosedChannel) {
- std::vector<std::string> read_whitelist;
- read_whitelist.push_back("/proc/cpuinfo");
+ std::vector<BrokerFilePermission> permissions;
+ permissions.push_back(BrokerFilePermission::ReadOnly("/proc/cpuinfo"));
// Get the writing end of a pipe into the broker (child) process so
// that we can reliably detect when it dies.
int lifeline_fds[2];
PCHECK(0 == pipe(lifeline_fds));
- BrokerProcess open_broker(EPERM, read_whitelist, std::vector<std::string>(),
- true /* fast_check_in_client */,
+ BrokerProcess open_broker(EPERM, permissions, true /* fast_check_in_client */,
false /* quiet_failures_for_tests */);
ASSERT_TRUE(open_broker.Init(base::Bind(&CloseFD, lifeline_fds[0])));
// Make sure the writing end only exists in the broker process.
@@ -533,6 +603,55 @@ TEST(BrokerProcess, BrokerDiesOnClosedChannel) {
EXPECT_EQ(1, process_info.si_status);
}
+TEST(BrokerProcess, CreateFile) {
+ std::string temp_str;
+ {
+ ScopedTemporaryFile tmp_file;
+ temp_str = tmp_file.full_file_name();
+ }
+ const char* tempfile_name = temp_str.c_str();
+
+ std::vector<BrokerFilePermission> permissions;
+ permissions.push_back(BrokerFilePermission::ReadWriteCreate(tempfile_name));
+
+ BrokerProcess open_broker(EPERM, permissions);
+ ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+
+ int fd = -1;
+
+ // Try without O_EXCL
+ fd = open_broker.Open(tempfile_name, O_RDWR | O_CREAT);
+ ASSERT_EQ(fd, -EPERM);
+
+ const char kTestText[] = "TESTTESTTEST";
+ // Create a file
+ fd = open_broker.Open(tempfile_name, O_RDWR | O_CREAT | O_EXCL);
+ ASSERT_GE(fd, 0);
+ {
+ base::ScopedFD scoped_fd(fd);
+
+ // Confirm fail if file exists
+ int bad_fd = open_broker.Open(tempfile_name, O_RDWR | O_CREAT | O_EXCL);
+ ASSERT_EQ(bad_fd, -EEXIST);
+
+ // Write to the descriptor opened by the broker.
+
+ ssize_t len = HANDLE_EINTR(write(fd, kTestText, sizeof(kTestText)));
+ ASSERT_EQ(len, static_cast<ssize_t>(sizeof(kTestText)));
+ }
+
+ int fd_check = open(tempfile_name, O_RDONLY);
+ ASSERT_GE(fd_check, 0);
+ {
+ base::ScopedFD scoped_fd(fd_check);
+ char buf[1024];
+ ssize_t len = HANDLE_EINTR(read(fd_check, buf, sizeof(buf)));
+
+ ASSERT_EQ(len, static_cast<ssize_t>(sizeof(kTestText)));
+ ASSERT_EQ(memcmp(kTestText, buf, sizeof(kTestText)), 0);
+ }
+}
+
} // namespace syscall_broker
} // namespace sandbox
« no previous file with comments | « sandbox/linux/syscall_broker/broker_process.cc ('k') | sandbox/linux/tests/unit_tests.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698