From 52d1ea8984d6bea122760ef5660f22c11366bb21 Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Wed, 9 Mar 2022 05:16:27 -0800 Subject: [PATCH] Avoid hard failures in StartSubProcess PiperOrigin-RevId: 433453289 Change-Id: Ib8b08ddd31c4daa9a377960d52f0a7eb7b17de19 --- sandboxed_api/sandbox2/BUILD.bazel | 1 + sandboxed_api/sandbox2/CMakeLists.txt | 1 + sandboxed_api/sandbox2/executor.cc | 60 ++++++++++++--------------- sandboxed_api/sandbox2/executor.h | 13 +++--- sandboxed_api/sandbox2/monitor.cc | 19 ++++++--- 5 files changed, 49 insertions(+), 45 deletions(-) diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index e7126c0..40ecfa5 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -288,6 +288,7 @@ cc_library( ":util", "//sandboxed_api:config", "//sandboxed_api/util:fileops", + "//sandboxed_api/util:status", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/memory", "@com_google_absl//absl/strings", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 6c76860..c17d5a3 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -286,6 +286,7 @@ target_link_libraries(sandbox2_executor glog::glog sapi::config sapi::fileops + sapi::status sandbox2::fork_client sandbox2::global_forkserver ) diff --git a/sandboxed_api/sandbox2/executor.cc b/sandboxed_api/sandbox2/executor.cc index 828ff7d..9286379 100644 --- a/sandboxed_api/sandbox2/executor.cc +++ b/sandboxed_api/sandbox2/executor.cc @@ -36,6 +36,7 @@ #include "sandboxed_api/sandbox2/ipc.h" #include "sandboxed_api/sandbox2/util.h" #include "sandboxed_api/util/fileops.h" +#include "sandboxed_api/util/os_error.h" namespace sandbox2 { @@ -80,19 +81,21 @@ std::vector Executor::CopyEnviron() { return util::CharPtrArray(environ).ToStringVector(); } -pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns, - const std::vector& caps, - pid_t* init_pid_out) { +absl::StatusOr Executor::StartSubProcess( + int32_t clone_flags, const Namespace* ns, const std::vector& caps) { if (started_) { - LOG(ERROR) << "This executor has already been started"; - return -1; + return absl::FailedPreconditionError( + "This executor has already been started"); } if (!path_.empty()) { exec_fd_ = file_util::fileops::FDCloser(open(path_.c_str(), O_PATH)); if (exec_fd_.get() < 0) { - PLOG(ERROR) << "Could not open file " << path_; - return -1; + if (errno == ENOENT) { + return absl::NotFoundError(sapi::OsErrorMessage(errno, path_)); + } + return absl::InternalError( + sapi::OsErrorMessage(errno, "Could not open file ", path_)); } } @@ -155,33 +158,22 @@ pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns, const std::string ns_path = absl::StrCat("/proc/", libunwind_sbox_for_pid_, "/ns/user"); ns_fd = file_util::fileops::FDCloser(open(ns_path.c_str(), O_RDONLY)); - PCHECK(ns_fd.get() != -1) - << "Could not open user ns fd (" << ns_path << ")"; + if (ns_fd.get() == -1) { + return absl::InternalError(sapi::OsErrorMessage( + errno, "Could not open user ns fd (", ns_path, ")")); + } } - pid_t init_pid = -1; + Process process; - pid_t sandboxee_pid; if (fork_client_) { - sandboxee_pid = fork_client_->SendRequest(request, exec_fd_.get(), - client_comms_fd_.get(), - ns_fd.get(), &init_pid); + process.main_pid = fork_client_->SendRequest( + request, exec_fd_.get(), client_comms_fd_.get(), ns_fd.get(), + &process.init_pid); } else { - sandboxee_pid = GlobalForkClient::SendRequest(request, exec_fd_.get(), - client_comms_fd_.get(), - ns_fd.get(), &init_pid); - } - - if (init_pid < 0) { - LOG(ERROR) << "Could not obtain init PID"; - } else if (init_pid == 0 && request.clone_flags() & CLONE_NEWPID) { - LOG(FATAL) - << "No init process was spawned even though a PID NS was created, " - << "potential logic bug"; - } - - if (init_pid_out) { - *init_pid_out = init_pid; + process.main_pid = GlobalForkClient::SendRequest( + request, exec_fd_.get(), client_comms_fd_.get(), ns_fd.get(), + &process.init_pid); } started_ = true; @@ -189,19 +181,19 @@ pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns, client_comms_fd_.Close(); exec_fd_.Close(); - VLOG(1) << "StartSubProcess returned with: " << sandboxee_pid; - return sandboxee_pid; + VLOG(1) << "StartSubProcess returned with: " << process.main_pid; + return process; } std::unique_ptr Executor::StartForkServer() { // This flag is set explicitly to 'true' during object instantiation, and // custom fork-servers should never be sandboxed. set_enable_sandbox_before_exec(false); - pid_t pid = StartSubProcess(0); - if (pid == -1) { + absl::StatusOr process = StartSubProcess(0); + if (!process.ok()) { return nullptr; } - return absl::make_unique(pid, ipc_.comms()); + return absl::make_unique(process->main_pid, ipc_.comms()); } void Executor::SetUpServerSideCommsFd() { diff --git a/sandboxed_api/sandbox2/executor.h b/sandboxed_api/sandbox2/executor.h index 3c543d8..49740af 100644 --- a/sandboxed_api/sandbox2/executor.h +++ b/sandboxed_api/sandbox2/executor.h @@ -39,6 +39,11 @@ namespace sandbox2 { // new processes which will be sandboxed. class Executor final { public: + struct Process { + pid_t init_pid; + pid_t main_pid; + }; + Executor(const Executor&) = delete; Executor& operator=(const Executor&) = delete; @@ -120,11 +125,9 @@ class Executor final { // // caps is a vector of capabilities that are kept in the permitted set after // the clone, use with caution. - // - // Returns the same values as fork(). - pid_t StartSubProcess(int clone_flags, const Namespace* ns = nullptr, - const std::vector& caps = {}, - pid_t* init_pid_out = nullptr); + absl::StatusOr StartSubProcess(int clone_flags, + const Namespace* ns = nullptr, + const std::vector& caps = {}); // Whether the Executor has been started yet bool started_ = false; diff --git a/sandboxed_api/sandbox2/monitor.cc b/sandboxed_api/sandbox2/monitor.cc index 0f94b90..0cf59ee 100644 --- a/sandboxed_api/sandbox2/monitor.cc +++ b/sandboxed_api/sandbox2/monitor.cc @@ -276,13 +276,20 @@ void Monitor::Run() { } // Get PID of the sandboxee. - pid_t init_pid = 0; bool should_have_init = ns && (ns->GetCloneFlags() & CLONE_NEWPID); - pid_ = executor_->StartSubProcess(clone_flags, ns, policy_->capabilities(), - &init_pid); + absl::StatusOr process = + executor_->StartSubProcess(clone_flags, ns, policy_->capabilities()); - if (init_pid > 0) { - if (ptrace(PTRACE_SEIZE, init_pid, 0, PTRACE_O_EXITKILL) != 0) { + if (!process.ok()) { + LOG(ERROR) << "Starting sandboxed subprocess failed: " << process.status(); + SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SUBPROCESS); + return; + } + + pid_ = process->main_pid; + + if (process->init_pid > 0) { + if (ptrace(PTRACE_SEIZE, process->init_pid, 0, PTRACE_O_EXITKILL) != 0) { if (errno == ESRCH) { SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_PTRACE); return; @@ -291,7 +298,7 @@ void Monitor::Run() { } } - if (pid_ <= 0 || (should_have_init && init_pid <= 0)) { + if (pid_ <= 0 || (should_have_init && process->init_pid <= 0)) { SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SUBPROCESS); return; }