diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index e26235b..072ce02 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/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", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 8b2ea11..2867512 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/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 - PUBLIC absl::core_headers - glog::glog - sandbox2::comms - sandbox2::result - sandbox2::syscall - sandbox2::util - PRIVATE sapi::base - sapi::config +target_link_libraries(sandbox2_notify PRIVATE + sandbox2::comms + sandbox2::result + sandbox2::syscall + sapi::base ) # 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 - absl::status + PUBLIC absl::status absl::statusor absl::synchronization absl::time diff --git a/sandboxed_api/sandbox2/monitor.cc b/sandboxed_api/sandbox2/monitor.cc index 1c16c8f..efb4342 100644 --- a/sandboxed_api/sandbox2/monitor.cc +++ b/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 (trace_response == Notify::TraceAction::kAllow) { + if (notify_->EventSyscallTrap(syscall)) { + 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(®s, 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 - // 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) { + if (__WPTRACEEVENT(status) == 0) { // 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; - EventPtraceNewProcess(pid, event_msg); - break; + /* fall through */ case PTRACE_EVENT_VFORK: - VLOG(2) << "PID: " << pid << " PTRACE_EVENT_VFORK, PID: " << event_msg; - EventPtraceNewProcess(pid, event_msg); - break; + /* fall through */ case PTRACE_EVENT_CLONE: - VLOG(2) << "PID: " << pid << " PTRACE_EVENT_CLONE, PID: " << event_msg; - EventPtraceNewProcess(pid, event_msg); - break; + /* fall through */ case PTRACE_EVENT_VFORK_DONE: ContinueProcess(pid, 0); break; diff --git a/sandboxed_api/sandbox2/monitor.h b/sandboxed_api/sandbox2/monitor.h index 113a32f..a985590 100644 --- a/sandboxed_api/sandbox2/monitor.h +++ b/sandboxed_api/sandbox2/monitor.h @@ -28,7 +28,6 @@ #include #include -#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. - void EventPtraceNewProcess(pid_t pid, int event_msg); - - // Processes execution path. + // Processes excution 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 network_proxy_server_; std::thread network_proxy_thread_; - - // Syscalls that are running, whose result values we want to inspect. - absl::flat_hash_map syscalls_in_progress_; }; } // namespace sandbox2 diff --git a/sandboxed_api/sandbox2/notify.h b/sandboxed_api/sandbox2/notify.h index 302a0a1..b35c01f 100644 --- a/sandboxed_api/sandbox2/notify.h +++ b/sandboxed_api/sandbox2/notify.h @@ -19,13 +19,9 @@ #include -#include -#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 - // this method returns true. This allows for implementing 'log, but allow' - // policies. - ABSL_DEPRECATED("Override EventSyscallTrace() instead") + // Called when a policy called TRACE. The syscall is allowed if this method + // returns true. + // This allows for implementing 'log, but allow' policies. 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) {} }; diff --git a/sandboxed_api/sandbox2/regs.cc b/sandboxed_api/sandbox2/regs.cc index 106a815..338772f 100644 --- a/sandboxed_api/sandbox2/regs.cc +++ b/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(user_regs_.rax); - } - if (syscall_arch == sapi::cpu::kX86) { - return static_cast(user_regs_.rax & 0xFFFFFFFF); - } -#elif defined(SAPI_PPC64_LE) - if (ABSL_PREDICT_TRUE(syscall_arch == sapi::cpu::kPPC64LE)) { - return static_cast(user_regs_.gpr[3]); - } -#elif defined(SAPI_ARM64) - if (ABSL_PREDICT_TRUE(syscall_arch == sapi::cpu::kArm64)) { - return static_cast(user_regs_.regs[0]); - } -#elif defined(SAPI_ARM) - if (ABSL_PREDICT_TRUE(syscall_arch == sapi::cpu::kArm)) { - return static_cast(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(); diff --git a/sandboxed_api/sandbox2/regs.h b/sandboxed_api/sandbox2/regs.h index a9dbd90..57a6747 100644 --- a/sandboxed_api/sandbox2/regs.h +++ b/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. diff --git a/sandboxed_api/sandbox2/result.cc b/sandboxed_api/sandbox2/result.cc index 255e491..7aba1c7 100644 --- a/sandboxed_api/sandbox2/result.cc +++ b/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: diff --git a/sandboxed_api/sandbox2/result.h b/sandboxed_api/sandbox2/result.h index 3a425a0..cad5216 100644 --- a/sandboxed_api/sandbox2/result.h +++ b/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