From 34c7be759acf46fe19db35b63f45ddd2429b5601 Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Wed, 14 Jul 2021 01:32:57 -0700 Subject: [PATCH] Another round of file descriptor handling fixes PiperOrigin-RevId: 384646707 Change-Id: Ia1b51a348bcb2a1426ba26a4ed045b0522168745 --- sandboxed_api/sandbox2/BUILD.bazel | 1 + sandboxed_api/sandbox2/CMakeLists.txt | 1 + sandboxed_api/sandbox2/forkserver.cc | 68 ++++++++++++++++++--------- sandboxed_api/sandbox2/forkserver.h | 2 +- 4 files changed, 50 insertions(+), 22 deletions(-) diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index ca96c1d..25b70a6 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -411,6 +411,7 @@ cc_library( "//sandboxed_api/util:fileops", "//sandboxed_api/util:raw_logging", "//sandboxed_api/util:strerror", + "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/memory", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index f3425a8..204eeb1 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -381,6 +381,7 @@ add_library(sandbox2_forkserver ${SAPI_LIB_TYPE} ) add_library(sandbox2::forkserver ALIAS sandbox2_forkserver) target_link_libraries(sandbox2_forkserver PRIVATE + absl::flat_hash_map absl::memory absl::status absl::statusor diff --git a/sandboxed_api/sandbox2/forkserver.cc b/sandboxed_api/sandbox2/forkserver.cc index 422b096..a17dce2 100644 --- a/sandboxed_api/sandbox2/forkserver.cc +++ b/sandboxed_api/sandbox2/forkserver.cc @@ -34,6 +34,7 @@ #include #include +#include "absl/container/flat_hash_map.h" #include "absl/memory/memory.h" #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -61,21 +62,48 @@ namespace { using ::sapi::StrError; -// "Moves" the old FD to the new FD number. -// The old FD will be closed, the new one is marked as CLOEXEC. -void MoveToFdNumber(int* old_fd, int new_fd) { - if (dup2(*old_fd, new_fd) == -1) { - SAPI_RAW_PLOG(FATAL, "Moving temporary to proper FD failed."); - } - close(*old_fd); - - // Try to mark that FD as CLOEXEC. - int flags = fcntl(new_fd, F_GETFD); - if (flags == -1 || fcntl(new_fd, F_SETFD, flags | O_CLOEXEC) != 0) { - SAPI_RAW_PLOG(ERROR, "Marking FD as CLOEXEC failed"); +// "Moves" FDs in move_fds from current to target FD number while keeping FDs +// in keep_fds open - potentially moving them to another FD number as well in +// case of colisions. +// Ignores invalid (-1) fds. +void MoveFDs(std::initializer_list> move_fds, + std::initializer_list keep_fds) { + absl::flat_hash_map fd_map; + for (int* fd : keep_fds) { + if (*fd != -1) { + fd_map.emplace(*fd, fd); + } } - *old_fd = new_fd; + for (auto [old_fd, new_fd] : move_fds) { + if (*old_fd != -1) { + fd_map.emplace(*old_fd, old_fd); + } + } + + for (auto [old_fd, new_fd] : move_fds) { + if (*old_fd == -1 || *old_fd == new_fd) { + continue; + } + + // Make sure we won't override another fd + auto it = fd_map.find(new_fd); + if (it != fd_map.end()) { + int fd = dup(new_fd); + SAPI_RAW_CHECK(fd != -1, "Duplicating an FD failed."); + *it->second = fd; + fd_map.emplace(fd, it->second); + fd_map.erase(it); + } + + if (dup2(*old_fd, new_fd) == -1) { + SAPI_RAW_PLOG(FATAL, "Moving temporary to proper FD failed."); + } + + close(*old_fd); + fd_map.erase(*old_fd); + *old_fd = new_fd; + } } void RunInitProcess(std::set open_fds) { @@ -231,7 +259,11 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, PrepareExecveArgs(request, &args, &envs); } - SanitizeEnvironment(client_fd); + MoveFDs({{&execve_fd, Comms::kSandbox2TargetExecFD}, + {&client_fd, Comms::kSandbox2ClientCommsFD}}, + {&signaling_fd}); + + SanitizeEnvironment(); std::set open_fds; if (!sanitizer::GetListOfFDs(&open_fds)) { @@ -331,8 +363,6 @@ pid_t ForkServer::ServeRequest() { if (fork_request.mode() == FORKSERVER_FORK_EXECVE || fork_request.mode() == FORKSERVER_FORK_EXECVE_SANDBOX) { SAPI_RAW_CHECK(comms_->RecvFD(&exec_fd), "Failed to receive Exec FD"); - // We're duping to a high number here to avoid colliding with the IPC FDs. - MoveToFdNumber(&exec_fd, Comms::kSandbox2TargetExecFD); } // Make the kernel notify us with SIGCHLD when the process terminates. @@ -519,11 +549,7 @@ void ForkServer::CreateInitialNamespaces() { close(fds[1]); } -void ForkServer::SanitizeEnvironment(int client_fd) { - // Duplicate client's CommsFD onto fd=Comms::kSandbox2ClientCommsFD (1023). - SAPI_RAW_CHECK(dup2(client_fd, Comms::kSandbox2ClientCommsFD) != -1, - "while remapping client comms fd"); - close(client_fd); +void ForkServer::SanitizeEnvironment() { // Mark all file descriptors, except the standard ones (needed // for proper sandboxed process operations), as close-on-exec. SAPI_RAW_CHECK(sanitizer::SanitizeCurrentProcess( diff --git a/sandboxed_api/sandbox2/forkserver.h b/sandboxed_api/sandbox2/forkserver.h index acf9852..5410461 100644 --- a/sandboxed_api/sandbox2/forkserver.h +++ b/sandboxed_api/sandbox2/forkserver.h @@ -69,7 +69,7 @@ class ForkServer { std::vector* envp); // Ensures that no unnecessary file descriptors are lingering after execve(). - static void SanitizeEnvironment(int client_fd); + static void SanitizeEnvironment(); // Executes the sandboxee, or exit with Executor::kFailedExecve. static void ExecuteProcess(int execve_fd, const char** argv,