Reland "Add a new BPF helper syscall_broker::BrokerProcess::IsSyscallAllowed()."

This is a reland of 7217c9c4

Original change's description:
> Add a new BPF helper syscall_broker::BrokerProcess::IsSyscallAllowed().
> 
> This helper can be used in bpf_dsl::Policy::EvaluateSyscall() to make it
> easier to craft BPF policies that trap to the broker process.
> 
> Change-Id: I8588b0a934343d7d0009c0d07bf259a085451ece
> Reviewed-on: https://chromium-review.googlesource.com/1175934
> Commit-Queue: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Tom Sepez <tsepez@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#583407}

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

sandbox/linux/syscall_broker/broker_process.cc

@@ -103,6 +103,80 @@ bool BrokerProcess::Init(
   return false;
 }
 
+bool BrokerProcess::IsSyscallAllowed(int sysno) const {
+  switch (sysno) {
+#if !defined(__aarch64__)
+    case __NR_access:
+#endif
+    case __NR_faccessat:
+      return !fast_check_in_client_ ||
+             allowed_command_set_.test(COMMAND_ACCESS);
+
+#if !defined(__aarch64__)
+    case __NR_mkdir:
+#endif
+    case __NR_mkdirat:
+      return !fast_check_in_client_ || allowed_command_set_.test(COMMAND_MKDIR);
+
+#if !defined(__aarch64__)
+    case __NR_open:
+#endif
+    case __NR_openat:
+      return !fast_check_in_client_ || allowed_command_set_.test(COMMAND_OPEN);
+
+#if !defined(__aarch64__)
+    case __NR_readlink:
+#endif
+    case __NR_readlinkat:
+      return !fast_check_in_client_ ||
+             allowed_command_set_.test(COMMAND_READLINK);
+
+#if !defined(__aarch64__)
+    case __NR_rename:
+#endif
+    case __NR_renameat:
+    case __NR_renameat2:
+      return !fast_check_in_client_ ||
+             allowed_command_set_.test(COMMAND_RENAME);
+
+#if !defined(__aarch64__)
+    case __NR_rmdir:
+      return !fast_check_in_client_ || allowed_command_set_.test(COMMAND_RMDIR);
+#endif
+
+#if !defined(__aarch64__)
+    case __NR_stat:
+    case __NR_lstat:
+#endif
+#if defined(__NR_fstatat)
+    case __NR_fstatat:
+#endif
+#if defined(__x86_64__) || defined(__aarch64__)
+    case __NR_newfstatat:
+#endif
+      return !fast_check_in_client_ || allowed_command_set_.test(COMMAND_STAT);
+
+#if defined(__i386__) || defined(__arm__) || defined(__mips32__)
+    case __NR_stat64:
+    case __NR_lstat64:
+      // For security purposes, map stat64 to COMMAND_STAT permission. The
+      // separate COMMAND_STAT64 only exists to broker different-sized
+      // argument structs.
+      return !fast_check_in_client_ || allowed_command_set_.test(COMMAND_STAT);
+#endif
+
+#if !defined(__aarch64__)
+    case __NR_unlink:
+#endif
+    case __NR_unlinkat:
+      return !fast_check_in_client_ ||
+             allowed_command_set_.test(COMMAND_UNLINK);
+
+    default:
+      return false;
+  }
+}
+
 void BrokerProcess::CloseChannel() {
   broker_client_.reset();
 }

sandbox/linux/syscall_broker/broker_process.h

