Ability to inspect a syscall's return value.

PiperOrigin-RevId: 421552017
Change-Id: I7103720723b5e5828f80731a724c5672895dfa54

sandboxed_api/sandbox2/BUILD.bazel

@@ -46,7 +46,6 @@ cc_library(
         ":violation_cc_proto",
         "//sandboxed_api:config",
         "//sandboxed_api/util:status",
-        "//sandboxed_api/util:strerror",
         "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/strings",
@@ -197,6 +196,10 @@ cc_library(
         ":comms",
         ":result",
         ":syscall",
+        ":util",
+        "//sandboxed_api:config",
+        "@com_google_absl//absl/base:core_headers",
+        "@com_google_glog//:glog",
     ],
 )
 
@@ -347,7 +350,6 @@ cc_library(
         "//sandboxed_api/util:file_helpers",
         "//sandboxed_api/util:fileops",
         "//sandboxed_api/util:flags",
-        "//sandboxed_api/util:raw_logging",
         "//sandboxed_api/util:status",
         "//sandboxed_api/util:strerror",
         "//sandboxed_api/util:temp_file",

sandboxed_api/sandbox2/CMakeLists.txt

@@ -170,11 +170,15 @@ add_library(sandbox2_notify ${SAPI_LIB_TYPE}
   notify.h
 )
 add_library(sandbox2::notify ALIAS sandbox2_notify)
-target_link_libraries(sandbox2_notify PRIVATE
-  sandbox2::comms
-  sandbox2::result
-  sandbox2::syscall
-  sapi::base
+target_link_libraries(sandbox2_notify
+  PUBLIC absl::core_headers
+         glog::glog
+         sandbox2::comms
+         sandbox2::result
+         sandbox2::syscall
+         sandbox2::util
+  PRIVATE sapi::base
+          sapi::config
 )
 
 # sandboxed_api/sandbox2:limits
@@ -296,7 +300,6 @@ add_library(sandbox2::sandbox2 ALIAS sandbox2_sandbox2)
 target_link_libraries(sandbox2_sandbox2
   PRIVATE absl::core_headers
           absl::cleanup
-          absl::flat_hash_map
           absl::flat_hash_set
           absl::memory
           absl::optional
@@ -305,7 +308,8 @@ target_link_libraries(sandbox2_sandbox2
           absl::strings
           sapi::strerror
           sapi::base
-  PUBLIC  absl::status
+  PUBLIC  absl::flat_hash_map
+          absl::status
           absl::statusor
           absl::synchronization
           absl::time

sandboxed_api/sandbox2/monitor.cc

@@ -127,6 +127,18 @@ void StopProcess(pid_t pid, int signo) {
   }
 }
 
+void CompleteSyscall(pid_t pid, int signo) {
+  if (ptrace(PTRACE_SYSCALL, pid, 0, signo) == -1) {
+    if (errno == ESRCH) {
+      LOG(WARNING) << "Process " << pid
+                   << " died while trying to PTRACE_SYSCALL it";
+    } else {
+      PLOG(ERROR) << "ptrace(PTRACE_SYSCALL, pid=" << pid << ", sig=" << signo
+                  << ")";
+    }
+  }
+}
+
 void MaybeEnableTomoyoLsmWorkaround(Mounts& mounts, std::string& comms_fd_dev) {
   static auto tomoyo_active = []() -> bool {
     std::string lsm_list;
@@ -768,14 +780,20 @@ void Monitor::ActionProcessSyscall(Regs* regs, const Syscall& syscall) {
   // Notify can decide whether we want to allow this syscall. It could be useful
   // for sandbox setups in which some syscalls might still need some logging,
   // but nonetheless be allowed ('permissible syscalls' in sandbox v1).
-  if (notify_->EventSyscallTrap(syscall)) {
-    LOG(WARNING) << "[PERMITTED]: SYSCALL ::: PID: " << regs->pid()
-                 << ", PROG: '" << util::GetProgName(regs->pid())
-                 << "' : " << syscall.GetDescription();
-
+  auto trace_response = notify_->EventSyscallTrace(syscall);
+  if (trace_response == Notify::TraceAction::kAllow) {
     ContinueProcess(regs->pid(), 0);
     return;
   }
+  if (trace_response == Notify::TraceAction::kInspectAfterReturn) {
+    // Note that a process might die without an exit-stop before the syscall is
+    // completed (eg. a thread calls execve() and the thread group leader dies),
+    // so this entry might never get removed from the table. This may increase
+    // the monitor's memory usage by O(number-of-sandboxed-pids).
+    syscalls_in_progress_[regs->pid()] = syscall;
+    CompleteSyscall(regs->pid(), 0);
+    return;
+  }
 
   // TODO(wiktorg): Further clean that up, probably while doing monitor cleanup
   // log_file_ not null iff FLAGS_sandbox2_danger_danger_permit_all_and_log is
@@ -856,11 +874,74 @@ void Monitor::EventPtraceSeccomp(pid_t pid, int event_msg) {
   ActionProcessSyscall(&regs, syscall);
 }
 
+void Monitor::EventSyscallExit(pid_t pid) {
+  // Check that the monitor wants to inspect the current syscall's return value.
+  auto index = syscalls_in_progress_.find(pid);
+  if (index == syscalls_in_progress_.end()) {
+    LOG(ERROR) << "Expected a syscall in progress in PID " << pid;
+    SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_INSPECT);
+    return;
+  }
+  Regs regs(pid);
+  auto status = regs.Fetch();
+  if (!status.ok()) {
+    LOG(ERROR) << status;
+    SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_FETCH);
+    return;
+  }
+  int64_t return_value = regs.GetReturnValue(sapi::host_cpu::Architecture());
+  notify_->EventSyscallReturn(index->second, return_value);
+  syscalls_in_progress_.erase(index);
+  ContinueProcess(pid, 0);
+}
+
+void Monitor::EventPtraceNewProcess(pid_t pid, int event_msg) {
+  // ptrace doesn't issue syscall-exit-stops for successful fork/vfork/clone
+  // system calls. Check if the monitor wanted to inspect the syscall's return
+  // value, and call EventSyscallReturn for the parent process if so.
+  auto index = syscalls_in_progress_.find(pid);
+  if (index != syscalls_in_progress_.end()) {
+    auto syscall_nr = index->second.nr();
+    bool creating_new_process = syscall_nr == __NR_clone;
+#ifdef __NR_fork
+    creating_new_process = creating_new_process || syscall_nr == __NR_fork;
+#endif
+#ifdef __NR_vfork
+    creating_new_process = creating_new_process || syscall_nr == __NR_vfork;
+#endif
+    if (!creating_new_process) {
+      LOG(ERROR) << "Expected a fork/vfork/clone syscall in progress in PID "
+                 << pid << "; actual: " << index->second.GetDescription();
+      SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_INSPECT);
+      return;
+    }
+    notify_->EventSyscallReturn(index->second, event_msg);
+    syscalls_in_progress_.erase(index);
+  }
+  ContinueProcess(pid, 0);
+}
+
 void Monitor::EventPtraceExec(pid_t pid, int event_msg) {
   if (!IsActivelyMonitoring()) {
     VLOG(1) << "PTRACE_EVENT_EXEC seen from PID: " << event_msg
             << ". SANDBOX ENABLED!";
     SetActivelyMonitoring();
+  } else {
+    // ptrace doesn't issue syscall-exit-stops for successful execve/execveat
+    // system calls. Check if the monitor wanted to inspect the syscall's return
+    // value, and call EventSyscallReturn if so.
+    auto index = syscalls_in_progress_.find(pid);
+    if (index != syscalls_in_progress_.end()) {
+      auto syscall_nr = index->second.nr();
+      if (syscall_nr != __NR_execve && syscall_nr != __NR_execveat) {
+        LOG(ERROR) << "Expected an execve/execveat syscall in progress in PID "
+                   << pid << "; actual: " << index->second.GetDescription();
+        SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_INSPECT);
+        return;
+      }
+      notify_->EventSyscallReturn(index->second, 0);
+      syscalls_in_progress_.erase(index);
+    }
   }
   ContinueProcess(pid, 0);
 }
@@ -932,7 +1013,10 @@ void Monitor::EventPtraceStop(pid_t pid, int stopsig) {
 
 void Monitor::StateProcessStopped(pid_t pid, int status) {
   int stopsig = WSTOPSIG(status);
-  if (__WPTRACEEVENT(status) == 0) {
+  // We use PTRACE_O_TRACESYSGOOD, so we can tell it's a syscall stop without
+  // calling PTRACE_GETSIGINFO by checking the value of the reported signal.
+  bool is_syscall_exit = stopsig == (SIGTRAP | 0x80);
+  if (__WPTRACEEVENT(status) == 0 && !is_syscall_exit) {
     // Must be a regular signal delivery.
     VLOG(2) << "PID: " << pid
             << " received signal: " << util::GetSignalName(stopsig);
@@ -980,13 +1064,25 @@ void Monitor::StateProcessStopped(pid_t pid, int status) {
 #define PTRACE_EVENT_STOP 128
 #endif
 
+  if (is_syscall_exit) {
+    VLOG(2) << "PID: " << pid << " syscall-exit-stop: " << event_msg;
+    EventSyscallExit(pid);
+    return;
+  }
+
   switch (__WPTRACEEVENT(status)) {
     case PTRACE_EVENT_FORK:
-      /* fall through */
+      VLOG(2) << "PID: " << pid << " PTRACE_EVENT_FORK, PID: " << event_msg;
+      EventPtraceNewProcess(pid, event_msg);
+      break;
     case PTRACE_EVENT_VFORK:
-      /* fall through */
+      VLOG(2) << "PID: " << pid << " PTRACE_EVENT_VFORK, PID: " << event_msg;
+      EventPtraceNewProcess(pid, event_msg);
+      break;
     case PTRACE_EVENT_CLONE:
-      /* fall through */
+      VLOG(2) << "PID: " << pid << " PTRACE_EVENT_CLONE, PID: " << event_msg;
+      EventPtraceNewProcess(pid, event_msg);
+      break;
     case PTRACE_EVENT_VFORK_DONE:
       ContinueProcess(pid, 0);
       break;

sandboxed_api/sandbox2/monitor.h

@@ -28,6 +28,7 @@
 #include <memory>
 #include <thread>
 
+#include "absl/container/flat_hash_map.h"
 #include "absl/synchronization/notification.h"
 #include "sandboxed_api/sandbox2/comms.h"
 #include "sandboxed_api/sandbox2/executor.h"
@@ -135,12 +136,18 @@ class Monitor final {
   // Processes exit path.
   void EventPtraceExit(pid_t pid, int event_msg);
 
-  // Processes excution path.
+  // Processes fork/vfork/clone path.
+  void EventPtraceNewProcess(pid_t pid, int event_msg);
+
+  // Processes execution path.
   void EventPtraceExec(pid_t pid, int event_msg);
 
   // Processes stop path.
   void EventPtraceStop(pid_t pid, int stopsig);
 
+  // Processes syscall exit.
+  void EventSyscallExit(pid_t pid);
+
   // Enable network proxy server, this will start a thread in the sandbox
   // that waits for connection requests from the sandboxee.
   void EnableNetworkProxyServer();
@@ -196,6 +203,9 @@ class Monitor final {
   std::unique_ptr<NetworkProxyServer> network_proxy_server_;
 
   std::thread network_proxy_thread_;
+
+  // Syscalls that are running, whose result values we want to inspect.
+  absl::flat_hash_map<pid_t, Syscall> syscalls_in_progress_;
 };
 
 }  // namespace sandbox2

sandboxed_api/sandbox2/notify.h

@@ -19,9 +19,13 @@
 
 #include <sys/types.h>
 
+#include <glog/logging.h>
+#include "absl/base/attributes.h"
+#include "sandboxed_api/config.h"
 #include "sandboxed_api/sandbox2/comms.h"
 #include "sandboxed_api/sandbox2/result.h"
 #include "sandboxed_api/sandbox2/syscall.h"
+#include "sandboxed_api/sandbox2/util.h"
 
 namespace sandbox2 {
 
@@ -50,11 +54,41 @@ class Notify {
   virtual void EventSyscallViolation(const Syscall& syscall,
                                      ViolationType type) {}
 
-  // Called when a policy called TRACE. The syscall is allowed if this method
-  // returns true.
-  // This allows for implementing 'log, but allow' policies.
+  // Called when a policy called TRACE. The syscall is allowed and logged if
+  // this method returns true. This allows for implementing 'log, but allow'
+  // policies.
+  ABSL_DEPRECATED("Override EventSyscallTrace() instead")
   virtual bool EventSyscallTrap(const Syscall& syscall) { return false; }
 
+  // Actions to perform after calling EventSyscallTrace.
+  enum class TraceAction {
+    // Deny the syscall.
+    kDeny,
+    // Allow the syscall.
+    kAllow,
+    // Allow the syscall so its return value can be inspected through a
+    // subsequent call to EventSyscallReturn.
+    // Requires Linux kernel 4.8 or later.
+    kInspectAfterReturn
+  };
+
+  // Called when a policy called TRACE. The syscall is allowed or denied
+  // depending on the return value of this function.
+  virtual TraceAction EventSyscallTrace(const Syscall& syscall) {
+    if (EventSyscallTrap(syscall)) {
+      LOG(WARNING) << "[PERMITTED]: SYSCALL ::: PID: " << syscall.pid()
+                   << ", PROG: '" << util::GetProgName(syscall.pid())
+                   << "' : " << syscall.GetDescription();
+      return TraceAction::kAllow;
+    }
+    return TraceAction::kDeny;
+  }
+
+  // Called when a policy called TRACE and EventSyscallTrace returned
+  // kInspectAfterReturn.
+  virtual void EventSyscallReturn(const Syscall& syscall,
+                                  int64_t return_value) {}
+
   // Called when a process received a signal.
   virtual void EventSignal(pid_t pid, int sig_no) {}
 };

sandboxed_api/sandbox2/regs.cc

@@ -187,6 +187,30 @@ Syscall Regs::ToSyscall(sapi::cpu::Architecture syscall_arch) const {
   return Syscall(pid_);
 }
 
+int64_t Regs::GetReturnValue(sapi::cpu::Architecture syscall_arch) const {
+#if defined(SAPI_X86_64)
+  if (ABSL_PREDICT_TRUE(syscall_arch == sapi::cpu::kX8664)) {
+    return static_cast<int64_t>(user_regs_.rax);
+  }
+  if (syscall_arch == sapi::cpu::kX86) {
+    return static_cast<int32_t>(user_regs_.rax & 0xFFFFFFFF);
+  }
+#elif defined(SAPI_PPC64_LE)
+  if (ABSL_PREDICT_TRUE(syscall_arch == sapi::cpu::kPPC64LE)) {
+    return static_cast<int64_t>(user_regs_.gpr[3]);
+  }
+#elif defined(SAPI_ARM64)
+  if (ABSL_PREDICT_TRUE(syscall_arch == sapi::cpu::kArm64)) {
+    return static_cast<int64_t>(user_regs_.regs[0]);
+  }
+#elif defined(SAPI_ARM)
+  if (ABSL_PREDICT_TRUE(syscall_arch == sapi::cpu::kArm)) {
+    return static_cast<int32_t>(user_regs_.regs[0]);
+  }
+#endif
+  return -1;
+}
+
 void Regs::StoreRegisterValuesInProtobuf(RegisterValues* values) const {
 #if defined(SAPI_X86_64)
   RegisterX8664* regs = values->mutable_register_x86_64();

sandboxed_api/sandbox2/regs.h

@@ -48,6 +48,9 @@ class Regs {
   // Converts raw register values obtained on syscall entry to syscall info
   Syscall ToSyscall(sapi::cpu::Architecture syscall_arch) const;
 
+  // Returns the content of the register that holds a syscall's return value
+  int64_t GetReturnValue(sapi::cpu::Architecture syscall_arch) const;
+
   pid_t pid() const { return pid_; }
 
   // Stores register values in a protobuf structure.

sandboxed_api/sandbox2/result.cc

@@ -189,6 +189,8 @@ std::string Result::ReasonCodeEnumToString(ReasonCodeEnum value) {
       return "FAILED_KILL";
     case sandbox2::Result::FAILED_CHILD:
       return "FAILED_CHILD";
+    case sandbox2::Result::FAILED_INSPECT:
+      return "FAILED_INSPECT";
     case sandbox2::Result::VIOLATION_SYSCALL:
       return "VIOLATION_SYSCALL";
     case sandbox2::Result::VIOLATION_ARCH:

sandboxed_api/sandbox2/result.h

@@ -80,6 +80,7 @@ class Result {
     FAILED_MONITOR,
     FAILED_KILL,
     FAILED_CHILD,
+    FAILED_INSPECT,
 
     // TODO(wiktorg) not used currently (syscall number stored insted) - need to
     // fix clients first