Automated rollback of commit fac8713fbe.

PiperOrigin-RevId: 421356226
Change-Id: I4a179aeed226e005449c980e11b049759dad3878

sandboxed_api/sandbox2/BUILD.bazel

@@ -46,6 +46,7 @@ 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",
@@ -196,10 +197,6 @@ cc_library(
         ":comms",
         ":result",
         ":syscall",
-        ":util",
-        "//sandboxed_api:config",
-        "@com_google_absl//absl/base:core_headers",
-        "@com_google_glog//:glog",
     ],
 )
 
@@ -350,6 +347,7 @@ 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,15 +170,11 @@ add_library(sandbox2_notify ${SAPI_LIB_TYPE}
   notify.h
 )
 add_library(sandbox2::notify ALIAS sandbox2_notify)
-target_link_libraries(sandbox2_notify
+target_link_libraries(sandbox2_notify PRIVATE
-  PUBLIC absl::core_headers
+  sandbox2::comms
-         glog::glog
+  sandbox2::result
-         sandbox2::comms
+  sandbox2::syscall
-         sandbox2::result
+  sapi::base
-         sandbox2::syscall
-         sandbox2::util
-  PRIVATE sapi::base
-          sapi::config
 )
 
 # sandboxed_api/sandbox2:limits
@@ -300,6 +296,7 @@ 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
@@ -308,8 +305,7 @@ target_link_libraries(sandbox2_sandbox2
           absl::strings
           sapi::strerror
           sapi::base
-  PUBLIC  absl::flat_hash_map
+  PUBLIC  absl::status
-          absl::status
           absl::statusor
           absl::synchronization
           absl::time

sandboxed_api/sandbox2/monitor.cc

@@ -127,18 +127,6 @@ 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;
@@ -780,20 +768,14 @@ 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).
-  auto trace_response = notify_->EventSyscallTrace(syscall);
+  if (notify_->EventSyscallTrap(syscall)) {
-  if (trace_response == Notify::TraceAction::kAllow) {
+    LOG(WARNING) << "[PERMITTED]: SYSCALL ::: PID: " << regs->pid()
+                 << ", PROG: '" << util::GetProgName(regs->pid())
+                 << "' : " << syscall.GetDescription();
+
     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
@@ -874,68 +856,11 @@ 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();
-    if (syscall_nr != __NR_fork && syscall_nr != __NR_vfork &&
-        syscall_nr != __NR_clone) {
-      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);
 }
@@ -1007,10 +932,7 @@ void Monitor::EventPtraceStop(pid_t pid, int stopsig) {
 
 void Monitor::StateProcessStopped(pid_t pid, int status) {
   int stopsig = WSTOPSIG(status);
-  // We use PTRACE_O_TRACESYSGOOD, so we can tell it's a syscall stop without
+  if (__WPTRACEEVENT(status) == 0) {
-  // 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);
@@ -1058,25 +980,13 @@ 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:
-      VLOG(2) << "PID: " << pid << " PTRACE_EVENT_FORK, PID: " << event_msg;
+      /* fall through */
-      EventPtraceNewProcess(pid, event_msg);
-      break;
     case PTRACE_EVENT_VFORK:
-      VLOG(2) << "PID: " << pid << " PTRACE_EVENT_VFORK, PID: " << event_msg;
+      /* fall through */
-      EventPtraceNewProcess(pid, event_msg);
-      break;
     case PTRACE_EVENT_CLONE:
-      VLOG(2) << "PID: " << pid << " PTRACE_EVENT_CLONE, PID: " << event_msg;
+      /* fall through */
-      EventPtraceNewProcess(pid, event_msg);
-      break;
     case PTRACE_EVENT_VFORK_DONE:
       ContinueProcess(pid, 0);
       break;

sandboxed_api/sandbox2/monitor.h

@@ -28,7 +28,6 @@
 #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"
@@ -136,18 +135,12 @@ class Monitor final {
   // Processes exit path.
   void EventPtraceExit(pid_t pid, int event_msg);
 
-  // Processes fork/vfork/clone path.
+  // Processes excution 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();
@@ -203,9 +196,6 @@ 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,13 +19,9 @@
 
 #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 {
 
@@ -54,41 +50,11 @@ class Notify {
   virtual void EventSyscallViolation(const Syscall& syscall,
                                      ViolationType type) {}
 
-  // Called when a policy called TRACE. The syscall is allowed and logged if
+  // Called when a policy called TRACE. The syscall is allowed if this method
-  // this method returns true. This allows for implementing 'log, but allow'
+  // returns true.
-  // policies.
+  // 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,30 +187,6 @@ 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,9 +48,6 @@ 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,8 +189,6 @@ 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,7 +80,6 @@ class Result {
     FAILED_MONITOR,
     FAILED_KILL,
     FAILED_CHILD,
-    FAILED_INSPECT,
 
     // TODO(wiktorg) not used currently (syscall number stored insted) - need to
     // fix clients first