@@ -74,6 +74,14 @@ class SANDBOX_EXPORT BrokerProcess {
   // Return the PID of the child created by Init().
   int broker_pid() const { return broker_pid_; }
 
+  // Can be used in bpf_dsl::Policy::EvaluateSyscall() implementations to
+  // determine if the system call |sysno| should be trapped and forwarded
+  // to the broker process for handling. This examines the
+  // |allowed_command_set_| iff |fast_check_in_client_| is true. If
+  // the fast checks are disabled, then all possible brokerable system
+  // calls are forwarded to the broker process for handling.
+  bool IsSyscallAllowed(int sysno) const;
+
   // The following methods are used in place of the equivalently-named
   // syscalls by the trap handler. They, in turn, forward the call onto
   // |broker_client_| for further processing. They will all be async signal

sandbox/linux/syscall_broker/broker_process_unittest.cc

@@ -27,6 +27,8 @@
 #include "base/macros.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/posix/unix_domain_socket.h"
+#include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "sandbox/linux/syscall_broker/broker_client.h"
 #include "sandbox/linux/tests/scoped_temporary_file.h"
@@ -1452,5 +1454,85 @@ TEST(BrokerProcess, UnlinkHost) {
   TestUnlinkHelper(false);
 }
 
+TEST(BrokerProcess, IsSyscallAllowed) {
+  const struct {
+    int sysno;
+    BrokerCommand command;
+  } kSyscallToCommandMap[] = {
+#if defined(__NR_access)
+    {__NR_access, COMMAND_ACCESS},
+#endif
+    {__NR_faccessat, COMMAND_ACCESS},
+#if defined(__NR_mkdir)
+    {__NR_mkdir, COMMAND_MKDIR},
+#endif
+    {__NR_mkdirat, COMMAND_MKDIR},
+#if defined(__NR_open)
+    {__NR_open, COMMAND_OPEN},
+#endif
+    {__NR_openat, COMMAND_OPEN},
+#if defined(__NR_readlink)
+    {__NR_readlink, COMMAND_READLINK},
+#endif
+    {__NR_readlinkat, COMMAND_READLINK},
+#if defined(__NR_rename)
+    {__NR_rename, COMMAND_RENAME},
+#endif
+    {__NR_renameat, COMMAND_RENAME},
+#if defined(__NR_rmdir)
+    {__NR_rmdir, COMMAND_RMDIR},
+#endif
+#if defined(__NR_stat)
+    {__NR_stat, COMMAND_STAT},
+#endif
+#if defined(__NR_lstat)
+    {__NR_lstat, COMMAND_STAT},
+#endif
+#if defined(__NR_fstatat)
+    {__NR_fstatat, COMMAND_STAT},
+#endif
+#if defined(__NR_newfstatat)
+    {__NR_newfstatat, COMMAND_STAT},
+#endif
+#if defined(__NR_stat64)
+    {__NR_stat64, COMMAND_STAT},
+#endif
+#if defined(__NR_lstat64)
+    {__NR_lstat64, COMMAND_STAT},
+#endif
+#if defined(__NR_unlink)
+    {__NR_unlink, COMMAND_UNLINK},
+#endif
+    {__NR_unlinkat, COMMAND_UNLINK},
+  };
+
+  for (const auto& test : kSyscallToCommandMap) {
+    // Test with fast_check_in_client.
+    {
+      SCOPED_TRACE(base::StringPrintf("fast check, sysno=%d", test.sysno));
+      BrokerProcess process(ENOSYS, MakeBrokerCommandSet({test.command}), {},
+                            true, true);
+      EXPECT_TRUE(process.IsSyscallAllowed(test.sysno));
+      for (const auto& other : kSyscallToCommandMap) {
+        SCOPED_TRACE(base::StringPrintf("others test, sysno=%d", other.sysno));
+        EXPECT_EQ(other.command == test.command,
+                  process.IsSyscallAllowed(other.sysno));
+      }
+    }
+
+    // Test without fast_check_in_client.
+    {
+      SCOPED_TRACE(base::StringPrintf("no fast check, sysno=%d", test.sysno));
+      BrokerProcess process(ENOSYS, MakeBrokerCommandSet({test.command}), {},
+                            false, true);
+      EXPECT_TRUE(process.IsSyscallAllowed(test.sysno));
+      for (const auto& other : kSyscallToCommandMap) {
+        SCOPED_TRACE(base::StringPrintf("others test, sysno=%d", other.sysno));
+        EXPECT_TRUE(process.IsSyscallAllowed(other.sysno));
+      }
+    }
+  }
+}
+
 }  // namespace syscall_broker
 }  // namespace sandbox

services/service_manager/sandbox/linux/bpf_gpu_policy_linux.cc

