Broker readlink system call subject to read permissions.

This allows the readlink call to trap into a broker process rather
than being denied inside a sandbox.

Bug: 715679
Change-Id: Id03f45b298ba3c09de9a9b8ac3afc93a368bb4d5
Reviewed-on: https://chromium-review.googlesource.com/795012Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520996}

sandbox/linux/syscall_broker/broker_client.cc

@@ -229,5 +229,50 @@ int BrokerClient::Rename(const char* oldpath, const char* newpath) {
   return return_value;
 }
 
+int BrokerClient::Readlink(const char* path, char* buf, size_t bufsize) {
+  if (fast_check_in_client_) {
+    bool ignore;
+    if (!broker_policy_.GetFileNameIfAllowedToOpen(path, O_RDONLY, nullptr,
+                                                   &ignore)) {
+      return -broker_policy_.denied_errno();
+    }
+  }
+
+  base::Pickle write_pickle;
+  write_pickle.WriteInt(COMMAND_READLINK);
+  write_pickle.WriteString(path);
+  RAW_CHECK(write_pickle.size() <= kMaxMessageLength);
+
+  int returned_fd = -1;
+  uint8_t reply_buf[kMaxMessageLength];
+  ssize_t msg_len = base::UnixDomainSocket::SendRecvMsg(
+      ipc_channel_.get(), reply_buf, sizeof(reply_buf), &returned_fd,
+      write_pickle);
+
+  if (msg_len <= 0) {
+    if (!quiet_failures_for_tests_)
+      RAW_LOG(ERROR, "Could not make request to broker process");
+    return -ENOMEM;
+  }
+
+  base::Pickle read_pickle(reinterpret_cast<char*>(reply_buf), msg_len);
+  base::PickleIterator iter(read_pickle);
+  int return_value = -1;
+  int return_length = 0;
+  const char* return_data = nullptr;
+  if (!iter.ReadInt(&return_value))
+    return -ENOMEM;
+  if (return_value < 0)
+    return return_value;
+  if (!iter.ReadData(&return_data, &return_length))
+    return -ENOMEM;
+  if (return_length < 0)
+    return -ENOMEM;
+  if (static_cast<size_t>(return_length) > bufsize)
+    return -ENAMETOOLONG;
+  memcpy(buf, return_data, return_length);
+  return return_value;
+}
+
 }  // namespace syscall_broker
 }  // namespace sandbox

sandbox/linux/syscall_broker/broker_client.h

