Tighten up broker-side BPF policy based on command set.

Remove unused argument |policy| from callbacks.

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Id9f314616b55d774bd0b3af5b66abfa14683c788
Reviewed-on: https://chromium-review.googlesource.com/809919Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522180}

content/gpu/gpu_sandbox_hook_linux.cc

@@ -333,7 +333,6 @@ sandbox::syscall_broker::BrokerCommandSet CommandSetForGPU(
 }
 
 bool BrokerProcessPreSandboxHook(
-    service_manager::BPFBasePolicy* policy,
     service_manager::SandboxLinux::Options options) {
   // Oddly enough, we call back into gpu to invoke this service manager
   // method, since it is part of the embedder component, and the service
@@ -344,10 +343,9 @@ bool BrokerProcessPreSandboxHook(
 
 }  // namespace
 
-bool GpuProcessPreSandboxHook(service_manager::BPFBasePolicy* policy,
-                              service_manager::SandboxLinux::Options options) {
+bool GpuProcessPreSandboxHook(service_manager::SandboxLinux::Options options) {
   service_manager::SandboxLinux::GetInstance()->StartBrokerProcess(
-      policy, CommandSetForGPU(options), FilePermissionsForGpu(options),
+      CommandSetForGPU(options), FilePermissionsForGpu(options),
       base::BindOnce(BrokerProcessPreSandboxHook), options);
 
   if (!LoadLibrariesForGpu(options))

content/gpu/gpu_sandbox_hook_linux.h

@@ -9,8 +9,7 @@
 
 namespace content {
 
-bool GpuProcessPreSandboxHook(service_manager::BPFBasePolicy* policy,
-                              service_manager::SandboxLinux::Options options);
+bool GpuProcessPreSandboxHook(service_manager::SandboxLinux::Options options);
 
 }  // namespace content
 

content/network/network_sandbox_hook_linux.cc

@@ -12,8 +12,7 @@ using sandbox::syscall_broker::BrokerFilePermission;
 
 namespace content {
 
-bool NetworkPreSandboxHook(service_manager::BPFBasePolicy* policy,
-                           service_manager::SandboxLinux::Options options) {
+bool NetworkPreSandboxHook(service_manager::SandboxLinux::Options options) {
   sandbox::syscall_broker::BrokerCommandSet command_set;
   command_set.set(sandbox::syscall_broker::COMMAND_ACCESS);
   command_set.set(sandbox::syscall_broker::COMMAND_OPEN);
@@ -26,7 +25,7 @@ bool NetworkPreSandboxHook(service_manager::BPFBasePolicy* policy,
       BrokerFilePermission::ReadWriteCreateRecursive("/")};
 
   service_manager::SandboxLinux::GetInstance()->StartBrokerProcess(
-      policy, command_set, std::move(file_permissions),
+      command_set, std::move(file_permissions),
       service_manager::SandboxLinux::PreSandboxHook(), options);
 
   return true;

content/network/network_sandbox_hook_linux.h

@@ -9,8 +9,7 @@
 
 namespace content {
 
-bool NetworkPreSandboxHook(service_manager::BPFBasePolicy* policy,
-                           service_manager::SandboxLinux::Options options);
+bool NetworkPreSandboxHook(service_manager::SandboxLinux::Options options);
 
 }  // namespace content
 

services/service_manager/sandbox/linux/bpf_broker_policy_linux.cc

@@ -12,7 +12,9 @@ using sandbox::bpf_dsl::ResultExpr;
 
 namespace service_manager {
 
-BrokerProcessPolicy::BrokerProcessPolicy() {}
+BrokerProcessPolicy::BrokerProcessPolicy(
+    const sandbox::syscall_broker::BrokerCommandSet& allowed_command_set)
+    : allowed_command_set_(allowed_command_set) {}
 
 BrokerProcessPolicy::~BrokerProcessPolicy() {}
 
@@ -20,44 +22,78 @@ ResultExpr BrokerProcessPolicy::EvaluateSyscall(int sysno) const {
   switch (sysno) {
 #if defined(__NR_access)
     case __NR_access:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_ACCESS))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_faccessat)
     case __NR_faccessat:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_ACCESS))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_open)
     case __NR_open:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_OPEN))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_openat)
     case __NR_openat:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_OPEN))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_unlink)
     case __NR_unlink:
+      return Allow();
 #endif
 #if defined(__NR_rename)
     case __NR_rename:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_RENAME))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_stat)
     case __NR_stat:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_STAT))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_stat64)
     case __NR_stat64:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_STAT))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_fstatat)
     case __NR_fstatat:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_STAT))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_newfstatat)
     case __NR_newfstatat:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_STAT))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_readlink)
     case __NR_readlink:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_READLINK))
+        return Allow();
+      break;
 #endif
 #if defined(__NR_readlinkat)
     case __NR_readlinkat:
+      if (allowed_command_set_.test(sandbox::syscall_broker::COMMAND_READLINK))
+        return Allow();
+      break;
 #endif
-      return Allow();
     default:
-      return BPFBasePolicy::EvaluateSyscall(sysno);
+      break;
   }
+  return BPFBasePolicy::EvaluateSyscall(sysno);
 }
 
 }  // namespace service_manager

services/service_manager/sandbox/linux/bpf_broker_policy_linux.h

@@ -6,6 +6,7 @@
 #define SERVICES_SERVICE_MANAGER_SANDBOX_LINUX_BPF_BROKER_POLICY_LINUX_H_
 
 #include "sandbox/linux/bpf_dsl/bpf_dsl.h"
