Linux sandbox: Add BrokerType and ForkSignalBasedBroker()

Adds BrokerProcess:BrokerType to BrokerProcess. It currently only
includes SIGNAL_BASED, but will be used to add USER_NOTIF

Also adds ForkSignalBasedBroker() in BrokerProcess, in order to factor
out the signal mechanism-specific code from BrokerProcess::Init().
Includes some other minor (but related) refactors.

Bug: 1117351
Change-Id: Idadeaafb4ef7bb0f952fd0d901cbfb91d59b3486
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2407059
Commit-Queue: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809493}

sandbox/linux/integration_tests/seccomp_broker_process_unittest.cc

@@ -48,6 +48,7 @@ using bpf_dsl::ResultExpr;
 using bpf_dsl::Trap;
 
 using BrokerProcess = syscall_broker::BrokerProcess;
+using BrokerType = syscall_broker::BrokerProcess::BrokerType;
 using BrokerCommandSet = syscall_broker::BrokerCommandSet;
 using BrokerFilePermission = syscall_broker::BrokerFilePermission;
 
@@ -55,15 +56,16 @@ using BrokerFilePermission = syscall_broker::BrokerFilePermission;
 
 class InitializedOpenBroker {
  public:
-  InitializedOpenBroker() {
+  explicit InitializedOpenBroker(
+      BrokerType broker_type = BrokerType::SIGNAL_BASED) {
     syscall_broker::BrokerCommandSet command_set;
     command_set.set(syscall_broker::COMMAND_OPEN);
     command_set.set(syscall_broker::COMMAND_ACCESS);
     std::vector<BrokerFilePermission> permissions = {
         BrokerFilePermission::ReadOnly("/proc/allowed"),
         BrokerFilePermission::ReadOnly("/proc/cpuinfo")};
-    broker_process_ =
-        std::make_unique<BrokerProcess>(EPERM, command_set, permissions);
+    broker_process_ = std::make_unique<BrokerProcess>(EPERM, command_set,
+                                                      permissions, broker_type);
     BPF_ASSERT(broker_process_->Init(base::BindOnce([]() { return true; })));
   }
 
@@ -493,10 +495,12 @@ class BPFTesterBrokerDelegate : public BPFTesterDelegate {
  public:
   explicit BPFTesterBrokerDelegate(bool fast_check_in_client,
                                    BrokerTestDelegate* broker_test_delegate,
-                                   SyscallerType syscaller_type)
+                                   SyscallerType syscaller_type,
+                                   BrokerType broker_type)
       : fast_check_in_client_(fast_check_in_client),
         broker_test_delegate_(broker_test_delegate),
-        syscaller_type_(syscaller_type) {}
+        syscaller_type_(syscaller_type),
+        broker_type_(broker_type) {}
   ~BPFTesterBrokerDelegate() override = default;
 
   std::unique_ptr<bpf_dsl::Policy> GetSandboxBPFPolicy() override {
@@ -505,7 +509,7 @@ class BPFTesterBrokerDelegate : public BPFTesterDelegate {
 
     broker_process_ = std::make_unique<BrokerProcess>(
         broker_params.denied_errno, broker_params.allowed_command_set,
-        broker_params.permissions, fast_check_in_client_);
+        broker_params.permissions, broker_type_, fast_check_in_client_);
     BPF_ASSERT(broker_process_->Init(base::BindOnce([]() { return true; })));
     broker_test_delegate_->OnBrokerStarted(broker_process_->broker_pid());
 
@@ -546,6 +550,7 @@ class BPFTesterBrokerDelegate : public BPFTesterDelegate {
   bool fast_check_in_client_;
   BrokerTestDelegate* broker_test_delegate_;
   SyscallerType syscaller_type_;
+  BrokerType broker_type_;
 
   std::unique_ptr<BrokerProcess> broker_process_;
   std::unique_ptr<Syscaller> syscaller_;
@@ -556,29 +561,35 @@ struct BrokerTestConfiguration {
   std::string test_name;
   bool fast_check_in_client;
   SyscallerType syscaller_type;
+  BrokerType broker_type;
 };
 
 // Lists out all the broker configurations we want to test.
 const std::vector<BrokerTestConfiguration> broker_test_configs = {
-    {"FastCheckInClient_IPCSyscaller", true, SyscallerType::IPCSyscaller},
+    {"FastCheckInClient_IPCSyscaller", true, SyscallerType::IPCSyscaller,
+     BrokerType::SIGNAL_BASED},
 #if defined(DIRECT_SYSCALLER_ENABLED)
-    {"FastCheckInClient_DirectSyscaller", true, SyscallerType::DirectSyscaller},
+    {"FastCheckInClient_DirectSyscaller", true, SyscallerType::DirectSyscaller,
+     BrokerType::SIGNAL_BASED},
 #endif
-    {"FastCheckInClient_LibcSyscaller", true, SyscallerType::LibcSyscaller},
-    {"NoFastCheckInClient_IPCSyscaller", false, SyscallerType::IPCSyscaller},
+    {"FastCheckInClient_LibcSyscaller", true, SyscallerType::LibcSyscaller,
+     BrokerType::SIGNAL_BASED},
+    {"NoFastCheckInClient_IPCSyscaller", false, SyscallerType::IPCSyscaller,
+     BrokerType::SIGNAL_BASED},
 #if defined(DIRECT_SYSCALLER_ENABLED)
     {"NoFastCheckInClient_DirectSyscaller", false,
-     SyscallerType::DirectSyscaller},
+     SyscallerType::DirectSyscaller, BrokerType::SIGNAL_BASED},
 #endif
-    {"NoFastCheckInClient_LibcSyscaller", false, SyscallerType::LibcSyscaller}};
+    {"NoFastCheckInClient_LibcSyscaller", false, SyscallerType::LibcSyscaller,
+     BrokerType::SIGNAL_BASED}};
 }  // namespace
 
 void RunSingleBrokerTest(BrokerTestDelegate* test_delegate,
                          const BrokerTestConfiguration& test_config) {
   test_delegate->ParentSetUp();
-  sandbox::SandboxBPFTestRunner bpf_test_runner(
-      new BPFTesterBrokerDelegate(test_config.fast_check_in_client,
-                                  test_delegate, test_config.syscaller_type));
+  sandbox::SandboxBPFTestRunner bpf_test_runner(new BPFTesterBrokerDelegate(
+      test_config.fast_check_in_client, test_delegate,
+      test_config.syscaller_type, test_config.broker_type));
   sandbox::UnitTests::RunTestInProcess(&bpf_test_runner, DEATH_SUCCESS());
   test_delegate->ParentTearDown();
 }

sandbox/linux/syscall_broker/broker_host.cc

@@ -96,7 +96,7 @@ void OpenFileForIPC(const BrokerCommandSet& allowed_command_set,
                     const std::string& requested_filename,
                     int flags,
                     BrokerSimpleMessage* reply,
-                    int* opened_file) {
+                    base::ScopedFD* opened_file) {
   const char* file_to_open = NULL;
   bool unlink_after_open = false;
   if (!CommandOpenIsSafe(allowed_command_set, permission_list,
@@ -107,8 +107,8 @@ void OpenFileForIPC(const BrokerCommandSet& allowed_command_set,
   }
 
   CHECK(file_to_open);
-  int opened_fd = sys_open(file_to_open, flags);
-  if (opened_fd < 0) {
+  opened_file->reset(sys_open(file_to_open, flags));
+  if (!opened_file->is_valid()) {
     RAW_CHECK(reply->AddIntToMessage(-errno));
     return;
   }
@@ -116,7 +116,6 @@ void OpenFileForIPC(const BrokerCommandSet& allowed_command_set,
   if (unlink_after_open)
     unlink(file_to_open);
 
-  *opened_file = opened_fd;
   RAW_CHECK(reply->AddIntToMessage(0));
 }
 
@@ -243,7 +242,7 @@ bool HandleRemoteCommand(const BrokerCommandSet& allowed_command_set,
                          const BrokerPermissionList& permission_list,
                          BrokerSimpleMessage* message,
                          BrokerSimpleMessage* reply,
-                         int* opened_file) {
+                         base::ScopedFD* opened_file) {
   // Message structure:
   //   int:    command type
   //   char[]: pathname
@@ -346,47 +345,45 @@ BrokerHost::BrokerHost(const BrokerPermissionList& broker_permission_list,
       allowed_command_set_(allowed_command_set),
       ipc_channel_(std::move(ipc_channel)) {}
 
-BrokerHost::~BrokerHost() {}
+BrokerHost::~BrokerHost() = default;
 
 // Handle a request on the IPC channel ipc_channel_.
 // A request should have a file descriptor attached on which we will reply and
 // that we will then close.
 // A request should start with an int that will be used as the command type.
-BrokerHost::RequestStatus BrokerHost::HandleRequest() const {
-  BrokerSimpleMessage message;
-  errno = 0;
-  base::ScopedFD temporary_ipc;
-  const ssize_t msg_len =
-      message.RecvMsgWithFlags(ipc_channel_.get(), 0, &temporary_ipc);
-
-  if (msg_len == 0 || (msg_len == -1 && errno == ECONNRESET)) {
-    // EOF from the client, or the client died, we should die.
-    return RequestStatus::LOST_CLIENT;
-  }
-
-  // The client sends exactly one file descriptor, on which we
-  // will write the reply.
-  if (msg_len < 0) {
-    PLOG(ERROR) << "Error reading message from the client";
-    return RequestStatus::FAILURE;
-  }
+void BrokerHost::LoopAndHandleRequests() {
+  for (;;) {
+    BrokerSimpleMessage message;
+    errno = 0;
+    base::ScopedFD temporary_ipc;
+    const ssize_t msg_len =
+        message.RecvMsgWithFlags(ipc_channel_.get(), 0, &temporary_ipc);
+
+    if (msg_len == 0 || (msg_len == -1 && errno == ECONNRESET)) {
+      // EOF from the client, or the client died, we should finish looping.
+      return;
+    }
 
-  BrokerSimpleMessage reply;
-  int opened_file = -1;
-  bool result =
-      HandleRemoteCommand(allowed_command_set_, broker_permission_list_,
-                          &message, &reply, &opened_file);
+    // The client sends exactly one file descriptor, on which we
+    // will write the reply.
+    if (msg_len < 0) {
+      PLOG(ERROR) << "Error reading message from the client";
+      continue;
+    }
 
-  ssize_t sent = reply.SendMsg(temporary_ipc.get(), opened_file);
-  if (sent < 0)
-    LOG(ERROR) << "sent failed";
+    BrokerSimpleMessage reply;
+    base::ScopedFD opened_file;
+    if (!HandleRemoteCommand(allowed_command_set_, broker_permission_list_,
+                             &message, &reply, &opened_file)) {
+      // Does not exit if we received a malformed message.
+      LOG(ERROR) << "Received malformed message from the client";
+      continue;
+    }
 
-  if (opened_file >= 0) {
-    int ret = IGNORE_EINTR(close(opened_file));
-    DCHECK(!ret) << "Could not close file descriptor";
+    ssize_t sent = reply.SendMsg(temporary_ipc.get(), opened_file.get());
+    if (sent < 0)
+      LOG(ERROR) << "sent failed";
   }
-
-  return result ? RequestStatus::SUCCESS : RequestStatus::FAILURE;
 }
 
 }  // namespace syscall_broker

sandbox/linux/syscall_broker/broker_host.h

@@ -20,14 +20,13 @@ class BrokerPermissionList;
 // |ipc_channel| according to |broker_permission_list|.
 class BrokerHost {
  public:
-  enum class RequestStatus { LOST_CLIENT = 0, SUCCESS, FAILURE };
-
   BrokerHost(const BrokerPermissionList& broker_permission_list,
              const BrokerCommandSet& allowed_command_set,
              BrokerChannel::EndPoint ipc_channel);
   ~BrokerHost();
 
-  RequestStatus HandleRequest() const;
+  // Receive system call requests and handle them forevermore.
+  void LoopAndHandleRequests();
 
  private:
   const BrokerPermissionList& broker_permission_list_;

sandbox/linux/syscall_broker/broker_process.cc

@@ -34,10 +34,12 @@ BrokerProcess::BrokerProcess(
     int denied_errno,
     const syscall_broker::BrokerCommandSet& allowed_command_set,
     const std::vector<syscall_broker::BrokerFilePermission>& permissions,
+    BrokerType broker_type,
     bool fast_check_in_client,
     bool quiet_failures_for_tests)
     : initialized_(false),
       broker_pid_(-1),
+      broker_type_(broker_type),
       fast_check_in_client_(fast_check_in_client),
       quiet_failures_for_tests_(quiet_failures_for_tests),
       allowed_command_set_(allowed_command_set),
@@ -58,16 +60,12 @@ BrokerProcess::~BrokerProcess() {
   }
 }
 
-bool BrokerProcess::Init(
+bool BrokerProcess::ForkSignalBasedBroker(
     base::OnceCallback<bool(void)> broker_process_init_callback) {
-  CHECK(!initialized_);
   BrokerChannel::EndPoint ipc_reader;
   BrokerChannel::EndPoint ipc_writer;
   BrokerChannel::CreatePair(&ipc_reader, &ipc_writer);
 
-#if !defined(THREAD_SANITIZER)
-  DCHECK_EQ(1, base::GetNumberOfThreads(base::GetCurrentProcessHandle()));
-#endif
   int child_pid = fork();
   if (child_pid == -1)
     return false;
@@ -80,9 +78,11 @@ bool BrokerProcess::Init(
     // We are the parent and we have just forked our broker process.
     ipc_reader.reset();
     broker_pid_ = child_pid;
+
     broker_client_ = std::make_unique<BrokerClient>(
         broker_permission_list_, std::move(ipc_writer), allowed_command_set_,
         fast_check_in_client_);
+
     initialized_ = true;
     return true;
   }
@@ -95,18 +95,26 @@ bool BrokerProcess::Init(
   // We are the broker process. Make sure to close the writer's end so that
   // we get notified if the client disappears.
   ipc_writer.reset();
+
   CHECK(std::move(broker_process_init_callback).Run());
-  BrokerHost broker_host(broker_permission_list_, allowed_command_set_,
-                         std::move(ipc_reader));
-  for (;;) {
-    switch (broker_host.HandleRequest()) {
-      case BrokerHost::RequestStatus::LOST_CLIENT:
-        _exit(1);
-      case BrokerHost::RequestStatus::SUCCESS:
-      case BrokerHost::RequestStatus::FAILURE:
-        continue;
-    }
-  }
+
+  BrokerHost broker_host_signal_based(
+      broker_permission_list_, allowed_command_set_, std::move(ipc_reader));
+  broker_host_signal_based.LoopAndHandleRequests();
+  _exit(1);
+  NOTREACHED();
+  return false;
+}
+
+bool BrokerProcess::Init(
+    base::OnceCallback<bool(void)> broker_process_init_callback) {
+  CHECK(!initialized_);
+
+#if !defined(THREAD_SANITIZER)
+  DCHECK_EQ(1, base::GetNumberOfThreads(base::GetCurrentProcessHandle()));
+#endif
+
+  return ForkSignalBasedBroker(std::move(broker_process_init_callback));
 }
 
 bool BrokerProcess::IsSyscallAllowed(int sysno) const {

sandbox/linux/syscall_broker/broker_process.h

@@ -39,6 +39,8 @@ class BrokerFilePermission;
 // 4. Use open_broker.Open() to open files.
 class SANDBOX_EXPORT BrokerProcess {
  public:
+  enum class BrokerType { SIGNAL_BASED };
+
   // |denied_errno| is the error code returned when methods such as Open()
   // or Access() are invoked on a file which is not in the allowlist (EACCESS
   // would be a typical value).  |allowed_command_mask| is a bitwise-or of
@@ -57,6 +59,7 @@ class SANDBOX_EXPORT BrokerProcess {
       int denied_errno,
       const syscall_broker::BrokerCommandSet& allowed_command_set,
       const std::vector<syscall_broker::BrokerFilePermission>& permissions,
+      BrokerType broker_type,
       bool fast_check_in_client = true,
       bool quiet_failures_for_tests = false);
 
@@ -95,11 +98,18 @@ class SANDBOX_EXPORT BrokerProcess {
   bool IsSyscallBrokerable(int sysno, bool fast_check) const;
 
   // Close the IPC channel with the other party. This should only be used
-  // by tests an none of the class methods should be used afterwards.
+  // by tests and none of the class methods should be used afterwards.
   void CloseChannel();
 
+  // Forks the signal-based broker, where syscall emulation is performed using
+  // signals in the sandboxed process that connect to the broker via Unix
+  // socket.
+  bool ForkSignalBasedBroker(
+      base::OnceCallback<bool(void)> broker_process_init_callback);
+
   bool initialized_;  // Whether we've been through Init() yet.
   pid_t broker_pid_;  // The PID of the broker (child) created in Init().
+  const BrokerType broker_type_;
   const bool fast_check_in_client_;
   const bool quiet_failures_for_tests_;
   syscall_broker::BrokerCommandSet allowed_command_set_;

sandbox/policy/linux/sandbox_linux.cc

@@ -492,7 +492,8 @@ void SandboxLinux::StartBrokerProcess(
     const Options& options) {
   // Leaked at shutdown, so use bare |new|.
   broker_process_ = new syscall_broker::BrokerProcess(
-      BPFBasePolicy::GetFSDeniedErrno(), allowed_command_set, permissions);
+      BPFBasePolicy::GetFSDeniedErrno(), allowed_command_set, permissions,
+      syscall_broker::BrokerProcess::BrokerType::SIGNAL_BASED);
 
   // The initialization callback will perform generic initialization and then
   // call broker_sandboxer_callback.