@@ -55,29 +55,6 @@ ResultExpr GpuProcessPolicy::EvaluateSyscall(int sysno) const {
     case __NR_prctl:
     case __NR_sysinfo:
       return Allow();
-#if !defined(__aarch64__)
-    case __NR_access:
-    case __NR_open:
-#endif  // !defined(__aarch64__)
-    case __NR_faccessat:
-    case __NR_openat:
-#if defined(__NR_stat)
-    case __NR_stat:
-#endif
-#if defined(__NR_stat64)
-    case __NR_stat64:
-#endif
-#if defined(__NR_fstatat)
-    case __NR_fstatat:
-#endif
-#if defined(__NR_newfstatat)
-    case __NR_newfstatat:
-#endif
-    {
-      auto* broker_process = SandboxLinux::GetInstance()->broker_process();
-      DCHECK(broker_process);
-      return Trap(BrokerProcess::SIGSYS_Handler, broker_process);
-    }
     case __NR_sched_getaffinity:
     case __NR_sched_setaffinity:
       return sandbox::RestrictSchedTarget(GetPolicyPid(), sysno);
@@ -85,6 +62,11 @@ ResultExpr GpuProcessPolicy::EvaluateSyscall(int sysno) const {
       if (SyscallSets::IsEventFd(sysno))
         return Allow();
 
+      auto* broker_process = SandboxLinux::GetInstance()->broker_process();
+      if (broker_process->IsSyscallAllowed(sysno)) {
+        return Trap(BrokerProcess::SIGSYS_Handler, broker_process);
+      }
+
       // Default on the baseline policy.
       return BPFBasePolicy::EvaluateSyscall(sysno);
   }

services/service_manager/sandbox/linux/bpf_network_policy_linux.cc

@@ -33,70 +33,13 @@ NetworkProcessPolicy::NetworkProcessPolicy() {}
 NetworkProcessPolicy::~NetworkProcessPolicy() {}
 
 ResultExpr NetworkProcessPolicy::EvaluateSyscall(int sysno) const {
-  switch (sysno) {
-#if defined(__NR_access)
-    case __NR_access:
-#endif
-#if defined(__NR_faccessat)
-    case __NR_faccessat:
-#endif
-#if defined(__NR_mkdir)
-    case __NR_mkdir:
-#endif
-#if defined(__NR_mkdirat)
-    case __NR_mkdirat:
-#endif
-#if defined(__NR_open)
-    case __NR_open:
-#endif
-#if defined(__NR_openat)
-    case __NR_openat:
-#endif
-#if defined(__NR_readlink)
-    case __NR_readlink:
-#endif
-#if defined(__NR_readlinkat)
-    case __NR_readlinkat:
-#endif
-#if defined(__NR_rmdir)
-    case __NR_rmdir:
-#endif
-#if defined(__NR_rename)
-    case __NR_rename:
-#endif
-#if defined(__NR_renameat)
-    case __NR_renameat:
-#endif
-#if defined(__NR_stat)
-    case __NR_stat:
-#endif
-#if defined(__NR_stat64)
-    case __NR_stat64:
-#endif
-#if defined(__NR_lstat)
-    case __NR_lstat:
-#endif
-#if defined(__NR_lstat64)
-    case __NR_lstat64:
-#endif
-#if defined(__NR_fstatat)
-    case __NR_fstatat:
-#endif
-#if defined(__NR_newfstatat)
-    case __NR_newfstatat:
-#endif
-#if defined(__NR_unlink)
-    case __NR_unlink:
-#endif
-#if defined(__NR_unlinkat)
-    case __NR_unlinkat:
-#endif
-      return Trap(BrokerProcess::SIGSYS_Handler,
-                  SandboxLinux::GetInstance()->broker_process());
-    default:
-      // TODO(tsepez): FIX this.
-      return Allow();
+  auto* broker_process = SandboxLinux::GetInstance()->broker_process();
+  if (broker_process->IsSyscallAllowed(sysno)) {
+    return Trap(BrokerProcess::SIGSYS_Handler, broker_process);
   }
+
+  // TODO(tsepez): FIX this.
+  return Allow();
 }
 
 }  // namespace service_manager