+#include "sandbox/linux/syscall_broker/broker_command.h"
 #include "services/service_manager/sandbox/export.h"
 #include "services/service_manager/sandbox/linux/bpf_base_policy_linux.h"
 
@@ -16,13 +17,16 @@ namespace service_manager {
 class SERVICE_MANAGER_SANDBOX_EXPORT BrokerProcessPolicy
     : public BPFBasePolicy {
  public:
-  BrokerProcessPolicy();
+  explicit BrokerProcessPolicy(
+      const sandbox::syscall_broker::BrokerCommandSet& allowed_command_set);
   ~BrokerProcessPolicy() override;
 
   sandbox::bpf_dsl::ResultExpr EvaluateSyscall(
       int system_call_number) const override;
 
  private:
+  const sandbox::syscall_broker::BrokerCommandSet allowed_command_set_;
+
   DISALLOW_COPY_AND_ASSIGN(BrokerProcessPolicy);
 };
 

services/service_manager/sandbox/linux/sandbox_linux.cc

@@ -40,6 +40,7 @@
 #include "sandbox/linux/services/thread_helpers.h"
 #include "sandbox/linux/services/yama.h"
 #include "sandbox/linux/suid/client/setuid_sandbox_client.h"
+#include "sandbox/linux/syscall_broker/broker_command.h"
 #include "sandbox/linux/syscall_broker/broker_process.h"
 #include "sandbox/sandbox_features.h"
 #include "services/service_manager/sandbox/linux/bpf_broker_policy_linux.h"
@@ -96,9 +97,9 @@ base::ScopedFD OpenProc(int proc_fd) {
 }
 
 bool UpdateProcessTypeAndEnableSandbox(
-    BPFBasePolicy* client_sandbox_policy,
     SandboxLinux::PreSandboxHook broker_side_hook,
-    SandboxLinux::Options options) {
+    SandboxLinux::Options options,
+    sandbox::syscall_broker::BrokerCommandSet allowed_command_set) {
   base::CommandLine::StringVector exec =
       base::CommandLine::ForCurrentProcess()->GetArgs();
   base::CommandLine::Reset();
@@ -111,12 +112,12 @@ bool UpdateProcessTypeAndEnableSandbox(
       command_line->GetSwitchValueASCII(switches::kProcessType)
           .append("-broker"));
 
-  auto broker_side_policy = std::make_unique<BrokerProcessPolicy>();
   if (broker_side_hook)
-    CHECK(std::move(broker_side_hook).Run(broker_side_policy.get(), options));
+    CHECK(std::move(broker_side_hook).Run(options));
 
   return SandboxSeccompBPF::StartSandboxWithExternalPolicy(
-      std::move(broker_side_policy), base::ScopedFD());
+      std::make_unique<BrokerProcessPolicy>(allowed_command_set),
+      base::ScopedFD());
 }
 
 }  // namespace
@@ -299,14 +300,13 @@ bool SandboxLinux::StartSeccompBPF(SandboxType sandbox_type,
     return true;
   }
 
+  if (hook)
+    CHECK(std::move(hook).Run(options));
+
   // If the kernel supports the sandbox, and if the command line says we
   // should enable it, enable it or die.
   std::unique_ptr<BPFBasePolicy> policy =
       SandboxSeccompBPF::PolicyForSandboxType(sandbox_type, options);
-
-  if (hook)
-    CHECK(std::move(hook).Run(policy.get(), options));
-
   SandboxSeccompBPF::StartSandboxWithExternalPolicy(std::move(policy),
                                                     OpenProc(proc_fd_));
   SandboxSeccompBPF::RunSandboxSanityChecks(sandbox_type, options);
@@ -479,7 +479,6 @@ bool SandboxLinux::LimitAddressSpace(const std::string& process_type,
 }
 
 void SandboxLinux::StartBrokerProcess(
-    BPFBasePolicy* client_sandbox_policy,
     const sandbox::syscall_broker::BrokerCommandSet& allowed_command_set,
     std::vector<sandbox::syscall_broker::BrokerFilePermission> permissions,
     PreSandboxHook broker_side_hook,
@@ -492,8 +491,8 @@ void SandboxLinux::StartBrokerProcess(
   // call broker_sandboxer_callback.
   CHECK(broker_process_->Init(
       base::Bind(&UpdateProcessTypeAndEnableSandbox,
-                 base::Unretained(client_sandbox_policy),
-                 base::Passed(std::move(broker_side_hook)), options)));
+                 base::Passed(std::move(broker_side_hook)), options,
+                 allowed_command_set)));
 }
 
 bool SandboxLinux::HasOpenDirectories() const {

services/service_manager/sandbox/linux/sandbox_linux.h

@@ -108,7 +108,7 @@ class SERVICE_MANAGER_SANDBOX_EXPORT SandboxLinux {
   // is passed to the BPF compiler and the sandbox is engaged. If
   // pre_sandbox_hook() returns true, the sandbox will be engaged
   // afterwards, otherwise the process is terminated.
-  using PreSandboxHook = base::OnceCallback<bool(BPFBasePolicy*, Options)>;
+  using PreSandboxHook = base::OnceCallback<bool(Options)>;
 
   // Get our singleton instance.
   static SandboxLinux* GetInstance();
@@ -209,7 +209,6 @@ class SERVICE_MANAGER_SANDBOX_EXPORT SandboxLinux {
   // This should never be destroyed, as after the sandbox is started it is
   // vital to the process.
   void StartBrokerProcess(
-      BPFBasePolicy* client_sandbox_policy,
       const sandbox::syscall_broker::BrokerCommandSet& allowed_command_set,
       std::vector<sandbox::syscall_broker::BrokerFilePermission> permissions,
       PreSandboxHook broker_side_hook,