Broker rename(2) system call subject to write permissions.

This was also seen in strace of network service.
Moved some code around to put reading command and writing reply
to the socket in the same function.

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

sandbox/linux/syscall_broker/broker_client.cc

@@ -191,5 +191,43 @@ int BrokerClient::StatFamilySyscall(IPCCommand syscall_type,
   return return_value;
 }
 
+int BrokerClient::Rename(const char* oldpath, const char* newpath) {
+  if (fast_check_in_client_) {
+    bool ignore;
+    if (!broker_policy_.GetFileNameIfAllowedToOpen(oldpath, O_RDWR, nullptr,
+                                                   &ignore) ||
+        !broker_policy_.GetFileNameIfAllowedToOpen(newpath, O_RDWR, nullptr,
+                                                   &ignore)) {
+      return -broker_policy_.denied_errno();
+    }
+  }
+
+  base::Pickle write_pickle;
+  write_pickle.WriteInt(COMMAND_RENAME);
+  write_pickle.WriteString(oldpath);
+  write_pickle.WriteString(newpath);
+  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;
+  if (!iter.ReadInt(&return_value))
+    return -ENOMEM;
+
+  return return_value;
+}
+
 }  // namespace syscall_broker
 }  // namespace sandbox

sandbox/linux/syscall_broker/broker_client.h

@@ -60,6 +60,11 @@ class BrokerClient {
   int Stat(const char* pathname, struct stat* sb);
   int Stat64(const char* pathname, struct stat64* sb);
 
+  // Can be used in place of rename().
+  // It's similar to the rename() system call and will return -errno on errors.
+  // This is async signal safe.
+  int Rename(const char* oldpath, const char* newpath);
+
   // 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

@@ -34,6 +34,7 @@ enum IPCCommand {
   COMMAND_ACCESS,
   COMMAND_STAT,
   COMMAND_STAT64,
+  COMMAND_RENAME,
 };
 
 }  // namespace syscall_broker

sandbox/linux/syscall_broker/broker_host.cc

@@ -137,59 +137,78 @@ void StatFileForIPC(const BrokerPolicy& policy,
   }
 }
 
-// Handle a |command_type| request contained in |iter| and send the reply
-// on |reply_ipc|.
+// Perform rename(2) on |old_filename| to |new_filename| and marshal the
+// result to |write_pickle|.
+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;
+  if (!policy.GetFileNameIfAllowedToOpen(old_filename.c_str(), O_RDWR,
+                                         &old_file_to_access, &ignore) ||
+      !policy.GetFileNameIfAllowedToOpen(new_filename.c_str(), O_RDWR,
+                                         &new_file_to_access, &ignore)) {
+    write_pickle->WriteInt(-policy.denied_errno());
+    return;
+  }
+  if (rename(old_file_to_access, new_file_to_access) < 0) {
+    write_pickle->WriteInt(-errno);
+    return;
+  }
+  write_pickle->WriteInt(0);
+}
+
+// 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,
-                         IPCCommand command_type,
-                         int reply_ipc,
-                         base::PickleIterator iter) {
-  // Currently all commands have filename as the first arg.
-  std::string requested_filename;
-  if (!iter.ReadString(&requested_filename))
+                         base::PickleIterator iter,
+                         base::Pickle* write_pickle,
+                         std::vector<int>* opened_files) {
+  int command_type;
+  if (!iter.ReadInt(&command_type))
     return false;
 
-  base::Pickle write_pickle;
-  std::vector<int> opened_files;
-
   switch (command_type) {
     case COMMAND_ACCESS: {
+      std::string requested_filename;
       int flags = 0;
-      if (!iter.ReadInt(&flags))
+      if (!iter.ReadString(&requested_filename) || !iter.ReadInt(&flags))
         return false;
-      AccessFileForIPC(policy, requested_filename, flags, &write_pickle);
+      AccessFileForIPC(policy, requested_filename, flags, write_pickle);
       break;
     }
     case COMMAND_OPEN: {
+      std::string requested_filename;
       int flags = 0;
-      if (!iter.ReadInt(&flags))
+      if (!iter.ReadString(&requested_filename) || !iter.ReadInt(&flags))
         return false;
-      OpenFileForIPC(
-          policy, requested_filename, flags, &write_pickle, &opened_files);
+      OpenFileForIPC(policy, requested_filename, flags, write_pickle,
+                     opened_files);
       break;
     }
     case COMMAND_STAT:
     case COMMAND_STAT64: {
-      StatFileForIPC(policy, command_type, requested_filename, &write_pickle);
+      std::string requested_filename;
+      if (!iter.ReadString(&requested_filename))
+        return false;
+      StatFileForIPC(policy, static_cast<IPCCommand>(command_type),
+                     requested_filename, write_pickle);
+      break;
+    }
+    case COMMAND_RENAME: {
+      std::string old_filename;
+      std::string new_filename;
+      if (!iter.ReadString(&old_filename) || !iter.ReadString(&new_filename))
+        return false;
+      RenameFileForIPC(policy, old_filename, new_filename, write_pickle);
       break;
     }
     default:
       LOG(ERROR) << "Invalid IPC command";
-      break;
-  }
-
-  CHECK_LE(write_pickle.size(), kMaxMessageLength);
-  ssize_t sent = base::UnixDomainSocket::SendMsg(
-      reply_ipc, write_pickle.data(), write_pickle.size(), opened_files);
-
-  // Close anything we have opened in this process.
-  for (int fd : opened_files) {
-    int ret = IGNORE_EINTR(close(fd));
-    DCHECK(!ret) << "Could not close file descriptor";
-  }
-
-  if (sent <= 0) {
-    LOG(ERROR) << "Could not send IPC reply";
-    return false;
+      return false;
   }
   return true;
 }