@@ -65,6 +65,9 @@ class BrokerClient {
   // This is async signal safe.
   int Rename(const char* oldpath, const char* newpath);
 
+  // Can be used in place of Readlink().
+  int Readlink(const char* path, char* buf, size_t bufsize);
+
   // Get the file descriptor used for IPC. This is used for tests.
   int GetIPCDescriptor() const { return ipc_channel_.get(); }
 

sandbox/linux/syscall_broker/broker_common.h

@@ -9,7 +9,6 @@
 #include <stddef.h>
 
 namespace sandbox {
-
 namespace syscall_broker {
 
 const size_t kMaxMessageLength = 4096;
@@ -35,10 +34,10 @@ enum IPCCommand {
   COMMAND_STAT,
   COMMAND_STAT64,
   COMMAND_RENAME,
+  COMMAND_READLINK,
 };
 
 }  // namespace syscall_broker
-
 }  // namespace sandbox
 
 #endif  // SANDBOX_LINUX_SYSCALL_BROKER_BROKER_COMMON_H_

sandbox/linux/syscall_broker/broker_host.cc

@@ -6,6 +6,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <limits.h>
 #include <stddef.h>
 #include <sys/socket.h>
 #include <sys/stat.h>
@@ -110,7 +111,6 @@ void StatFileForIPC(const BrokerPolicy& policy,
                     IPCCommand command_type,
                     const std::string& requested_filename,
                     base::Pickle* write_pickle) {
-  DCHECK(write_pickle);
   DCHECK(command_type == COMMAND_STAT || command_type == COMMAND_STAT64);
   const char* file_to_access = nullptr;
   if (!policy.GetFileNameIfAllowedToAccess(requested_filename.c_str(), F_OK,
@@ -143,7 +143,6 @@ void RenameFileForIPC(const BrokerPolicy& policy,
                       const std::string& old_filename,
                       const std::string& new_filename,
                       base::Pickle* write_pickle) {
-  DCHECK(write_pickle);
   bool ignore;
   const char* old_file_to_access = nullptr;
   const char* new_file_to_access = nullptr;
@@ -161,6 +160,27 @@ void RenameFileForIPC(const BrokerPolicy& policy,
   write_pickle->WriteInt(0);
 }
 
+// Perform readlink(2) on |filename| using a buffer of MAX_PATH bytes.
+void ReadlinkFileForIPC(const BrokerPolicy& policy,
+                        const std::string& filename,
+                        base::Pickle* write_pickle) {
+  bool ignore;
+  const char* file_to_access = nullptr;
+  if (!policy.GetFileNameIfAllowedToOpen(filename.c_str(), O_RDONLY,
+                                         &file_to_access, &ignore)) {
+    write_pickle->WriteInt(-policy.denied_errno());
+    return;
+  }
+  char buf[PATH_MAX];
+  ssize_t result = readlink(file_to_access, buf, sizeof(buf));
+  if (result < 0) {
+    write_pickle->WriteInt(-errno);
+    return;
+  }
+  write_pickle->WriteInt(result);
+  write_pickle->WriteData(buf, result);
+}
+
 // Handle a |command_type| request contained in |iter| and write the reply
 // to |write_pickle|, adding any files opened to |opened_files|.
 bool HandleRemoteCommand(const BrokerPolicy& policy,
@@ -206,6 +226,13 @@ bool HandleRemoteCommand(const BrokerPolicy& policy,
       RenameFileForIPC(policy, old_filename, new_filename, write_pickle);
       break;
     }
+    case COMMAND_READLINK: {
+      std::string filename;
+      if (!iter.ReadString(&filename))
+        return false;
+      ReadlinkFileForIPC(policy, filename, write_pickle);
+      break;
+    }
     default:
       LOG(ERROR) << "Invalid IPC command";
       return false;

sandbox/linux/syscall_broker/broker_process.cc

@@ -129,6 +129,11 @@ int BrokerProcess::Rename(const char* oldpath, const char* newpath) const {
   return broker_client_->Rename(oldpath, newpath);
 }
 
+int BrokerProcess::Readlink(const char* path, char* buf, size_t bufsize) const {
+  RAW_CHECK(initialized_);
+  return broker_client_->Readlink(path, buf, bufsize);
+}
+
 #if defined(MEMORY_SANITIZER)
 #define BROKER_UNPOISON_STRING(x) __msan_unpoison_string(x)
 #else
@@ -196,6 +201,22 @@ intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
       return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
                                   reinterpret_cast<struct stat*>(args.args[2]));
 #endif
+#if defined(__NR_readlink)
+    case __NR_readlink:
+      return broker_process->Readlink(
+          reinterpret_cast<const char*>(args.args[0]),
+          reinterpret_cast<char*>(args.args[1]),
+          static_cast<size_t>(args.args[2]));
+#endif
+#if defined(__NR_readlinkat)
+    case __NR_readlinkat:
+      if (static_cast<int>(args.args[0]) != AT_FDCWD)
+        return -EPERM;
+      return broker_process->Readlink(
+          reinterpret_cast<const char*>(args.args[1]),
+          reinterpret_cast<char*>(args.args[2]),
+          static_cast<size_t>(args.args[3]));
+#endif
 #if defined(__NR_rename)
     case __NR_rename:
       return broker_process->Rename(

sandbox/linux/syscall_broker/broker_process.h

@@ -83,6 +83,10 @@ class SANDBOX_EXPORT BrokerProcess {
   // It's similar to the rename() system call and will return -errno on errors.
   int Rename(const char* oldpath, const char* newpath) const;
 
+  // Can be used in place of readlink(). Will be async signal safe.
+  // It's similar to the read() system call and will return -errno on errors.
+  int Readlink(const char* path, char* buf, size_t bufsize) const;
+
   int broker_pid() const { return broker_pid_; }
 
   // Handler to be used with a bpf_dsl Trap() function to forward system calls

sandbox/linux/syscall_broker/broker_process_unittest.cc

@@ -60,7 +60,7 @@ TEST(BrokerProcess, CreateAndDestroy) {
 
   std::unique_ptr<BrokerProcess> open_broker(
       new BrokerProcess(EPERM, permissions));
-  ASSERT_TRUE(open_broker->Init(base::Bind(&NoOpCallback)));
+  ASSERT_TRUE(open_broker->Init(base::BindRepeating(&NoOpCallback)));
 
   ASSERT_TRUE(TestUtils::CurrentProcessHasChildren());
   // Destroy the broker and check it has exited properly.
@@ -71,7 +71,7 @@ TEST(BrokerProcess, CreateAndDestroy) {
 TEST(BrokerProcess, TestOpenAccessNull) {
   std::vector<BrokerFilePermission> empty;
   BrokerProcess open_broker(EPERM, empty);
-  ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+  ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
   int fd = open_broker.Open(NULL, O_RDONLY);
   ASSERT_EQ(fd, -EFAULT);
@@ -97,7 +97,7 @@ void TestOpenFilePerms(bool fast_check_in_client, int denied_errno) {
   permissions.push_back(BrokerFilePermission::ReadWrite(kRW_WhiteListed));
 
   BrokerProcess open_broker(denied_errno, permissions, fast_check_in_client);
-  ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+  ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
   int fd = -1;
   fd = open_broker.Open(kR_WhiteListed, O_RDONLY);
@@ -254,7 +254,7 @@ void TestBadPaths(bool fast_check_in_client) {
   permissions.push_back(BrokerFilePermission::ReadOnlyRecursive("/proc/"));
   std::unique_ptr<BrokerProcess> open_broker(
       new BrokerProcess(EPERM, permissions, fast_check_in_client));
-  ASSERT_TRUE(open_broker->Init(base::Bind(&NoOpCallback)));
+  ASSERT_TRUE(open_broker->Init(base::BindRepeating(&NoOpCallback)));
   // Open cpuinfo via the broker.
   int cpuinfo_fd = open_broker->Open(kFileCpuInfo, O_RDONLY);
   base::ScopedFD cpuinfo_fd_closer(cpuinfo_fd);
@@ -313,7 +313,7 @@ void TestOpenCpuinfo(bool fast_check_in_client, bool recursive) {
 
   std::unique_ptr<BrokerProcess> open_broker(
       new BrokerProcess(EPERM, permissions, fast_check_in_client));
-  ASSERT_TRUE(open_broker->Init(base::Bind(&NoOpCallback)));
+  ASSERT_TRUE(open_broker->Init(base::BindRepeating(&NoOpCallback)));
 
   int fd = -1;
   fd = open_broker->Open(kFileCpuInfo, O_RDWR);
@@ -390,7 +390,7 @@ TEST(BrokerProcess, OpenFileRW) {
   permissions.push_back(BrokerFilePermission::ReadWrite(tempfile_name));
 
   BrokerProcess open_broker(EPERM, permissions);
-  ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+  ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
   // Check we can access that file with read or write.
   int can_access = open_broker.Access(tempfile_name, R_OK | W_OK);
@@ -425,7 +425,7 @@ SANDBOX_TEST(BrokerProcess, BrokerDied) {
 
   BrokerProcess open_broker(EPERM, permissions, true /* fast_check_in_client */,
                             true /* quiet_failures_for_tests */);
-  SANDBOX_ASSERT(open_broker.Init(base::Bind(&NoOpCallback)));
+  SANDBOX_ASSERT(open_broker.Init(base::BindRepeating(&NoOpCallback)));
   const pid_t broker_pid = open_broker.broker_pid();
   SANDBOX_ASSERT(kill(broker_pid, SIGKILL) == 0);
 
@@ -449,7 +449,7 @@ void TestOpenComplexFlags(bool fast_check_in_client) {
   permissions.push_back(BrokerFilePermission::ReadOnly(kCpuInfo));
 
   BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-  ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+  ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
   // Test that we do the right thing for O_CLOEXEC and O_NONBLOCK.
   int fd = -1;
   int ret = 0;
@@ -538,7 +538,7 @@ SANDBOX_TEST_ALLOW_NOISE(BrokerProcess, MAYBE_RecvMsgDescriptorLeak) {
   permissions.push_back(BrokerFilePermission::ReadOnly(kCpuInfo));
 
   BrokerProcess open_broker(EPERM, permissions);
-  SANDBOX_ASSERT(open_broker.Init(base::Bind(&NoOpCallback)));
+  SANDBOX_ASSERT(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
   const int ipc_fd = BrokerProcessTestHelper::GetIPCDescriptor(&open_broker);
   SANDBOX_ASSERT(ipc_fd >= 0);
@@ -588,7 +588,7 @@ TEST(BrokerProcess, BrokerDiesOnClosedChannel) {
 
   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])));
+  ASSERT_TRUE(open_broker.Init(base::BindRepeating(&CloseFD, lifeline_fds[0])));
   // Make sure the writing end only exists in the broker process.
   CloseFD(lifeline_fds[1]);
   base::ScopedFD reader(lifeline_fds[0]);
@@ -624,7 +624,7 @@ TEST(BrokerProcess, CreateFile) {
   permissions.push_back(BrokerFilePermission::ReadWriteCreate(tempfile_name));
 
   BrokerProcess open_broker(EPERM, permissions);
-  ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+  ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
   int fd = -1;
 
@@ -674,7 +674,7 @@ TEST(BrokerProcess, StatFile) {
     // Nonexistent file with no permissions to see file.
     std::vector<BrokerFilePermission> permissions;
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
     memset(&sb, 0, sizeof(sb));
     EXPECT_EQ(-EPERM, open_broker.Stat(nonesuch_name, &sb));
@@ -683,7 +683,7 @@ TEST(BrokerProcess, StatFile) {
     // Actual file with no permission to see file.
     std::vector<BrokerFilePermission> permissions;
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
     memset(&sb, 0, sizeof(sb));
     EXPECT_EQ(-EPERM, open_broker.Stat(tempfile_name, &sb));
@@ -693,7 +693,7 @@ TEST(BrokerProcess, StatFile) {
     std::vector<BrokerFilePermission> permissions;
     permissions.push_back(BrokerFilePermission::ReadOnly(nonesuch_name));
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
     memset(&sb, 0, sizeof(sb));
     EXPECT_EQ(-ENOENT, open_broker.Stat(nonesuch_name, &sb));
@@ -703,7 +703,7 @@ TEST(BrokerProcess, StatFile) {
     std::vector<BrokerFilePermission> permissions;
     permissions.push_back(BrokerFilePermission::ReadOnly(tempfile_name));
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
 
     memset(&sb, 0, sizeof(sb));
     EXPECT_EQ(0, open_broker.Stat(tempfile_name, &sb));
@@ -754,7 +754,7 @@ TEST(BrokerProcess, RenameFile) {
 
     bool fast_check_in_client = false;
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
     EXPECT_EQ(-EPERM, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
 
     // ... and no files moved around.
@@ -768,7 +768,7 @@ TEST(BrokerProcess, RenameFile) {
 
     bool fast_check_in_client = false;
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
     EXPECT_EQ(-EPERM, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
 
     // ... and no files moved around.
@@ -783,7 +783,7 @@ TEST(BrokerProcess, RenameFile) {
 
     bool fast_check_in_client = false;
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
     EXPECT_EQ(-EPERM, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
 
     // ... and no files moved around.
@@ -798,7 +798,7 @@ TEST(BrokerProcess, RenameFile) {
 
     bool fast_check_in_client = false;
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
     EXPECT_EQ(-EPERM, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
 
     // ... and no files moved around.
@@ -813,7 +813,7 @@ TEST(BrokerProcess, RenameFile) {
 
     bool fast_check_in_client = false;
     BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
-    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
     EXPECT_EQ(0, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
 
     // ... and files were moved around.
@@ -825,5 +825,77 @@ TEST(BrokerProcess, RenameFile) {
   unlink(newpath.c_str());
 }
 
+void TestReadlinkHelper(bool fast_check_in_client) {
+  std::string oldpath;
+  std::string newpath;
+  {
+    // Just to generate names and ensure they do not exist upon scope exit.
+    ScopedTemporaryFile oldfile;
+    ScopedTemporaryFile newfile;
+    oldpath = oldfile.full_file_name();
+    newpath = newfile.full_file_name();
+  }
+
+  // Now make a link from old to new path name.
+  EXPECT_TRUE(symlink(oldpath.c_str(), newpath.c_str()) == 0);
+
+  const char* nonesuch_name = "/mbogo/nonesuch";
+  const char* oldpath_name = oldpath.c_str();
+  const char* newpath_name = newpath.c_str();
+  char buf[1024];
+  {
+    // Nonexistent file with no permissions to see file.
+    std::vector<BrokerFilePermission> permissions;
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
+    EXPECT_EQ(-EPERM, open_broker.Readlink(nonesuch_name, buf, sizeof(buf)));
+  }
+  {
+    // Actual file with no permissions to see file.
+    std::vector<BrokerFilePermission> permissions;
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
+    EXPECT_EQ(-EPERM, open_broker.Readlink(newpath_name, buf, sizeof(buf)));
+  }
+  {
+    // Nonexistent file with permissions to see file.
+    std::vector<BrokerFilePermission> permissions;
+    permissions.push_back(BrokerFilePermission::ReadOnly(nonesuch_name));
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
+    EXPECT_EQ(-ENOENT, open_broker.Readlink(nonesuch_name, buf, sizeof(buf)));
+  }
+  {
+    // Actual file with permissions to see file.
+    std::vector<BrokerFilePermission> permissions;
+    permissions.push_back(BrokerFilePermission::ReadOnly(newpath_name));
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
+    ssize_t retlen = open_broker.Readlink(newpath_name, buf, sizeof(buf));
+    EXPECT_TRUE(retlen == static_cast<ssize_t>(strlen(oldpath_name)));
+    EXPECT_EQ(0, memcmp(oldpath_name, buf, retlen));
+  }
+  {
+    // Actual file with permissions to see file, but too small a buffer.
+    std::vector<BrokerFilePermission> permissions;
+    permissions.push_back(BrokerFilePermission::ReadOnly(newpath_name));
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::BindRepeating(&NoOpCallback)));
+    EXPECT_EQ(-ENAMETOOLONG, open_broker.Readlink(newpath_name, buf, 4));
+  }
+
+  // Cleanup both paths.
+  unlink(oldpath.c_str());
+  unlink(newpath.c_str());
+}
+
+TEST(BrokerProcess, ReadlinkFileClient) {
+  TestReadlinkHelper(true);
+}
+
+TEST(BrokerProcess, ReadlinkFileHost) {
+  TestReadlinkHelper(false);
+}
+
 }  // namespace syscall_broker
 }  // namespace sandbox