diff --git a/sandboxed_api/examples/sum/main_sum.cc b/sandboxed_api/examples/sum/main_sum.cc index 6ed74a4..86f0086 100644 --- a/sandboxed_api/examples/sum/main_sum.cc +++ b/sandboxed_api/examples/sum/main_sum.cc @@ -156,13 +156,13 @@ absl::Status SumTransaction::Main() { sapi::v::Fd fd(fdesc); SAPI_RETURN_IF_ERROR(sandbox()->TransferToSandboxee(&fd)); LOG(INFO) << "remote_fd = " << fd.GetRemoteFd(); - TRANSACTION_FAIL_IF_NOT(fd.GetRemoteFd() == 3, "remote_fd != 3"); + TRANSACTION_FAIL_IF_NOT(fd.GetRemoteFd() != -1, "remote_fd == -1"); fdesc = open("/proc/self/comm", O_RDONLY); sapi::v::Fd fd2(fdesc); SAPI_RETURN_IF_ERROR(sandbox()->TransferToSandboxee(&fd2)); LOG(INFO) << "remote_fd2 = " << fd2.GetRemoteFd(); - TRANSACTION_FAIL_IF_NOT(fd2.GetRemoteFd() == 4, "remote_fd2 != 4"); + TRANSACTION_FAIL_IF_NOT(fd2.GetRemoteFd() != -1, "remote_fd2 == -1"); // Read from fd test. char buffer[1024] = {0}; diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 22b7168..d9680a2 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -579,7 +579,6 @@ target_link_libraries(sandbox2_forkserver sandbox2::bpf_helper sandbox2::client sandbox2::comms - sapi::fileops sandbox2::fork_client sandbox2::forkserver_proto sandbox2::namespace @@ -592,6 +591,7 @@ target_link_libraries(sandbox2_forkserver sapi::raw_logging PUBLIC absl::core_headers absl::log + sapi::fileops ) # sandboxed_api/sandbox2:fork_client diff --git a/sandboxed_api/sandbox2/forkserver.cc b/sandboxed_api/sandbox2/forkserver.cc index f5f8be9..a1d2d8a 100644 --- a/sandboxed_api/sandbox2/forkserver.cc +++ b/sandboxed_api/sandbox2/forkserver.cc @@ -37,9 +37,10 @@ #include #include #include -#include #include +#include +#include "absl/base/attributes.h" #include "absl/container/flat_hash_map.h" #include "absl/container/flat_hash_set.h" #include "absl/status/status.h" @@ -65,12 +66,10 @@ #include "sandboxed_api/util/strerror.h" namespace sandbox2 { - -namespace file_util = ::sapi::file_util; - namespace { using ::sapi::StrError; +using ::sapi::file_util::fileops::FDCloser; // "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 @@ -115,18 +114,11 @@ void MoveFDs(std::initializer_list> move_fds, } } -void RunInitProcess(pid_t main_pid, int pipe_fd, - const absl::flat_hash_set& open_fds) { +ABSL_ATTRIBUTE_NORETURN void RunInitProcess(pid_t main_pid, FDCloser pipe_fd) { if (prctl(PR_SET_NAME, "S2-INIT-PROC", 0, 0, 0) != 0) { SAPI_RAW_PLOG(WARNING, "prctl(PR_SET_NAME, 'S2-INIT-PROC')"); } - // Close all open fds (equals to CloseAllFDsExcept but does not require /proc - // to be available). - for (const auto& fd : open_fds) { - close(fd); - } - // Clear SA_NOCLDWAIT. struct sigaction sa; sa.sa_handler = SIG_DFL; @@ -144,7 +136,7 @@ void RunInitProcess(pid_t main_pid, int pipe_fd, SYSCALL(__NR_waitid, ALLOW), SYSCALL(__NR_exit, ALLOW), }; - if (pipe_fd >= 0) { + if (pipe_fd.get() >= 0) { code.insert(code.end(), {SYSCALL(__NR_getrusage, ALLOW), SYSCALL(__NR_write, ALLOW)}); } @@ -171,13 +163,13 @@ void RunInitProcess(pid_t main_pid, int pipe_fd, } if (info.si_pid == main_pid) { - if (pipe_fd >= 0) { - write(pipe_fd, &info.si_code, sizeof(info.si_code)); - write(pipe_fd, &info.si_status, sizeof(info.si_status)); + if (pipe_fd.get() >= 0) { + write(pipe_fd.get(), &info.si_code, sizeof(info.si_code)); + write(pipe_fd.get(), &info.si_status, sizeof(info.si_status)); rusage usage{}; getrusage(RUSAGE_CHILDREN, &usage); - write(pipe_fd, &usage, sizeof(usage)); + write(pipe_fd.get(), &usage, sizeof(usage)); } _exit(0); } @@ -283,9 +275,8 @@ void ForkServer::PrepareExecveArgs(const ForkRequest& request, } void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, - int client_fd, uid_t uid, gid_t gid, - int signaling_fd, int status_fd, - bool avoid_pivot_root) const { + uid_t uid, gid_t gid, FDCloser signaling_fd, + FDCloser status_fd, bool avoid_pivot_root) const { SAPI_RAW_CHECK(request.mode() != FORKSERVER_FORK_UNSPECIFIED, "Forkserver mode is unspecified"); @@ -299,10 +290,6 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, PrepareExecveArgs(request, &args, &envs); } - MoveFDs({{&execve_fd, Comms::kSandbox2TargetExecFD}, - {&client_fd, Comms::kSandbox2ClientCommsFD}}, - {&signaling_fd}); - SanitizeEnvironment(); absl::StatusOr> open_fds = sanitizer::GetListOfFDs(); @@ -326,19 +313,23 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, SAPI_RAW_PLOG(FATAL, "Could not spawn init process"); } if (child != 0) { - if (status_fd >= 0) { - open_fds->erase(status_fd); + if (status_fd.get() >= 0) { + open_fds->erase(status_fd.get()); } - RunInitProcess(child, status_fd, *open_fds); - } - if (status_fd >= 0) { - close(status_fd); + // Close all open fds (equals to CloseAllFDsExcept but does not require + // /proc to be available). + for (const auto& fd : *open_fds) { + close(fd); + } + RunInitProcess(child, std::move(status_fd)); } // Send sandboxee pid - auto status = SendPid(signaling_fd); + auto status = SendPid(signaling_fd.get()); SAPI_RAW_CHECK(status.ok(), absl::StrCat("sending pid: ", status.message()).c_str()); } + signaling_fd.Close(); + status_fd.Close(); if (request.mode() == FORKSERVER_FORK_EXECVE_SANDBOX) { // Sandboxing can be enabled either here - just before execve, or somewhere @@ -350,17 +341,16 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, // Create a Comms object here and not above, as we know we will execve and // therefore not call the Comms destructor, which would otherwise close the // comms file descriptor, which we do not want for the general case. - Comms client_comms(Comms::kDefaultConnection); - Client c(&client_comms); + Client c(comms_); // The following client calls are basically SandboxMeHere. We split it so // that we can set up the envp after we received the file descriptors but // before we enable the syscall filter. c.PrepareEnvironment(&execve_fd); - if (client_comms.GetConnectionFD() != Comms::kSandbox2ClientCommsFD) { + if (comms_->GetConnectionFD() != Comms::kSandbox2ClientCommsFD) { envs.push_back(absl::StrCat(Comms::kSandbox2CommsFDEnvVar, "=", - client_comms.GetConnectionFD())); + comms_->GetConnectionFD())); } envs.push_back(c.GetFdMapEnvVar()); // Convert args and envs before enabling sandbox (it'll allocate which might @@ -407,9 +397,14 @@ pid_t ForkServer::ServeRequest() { uid_t uid = getuid(); uid_t gid = getgid(); - int pfds[2] = {-1, -1}; - if (fork_request.monitor_type() == FORKSERVER_MONITOR_UNOTIFY) { - SAPI_RAW_PCHECK(pipe(pfds) == 0, "creating status pipe"); + FDCloser pipe_fds[2]; + { + int pfds[2] = {-1, -1}; + if (fork_request.monitor_type() == FORKSERVER_MONITOR_UNOTIFY) { + SAPI_RAW_PCHECK(pipe(pfds) == 0, "creating status pipe"); + } + pipe_fds[0] = FDCloser(pfds[0]); + pipe_fds[1] = FDCloser(pfds[1]); } int socketpair_fds[2]; @@ -423,8 +418,8 @@ pid_t ForkServer::ServeRequest() { "setsockopt failed"); } - file_util::fileops::FDCloser fd_closer0{socketpair_fds[0]}; - file_util::fileops::FDCloser fd_closer1{socketpair_fds[1]}; + FDCloser signaling_fds[] = {FDCloser(socketpair_fds[0]), + FDCloser(socketpair_fds[1])}; // Note: init_pid will be overwritten with the actual init pid if the init // process was started or stays at 0 if that is not needed - no pidns. @@ -459,7 +454,7 @@ pid_t ForkServer::ServeRequest() { _exit(0); } // Send sandboxee pid - absl::Status status = SendPid(fd_closer1.get()); + absl::Status status = SendPid(signaling_fds[1].get()); SAPI_RAW_CHECK(status.ok(), absl::StrCat("sending pid: ", status.message()).c_str()); } @@ -476,15 +471,29 @@ pid_t ForkServer::ServeRequest() { // Child. if (sandboxee_pid == 0) { - LaunchChild(fork_request, exec_fd, comms_fd, uid, gid, fd_closer1.get(), - pfds[1], avoid_pivot_root); + signaling_fds[0].Close(); + pipe_fds[0].Close(); + // Make sure we override the forkserver's comms fd + comms_->Terminate(); + if (exec_fd != -1) { + int signaling_fd = signaling_fds[1].Release(); + int pipe_fd = pipe_fds[1].Release(); + MoveFDs({{&exec_fd, Comms::kSandbox2TargetExecFD}, + {&comms_fd, Comms::kSandbox2ClientCommsFD}}, + {&signaling_fd, &pipe_fd}); + signaling_fds[1] = FDCloser(signaling_fd); + pipe_fds[1] = FDCloser(pipe_fd); + } + *comms_ = Comms(comms_fd); + LaunchChild(fork_request, exec_fd, uid, gid, std::move(signaling_fds[1]), + std::move(pipe_fds[1]), avoid_pivot_root); return sandboxee_pid; } - fd_closer1.Close(); + signaling_fds[1].Close(); if (avoid_pivot_root) { - if (auto pid = ReceivePid(fd_closer0.get()); !pid.ok()) { + if (auto pid = ReceivePid(signaling_fds[0].get()); !pid.ok()) { SAPI_RAW_LOG(ERROR, "%s", std::string(pid.status().message()).c_str()); } else { sandboxee_pid = pid.value(); @@ -498,7 +507,7 @@ pid_t ForkServer::ServeRequest() { sandboxee_pid = -1; // And the actual sandboxee is forked from the init process, so we need to // receive the actual PID. - if (auto pid_or = ReceivePid(fd_closer0.get()); !pid_or.ok()) { + if (auto pid_or = ReceivePid(signaling_fds[0].get()); !pid_or.ok()) { SAPI_RAW_LOG(ERROR, "%s", std::string(pid_or.status().message()).c_str()); if (init_pid != -1) { kill(init_pid, SIGKILL); @@ -510,9 +519,7 @@ pid_t ForkServer::ServeRequest() { } // Parent. - if (pfds[1] >= 0) { - close(pfds[1]); - } + pipe_fds[1].Close(); close(comms_fd); if (exec_fd >= 0) { close(exec_fd); @@ -523,9 +530,9 @@ pid_t ForkServer::ServeRequest() { comms_->SendInt32(sandboxee_pid), absl::StrCat("Failed to send sandboxee PID: ", sandboxee_pid).c_str()); - if (pfds[0] >= 0) { - SAPI_RAW_CHECK(comms_->SendFD(pfds[0]), "Failed to send status pipe"); - close(pfds[0]); + if (pipe_fds[0].get() >= 0) { + SAPI_RAW_CHECK(comms_->SendFD(pipe_fds[0].get()), + "Failed to send status pipe"); } return sandboxee_pid; } @@ -564,9 +571,9 @@ void ForkServer::CreateInitialNamespaces() { gid_t gid = getgid(); // Socket to synchronize so that we open ns fds before process dies - file_util::fileops::FDCloser create_efd(eventfd(0, EFD_CLOEXEC)); + FDCloser create_efd(eventfd(0, EFD_CLOEXEC)); SAPI_RAW_PCHECK(create_efd.get() != -1, "creating eventfd"); - file_util::fileops::FDCloser open_efd(eventfd(0, EFD_CLOEXEC)); + FDCloser open_efd(eventfd(0, EFD_CLOEXEC)); SAPI_RAW_PCHECK(open_efd.get() != -1, "creating eventfd"); pid_t pid = util::ForkWithFlags(CLONE_NEWUSER | CLONE_NEWNS | SIGCHLD); if (pid == -1 && errno == EPERM && IsLikelyChrooted()) { @@ -600,12 +607,11 @@ void ForkServer::CreateInitialNamespaces() { "synchronizing initial namespaces creation"); } -void ForkServer::SanitizeEnvironment() { +void ForkServer::SanitizeEnvironment() const { // Mark all file descriptors, except the standard ones (needed // for proper sandboxed process operations), as close-on-exec. absl::Status status = sanitizer::SanitizeCurrentProcess( - {STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO, - Comms::kSandbox2ClientCommsFD}, + {STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO, comms_->GetConnectionFD()}, /* close_fds = */ false); SAPI_RAW_CHECK( status.ok(), diff --git a/sandboxed_api/sandbox2/forkserver.h b/sandboxed_api/sandbox2/forkserver.h index 393ba9e..fd7b704 100644 --- a/sandboxed_api/sandbox2/forkserver.h +++ b/sandboxed_api/sandbox2/forkserver.h @@ -25,6 +25,7 @@ #include "absl/base/attributes.h" #include "absl/log/log.h" +#include "sandboxed_api/util/fileops.h" namespace sandbox2 { @@ -53,8 +54,9 @@ class ForkServer { private: // Creates and launched the child process. - void LaunchChild(const ForkRequest& request, int execve_fd, int client_fd, - uid_t uid, gid_t gid, int signaling_fd, int status_fd, + void LaunchChild(const ForkRequest& request, int execve_fd, uid_t uid, + gid_t gid, sapi::file_util::fileops::FDCloser signaling_fd, + sapi::file_util::fileops::FDCloser status_fd, bool avoid_pivot_root) const; // Prepares the Fork-Server (worker side, not the requester side) for work by @@ -73,7 +75,7 @@ class ForkServer { std::vector* envp); // Ensures that no unnecessary file descriptors are lingering after execve(). - static void SanitizeEnvironment(); + void SanitizeEnvironment() const; // Executes the sandboxee, or exit with Executor::kFailedExecve. static void ExecuteProcess(int execve_fd, const char* const* argv, diff --git a/sandboxed_api/sapi_test.cc b/sandboxed_api/sapi_test.cc index 4d4ee3c..5f66f2c 100644 --- a/sandboxed_api/sapi_test.cc +++ b/sandboxed_api/sapi_test.cc @@ -28,6 +28,7 @@ #include "sandboxed_api/examples/stringop/stringop-sapi.sapi.h" #include "sandboxed_api/examples/stringop/stringop_params.pb.h" #include "sandboxed_api/examples/sum/sum-sapi.sapi.h" +#include "sandboxed_api/sandbox.h" #include "sandboxed_api/testing.h" #include "sandboxed_api/transaction.h" #include "sandboxed_api/util/status_matchers.h" @@ -38,6 +39,7 @@ namespace { using ::sapi::IsOk; using ::sapi::StatusIs; using ::testing::Eq; +using ::testing::Gt; using ::testing::HasSubstr; // Functions that will be used during the benchmarks: @@ -195,22 +197,23 @@ TEST(SandboxTest, RestartSandboxFD) { TEST(SandboxTest, RestartTransactionSandboxFD) { sapi::BasicTransaction st{std::make_unique()}; - EXPECT_THAT(st.Run([](sapi::Sandbox* sandbox) -> absl::Status { - EXPECT_THAT(LeakFileDescriptor(sandbox, "/proc/self/exe"), Eq(3)); + int fd_no = -1; + ASSERT_THAT(st.Run([&fd_no](sapi::Sandbox* sandbox) -> absl::Status { + fd_no = LeakFileDescriptor(sandbox, "/proc/self/exe"); return absl::OkStatus(); }), IsOk()); - EXPECT_THAT(st.Run([](sapi::Sandbox* sandbox) -> absl::Status { - EXPECT_THAT(LeakFileDescriptor(sandbox, "/proc/self/exe"), Eq(4)); + EXPECT_THAT(st.Run([fd_no](sapi::Sandbox* sandbox) -> absl::Status { + EXPECT_THAT(LeakFileDescriptor(sandbox, "/proc/self/exe"), Gt(fd_no)); return absl::OkStatus(); }), IsOk()); EXPECT_THAT(st.Restart(), IsOk()); - EXPECT_THAT(st.Run([](sapi::Sandbox* sandbox) -> absl::Status { - EXPECT_THAT(LeakFileDescriptor(sandbox, "/proc/self/exe"), Eq(3)); + EXPECT_THAT(st.Run([fd_no](sapi::Sandbox* sandbox) -> absl::Status { + EXPECT_THAT(LeakFileDescriptor(sandbox, "/proc/self/exe"), Eq(fd_no)); return absl::OkStatus(); }), IsOk());