@@ -230,19 +249,29 @@ BrokerHost::RequestStatus BrokerHost::HandleRequest() const {
 
   base::Pickle pickle(buf, msg_len);
   base::PickleIterator iter(pickle);
-  int command_type;
-  if (iter.ReadInt(&command_type)) {
-    bool command_handled = HandleRemoteCommand(
-        broker_policy_, static_cast<IPCCommand>(command_type),
-        temporary_ipc.get(), iter);
-    if (!command_handled)
-      return RequestStatus::FAILURE;
-
-    return RequestStatus::SUCCESS;
+  base::Pickle write_pickle;
+  std::vector<int> opened_files;
+  bool result =
+      HandleRemoteCommand(broker_policy_, iter, &write_pickle, &opened_files);
+
+  if (result) {
+    CHECK_LE(write_pickle.size(), kMaxMessageLength);
+    ssize_t sent = base::UnixDomainSocket::SendMsg(
+        temporary_ipc.get(), write_pickle.data(), write_pickle.size(),
+        opened_files);
+    if (sent <= 0) {
+      LOG(ERROR) << "Could not send IPC reply";
+      result = false;
+    }
+  }
+
+  // Close anything we have opened in this process.
+  for (int fd : opened_files) {
+    int ret = IGNORE_EINTR(close(fd));
+    DCHECK(!ret) << "Could not close file descriptor";
   }
 
-  LOG(ERROR) << "Error parsing IPC request";
-  return RequestStatus::FAILURE;
+  return result ? RequestStatus::SUCCESS : RequestStatus::FAILURE;
 }
 
 }  // namespace syscall_broker

sandbox/linux/syscall_broker/broker_process.cc

@@ -124,6 +124,11 @@ int BrokerProcess::Stat64(const char* pathname, struct stat64* sb) const {
   return broker_client_->Stat64(pathname, sb);
 }
 
+int BrokerProcess::Rename(const char* oldpath, const char* newpath) const {
+  RAW_CHECK(initialized_);
+  return broker_client_->Rename(oldpath, newpath);
+}
+
 #if defined(MEMORY_SANITIZER)
 #define BROKER_UNPOISON_STRING(x) __msan_unpoison_string(x)
 #else
