stack_trace: do not add common libraries when not a custom fork-server

Avoids duplicate entries warnings and tightens the namespace.
Drive-by: modernize the policy.
PiperOrigin-RevId: 503108939
Change-Id: If34d23dd83ca39682799dfb36bd0b9b9ceb19fdc

sandboxed_api/sandbox2/BUILD.bazel

@@ -841,7 +841,7 @@ cc_test(
     tags = ["no_qemu_user_mode"],
     deps = [
         ":global_forkserver",
-        ":mounts",
+        ":namespace",
         ":regs",
         ":sandbox2",
         "//sandboxed_api:testing",

sandboxed_api/sandbox2/CMakeLists.txt

@@ -942,6 +942,7 @@ if(BUILD_TESTING AND SAPI_BUILD_TESTING)
     absl::strings
     sandbox2::bpf_helper
     sandbox2::global_forkserver
+    sandbox2::namespace
     sandbox2::sandbox2
     sandbox2::util
     sapi::fileops

sandboxed_api/sandbox2/monitor.cc

@@ -344,6 +344,7 @@ void Monitor::Run() {
 
   // Get PID of the sandboxee.
   bool should_have_init = ns && (ns->GetCloneFlags() & CLONE_NEWPID);
+  uses_custom_forkserver_ = executor_->fork_client_ != nullptr;
   absl::StatusOr<Executor::Process> process =
       executor_->StartSubProcess(clone_flags, ns, policy_->capabilities());
 
@@ -459,10 +460,9 @@ bool Monitor::ShouldCollectStackTrace() {
 
 absl::StatusOr<std::vector<std::string>> Monitor::GetAndLogStackTrace(
     const Regs* regs) {
-  auto* ns = policy_->GetNamespace();
-  const Mounts empty_mounts;
-  SAPI_ASSIGN_OR_RETURN(std::vector<std::string> stack_trace,
-                        GetStackTrace(regs, ns ? ns->mounts() : empty_mounts));
+  SAPI_ASSIGN_OR_RETURN(
+      std::vector<std::string> stack_trace,
+      GetStackTrace(regs, policy_->GetNamespace(), uses_custom_forkserver_));
 
   LOG(INFO) << "Stack trace: [";
   for (const auto& frame : CompactStackTrace(stack_trace)) {
@@ -1230,7 +1230,8 @@ void Monitor::StateProcessStopped(pid_t pid, int status) {
                         pid]() -> absl::StatusOr<std::vector<std::string>> {
       Regs regs(pid);
       SAPI_RETURN_IF_ERROR(regs.Fetch());
-      return GetStackTrace(&regs, policy_->GetNamespace()->mounts());
+      return GetStackTrace(&regs, policy_->GetNamespace(),
+                           uses_custom_forkserver_);
     }();
 
     if (!stack_trace.ok()) {

sandboxed_api/sandbox2/monitor.h

@@ -201,6 +201,8 @@ class Monitor final {
 
   // Is the sandboxee actively monitored, or maybe we're waiting for execve()?
   bool wait_for_execve_;
+  // Is the sandboxee forked from a custom forkserver?
+  bool uses_custom_forkserver_;
   // Log file specified by
   // --sandbox_danger_danger_permit_all_and_log flag.
   FILE* log_file_ = nullptr;

sandboxed_api/sandbox2/stack_trace.cc

@@ -76,48 +76,69 @@ class StackTracePeer {
   static absl::StatusOr<std::unique_ptr<Policy>> GetPolicy(
       pid_t target_pid, const std::string& maps_file,
       const std::string& app_path, const std::string& exe_path,
-      const Mounts& mounts);
+      const Namespace* ns, bool uses_custom_forkserver);
 
   static absl::StatusOr<UnwindResult> LaunchLibunwindSandbox(
-      const Regs* regs, const Mounts& mounts);
+      const Regs* regs, const Namespace* ns, bool uses_custom_forkserver);
 };
 
 absl::StatusOr<std::unique_ptr<Policy>> StackTracePeer::GetPolicy(
     pid_t target_pid, const std::string& maps_file, const std::string& app_path,
-    const std::string& exe_path, const Mounts& mounts) {
+    const std::string& exe_path, const Namespace* ns,
+    bool uses_custom_forkserver) {
   PolicyBuilder builder;
-  builder
-      // Use the mounttree of the original executable as starting point.
-      .SetMounts(mounts)
-      .AllowOpen()
+  if (uses_custom_forkserver) {
+    // Custom forkserver just forks, the binary is loaded outside of the
+    // sandboxee's mount namespace.
+    // Add all possible libraries without the need of parsing the binary
+    // or /proc/pid/maps.
+    for (const auto& library_path : {
+             "/usr/lib64",
+             "/usr/lib",
+             "/lib64",
+             "/lib",
+         }) {
+      if (access(library_path, F_OK) != -1) {
+        VLOG(1) << "Adding library folder '" << library_path << "'";
+        builder.AddDirectory(library_path);
+      } else {
+        VLOG(1) << "Could not add library folder '" << library_path
+                << "' as it does not exist";
+      }
+    }
+  } else {
+    // Use the mounttree of the original executable.
+    CHECK(ns != nullptr);
+    builder.SetMounts(ns->mounts());
+  }
+  builder.AllowOpen()
       .AllowRead()
       .AllowWrite()
       .AllowSyscall(__NR_close)
-      .AllowMmap()
       .AllowExit()
       .AllowHandleSignals()
+      .AllowTcMalloc()
+      .AllowSystemMalloc()
 
       // libunwind
+      .AllowMmap()
       .AllowStat()
       .AllowSyscall(__NR_lseek)
 #ifdef __NR__llseek
       .AllowSyscall(__NR__llseek)  // Newer glibc on PPC
 #endif
       .AllowSyscall(__NR_mincore)
-      .AllowSyscall(__NR_mprotect)
       .AllowSyscall(__NR_munmap)
-      .AllowSyscall(__NR_pipe2)
+      .AllowPipe()
 
       // Symbolizer
       .AllowSyscall(__NR_brk)
-      .AllowSyscall(__NR_clock_gettime)
+      .AllowTime()
 
       // Other
-      .AllowSyscall(__NR_dup)
-      .AllowSyscall(__NR_fcntl)
-      .AllowSyscall(__NR_getpid)
-      .AllowSyscall(__NR_gettid)
-      .AllowSyscall(__NR_madvise)
+      .AllowDup()
+      .AllowSafeFcntl()
+      .AllowGetPIDs()
 
       // Required for our ptrace replacement.
       .TrapPtrace()
@@ -141,23 +162,6 @@ absl::StatusOr<std::unique_ptr<Policy>> StackTracePeer::GetPolicy(
       // Add the binary itself.
       .AddFileAt(exe_path, app_path);
 
-  // Add all possible libraries without the need of parsing the binary
-  // or /proc/pid/maps.
-  for (const auto& library_path : {
-           "/usr/lib64",
-           "/usr/lib",
-           "/lib64",
-           "/lib",
-       }) {
-    if (access(library_path, F_OK) != -1) {
-      VLOG(1) << "Adding library folder '" << library_path << "'";
-      builder.AddDirectory(library_path);
-    } else {
-      VLOG(1) << "Could not add library folder '" << library_path
-              << "' as it does not exist";
-    }
-  }
-
   SAPI_ASSIGN_OR_RETURN(std::unique_ptr<Policy> policy, builder.TryBuild());
   policy->AllowUnsafeKeepCapabilities({CAP_SYS_PTRACE});
   // Use no special namespace flags when cloning. We will join an existing
@@ -167,7 +171,7 @@ absl::StatusOr<std::unique_ptr<Policy>> StackTracePeer::GetPolicy(
 }
 
 absl::StatusOr<UnwindResult> StackTracePeer::LaunchLibunwindSandbox(
-    const Regs* regs, const Mounts& mounts) {
+    const Regs* regs, const Namespace* ns, bool uses_custom_forkserver) {
   const pid_t pid = regs->pid();
 
   // Tell executor to use this special internal mode. Using `new` to access a
@@ -216,7 +220,8 @@ absl::StatusOr<UnwindResult> StackTracePeer::LaunchLibunwindSandbox(
   // The exe_path will have a mountable path of the application, even if it was
   // removed.
   // Resolve app_path backing file.
-  std::string exe_path = mounts.ResolvePath(app_path).value_or("");
+  std::string exe_path =
+      ns ? ns->mounts().ResolvePath(app_path).value_or("") : "";
 
   if (exe_path.empty()) {
     // File was probably removed.
@@ -233,9 +238,10 @@ absl::StatusOr<UnwindResult> StackTracePeer::LaunchLibunwindSandbox(
 
   // Add mappings for the binary (as they might not have been added due to the
   // forkserver).
-  SAPI_ASSIGN_OR_RETURN(std::unique_ptr<Policy> policy,
-                        StackTracePeer::GetPolicy(pid, unwind_temp_maps_path,
-                                                  app_path, exe_path, mounts));
+  SAPI_ASSIGN_OR_RETURN(
+      std::unique_ptr<Policy> policy,
+      StackTracePeer::GetPolicy(pid, unwind_temp_maps_path, app_path, exe_path,
+                                ns, uses_custom_forkserver));
   Sandbox2 sandbox(std::move(executor), std::move(policy));
 
   VLOG(1) << "Running libunwind sandbox";
@@ -283,8 +289,8 @@ absl::StatusOr<UnwindResult> StackTracePeer::LaunchLibunwindSandbox(
   return result;
 }
 
-absl::StatusOr<std::vector<std::string>> GetStackTrace(const Regs* regs,
-                                                       const Mounts& mounts) {
+absl::StatusOr<std::vector<std::string>> GetStackTrace(
+    const Regs* regs, const Namespace* ns, bool uses_custom_forkserver) {
   if (absl::GetFlag(FLAGS_sandbox_disable_all_stack_traces)) {
     return absl::UnavailableError("Stacktraces disabled");
   }
@@ -310,8 +316,9 @@ absl::StatusOr<std::vector<std::string>> GetStackTrace(const Regs* regs,
     return UnsafeGetStackTrace(regs->pid());
   }
 
-  SAPI_ASSIGN_OR_RETURN(UnwindResult res,
-                        StackTracePeer::LaunchLibunwindSandbox(regs, mounts));
+  SAPI_ASSIGN_OR_RETURN(
+      UnwindResult res,
+      StackTracePeer::LaunchLibunwindSandbox(regs, ns, uses_custom_forkserver));
   return std::vector<std::string>(res.stacktrace().begin(),
                                   res.stacktrace().end());
 }

sandboxed_api/sandbox2/stack_trace.h

@@ -23,7 +23,7 @@
 #include <vector>
 
 #include "absl/status/statusor.h"
-#include "sandboxed_api/sandbox2/mounts.h"
+#include "sandboxed_api/sandbox2/namespace.h"
 #include "sandboxed_api/sandbox2/regs.h"
 
 namespace sandbox2 {
@@ -32,8 +32,8 @@ namespace sandbox2 {
 constexpr size_t kDefaultMaxFrames = 200;
 
 // Returns the stack-trace of the PID=pid, one line per frame.
-absl::StatusOr<std::vector<std::string>> GetStackTrace(const Regs* regs,
-                                                       const Mounts& mounts);
+absl::StatusOr<std::vector<std::string>> GetStackTrace(
+    const Regs* regs, const Namespace* ns, bool uses_custom_forkserver);
 
 // Returns a stack trace that collapses duplicate stack frames and annotates
 // them with a repetition count.