@@ -190,6 +195,34 @@ intptr_t BrokerProcess::SIGSYS_Handler(const sandbox::arch_seccomp_data& args,
         return -EINVAL;
       return broker_process->Stat(reinterpret_cast<const char*>(args.args[1]),
                                   reinterpret_cast<struct stat*>(args.args[2]));
+#endif
+#if defined(__NR_rename)
+    case __NR_rename:
+      return broker_process->Rename(
+          reinterpret_cast<const char*>(args.args[0]),
+          reinterpret_cast<const char*>(args.args[1]));
+#endif
+#if defined(__NR_renameat)
+    case __NR_renameat:
+      if (static_cast<int>(args.args[0]) != AT_FDCWD ||
+          static_cast<int>(args.args[2]) != AT_FDCWD) {
+        return -EPERM;
+      }
+      return broker_process->Rename(
+          reinterpret_cast<const char*>(args.args[1]),
+          reinterpret_cast<const char*>(args.args[3]));
+#endif
+#if defined(__NR_renameat2)
+    case __NR_renameat2:
+      if (static_cast<int>(args.args[0]) != AT_FDCWD ||
+          static_cast<int>(args.args[2]) != AT_FDCWD) {
+        return -EPERM;
+      }
+      if (static_cast<int>(args.args[4]) != 0)
+        return -EINVAL;
+      return broker_process->Rename(
+          reinterpret_cast<const char*>(args.args[1]),
+          reinterpret_cast<const char*>(args.args[3]));
 #endif
     default:
       RAW_CHECK(false);

sandbox/linux/syscall_broker/broker_process.h

@@ -79,6 +79,10 @@ class SANDBOX_EXPORT BrokerProcess {
   int Stat(const char* pathname, struct stat* sb) const;
   int Stat64(const char* pathname, struct stat64* sb) const;
 
+  // Can be used in place of rename(). Will be async signal safe.
+  // It's similar to the rename() system call and will return -errno on errors.
+  int Rename(const char* oldpath, const char* newpath) 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

@@ -728,5 +728,102 @@ TEST(BrokerProcess, StatFile) {
   }
 }
 
+TEST(BrokerProcess, RenameFile) {
+  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 file using old path name.
+  int fd = open(oldpath.c_str(), O_RDWR | O_CREAT, 0600);
+  EXPECT_TRUE(fd > 0);
+  close(fd);
+
+  EXPECT_TRUE(access(oldpath.c_str(), F_OK) == 0);
+  EXPECT_TRUE(access(newpath.c_str(), F_OK) < 0);
+
+  {
+    // Check rename fails when no permission to new file.
+    std::vector<BrokerFilePermission> permissions;
+    permissions.push_back(BrokerFilePermission::ReadWrite(oldpath));
+
+    bool fast_check_in_client = false;
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    EXPECT_EQ(-EPERM, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
+
+    // ... and no files moved around.
+    EXPECT_TRUE(access(oldpath.c_str(), F_OK) == 0);
+    EXPECT_TRUE(access(newpath.c_str(), F_OK) < 0);
+  }
+  {
+    // Check rename fails when no permission to old file.
+    std::vector<BrokerFilePermission> permissions;
+    permissions.push_back(BrokerFilePermission::ReadWrite(newpath));
+
+    bool fast_check_in_client = false;
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    EXPECT_EQ(-EPERM, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
+
+    // ... and no files moved around.
+    EXPECT_TRUE(access(oldpath.c_str(), F_OK) == 0);
+    EXPECT_TRUE(access(newpath.c_str(), F_OK) < 0);
+  }
+  {
+    // Check rename fails when only read permission to first file.
+    std::vector<BrokerFilePermission> permissions;
+    permissions.push_back(BrokerFilePermission::ReadOnly(oldpath));
+    permissions.push_back(BrokerFilePermission::ReadWrite(newpath));
+
+    bool fast_check_in_client = false;
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    EXPECT_EQ(-EPERM, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
+
+    // ... and no files moved around.
+    EXPECT_TRUE(access(oldpath.c_str(), F_OK) == 0);
+    EXPECT_TRUE(access(newpath.c_str(), F_OK) < 0);
+  }
+  {
+    // Check rename fails when only read permission to first file.
+    std::vector<BrokerFilePermission> permissions;
+    permissions.push_back(BrokerFilePermission::ReadWrite(oldpath));
+    permissions.push_back(BrokerFilePermission::ReadOnly(newpath));
+
+    bool fast_check_in_client = false;
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    EXPECT_EQ(-EPERM, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
+
+    // ... and no files moved around.
+    EXPECT_TRUE(access(oldpath.c_str(), F_OK) == 0);
+    EXPECT_TRUE(access(newpath.c_str(), F_OK) < 0);
+  }
+  {
+    // Check rename passes with write permissions to both files.
+    std::vector<BrokerFilePermission> permissions;
+    permissions.push_back(BrokerFilePermission::ReadWrite(oldpath));
+    permissions.push_back(BrokerFilePermission::ReadWrite(newpath));
+
+    bool fast_check_in_client = false;
+    BrokerProcess open_broker(EPERM, permissions, fast_check_in_client);
+    ASSERT_TRUE(open_broker.Init(base::Bind(&NoOpCallback)));
+    EXPECT_EQ(0, open_broker.Rename(oldpath.c_str(), newpath.c_str()));
+
+    // ... and files were moved around.
+    EXPECT_TRUE(access(oldpath.c_str(), F_OK) < 0);
+    EXPECT_TRUE(access(newpath.c_str(), F_OK) == 0);
+  }
+
+  // Cleanup using new path name.
+  unlink(newpath.c_str());
+}
+
 }  // namespace syscall_broker
 }  // namespace sandbox