From ca6ec4337d876751097e6ba76fa1df6d7917e6d4 Mon Sep 17 00:00:00 2001 From: Christian Blichmann Date: Mon, 10 May 2021 07:03:15 -0700 Subject: [PATCH] Add workaround for active Tomoyo LSM Recenly, Debian based distribution kernels started activating the Tomoyo Linux Security Module by default. Even if it is not used, this changes the behavior of `/dev/fd` (pointing to `/proc/self/fd` by default), which Sandbox2 needs during `execveat()`. As a result, Sandbox2 and Sandboxed API always fail without one of the following conditions - `/proc` mounted within the sandboxee - `/dev` mounted - `/dev/fd` symlinked to `/proc/self/fd` in the sandboxee's mount namespace Some code pointers to upstream Linux 5.12.2: - https://elixir.bootlin.com/linux/v5.12.2/source/fs/exec.c#L1775 - https://elixir.bootlin.com/linux/v5.12.2/source/security/tomoyo/tomoyo.c#L107 - https://elixir.bootlin.com/linux/v5.12.2/source/security/tomoyo/domain.c#L729 To find out whether your system has Tomoyo enabled, use this command, similar to what this change does in code: ``` $ cat /sys/kernel/security/lsm | grep tomoyo && echo "Tomoyo active" capability,yama,apparmor,tomoyo Tomoyo active ``` The config setting `CONFIG_DEFAULT_SECURITY` controls which LSMs are built into the kernel by default. PiperOrigin-RevId: 372919524 Change-Id: I2181819c04f15f57d96c44ea9977d0def4a1b623 --- sandboxed_api/sandbox2/BUILD.bazel | 4 ++ sandboxed_api/sandbox2/CMakeLists.txt | 14 +++-- sandboxed_api/sandbox2/comms.h | 6 ++ sandboxed_api/sandbox2/forkserver.cc | 6 +- sandboxed_api/sandbox2/monitor.cc | 86 +++++++++++++++++++++------ sandboxed_api/sandbox2/monitor.h | 4 ++ sandboxed_api/sandbox2/mounts.cc | 2 +- sandboxed_api/sandbox2/mounts.h | 2 + 8 files changed, 95 insertions(+), 29 deletions(-) diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index 1dbaad8..de31317 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -326,11 +326,14 @@ cc_library( "//sandboxed_api/sandbox2/unwind:unwind_cc_proto", "//sandboxed_api/sandbox2/util:bpf_helper", "//sandboxed_api/util:file_base", + "//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:temp_file", "@com_google_absl//absl/base:core_headers", + "@com_google_absl//absl/cleanup", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/memory", @@ -404,6 +407,7 @@ cc_library( ":util", "//sandboxed_api/sandbox2/unwind", "//sandboxed_api/sandbox2/util:bpf_helper", + "//sandboxed_api/util:file_helpers", "//sandboxed_api/util:fileops", "//sandboxed_api/util:raw_logging", "//sandboxed_api/util:strerror", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 918eda0..04d38d8 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -293,6 +293,7 @@ add_library(sandbox2_sandbox2 ${SAPI_LIB_TYPE} 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 @@ -304,15 +305,16 @@ target_link_libraries(sandbox2_sandbox2 PUBLIC absl::status absl::statusor absl::time - sapi::flags - sapi::status - sandbox2::bpf_helper - sandbox2::client sapi::config - sandbox2::comms - sandbox2::executor sapi::file_base sapi::fileops + sapi::flags + sapi::status + sapi::temp_file + sandbox2::bpf_helper + sandbox2::client + sandbox2::comms + sandbox2::executor sandbox2::fork_client sandbox2::forkserver_proto sandbox2::global_forkserver diff --git a/sandboxed_api/sandbox2/comms.h b/sandboxed_api/sandbox2/comms.h index 12e0385..568c11d 100644 --- a/sandboxed_api/sandbox2/comms.h +++ b/sandboxed_api/sandbox2/comms.h @@ -62,6 +62,12 @@ class Comms { // Any payload size above this limit will LOG(WARNING). static constexpr size_t kWarnMsgSize = (256ULL << 20); + // A high file descriptor number to be used with certain fork server request + // modes to map the target executable. This is considered to be an + // implementation detail. + // This number is chosen so that low FD numbers are not interfered with. + static constexpr int kSandbox2TargetExecFD = 1022; + // Sandbox2-specific convention where FD=1023 is always passed to the // sandboxed process as a communication channel (encapsulated in the // sandbox2::Comms object at the server-side). diff --git a/sandboxed_api/sandbox2/forkserver.cc b/sandboxed_api/sandbox2/forkserver.cc index 51f9971..73061e7 100644 --- a/sandboxed_api/sandbox2/forkserver.cc +++ b/sandboxed_api/sandbox2/forkserver.cc @@ -315,10 +315,6 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, } pid_t ForkServer::ServeRequest() { - // Keep the low FD numbers clean so that client FD mappings don't interfer - // with us. - constexpr int kTargetExecFd = 1022; - ForkRequest fork_request; if (!comms_->RecvProtoBuf(&fork_request)) { if (comms_->IsTerminated()) { @@ -336,7 +332,7 @@ pid_t ForkServer::ServeRequest() { 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, kTargetExecFd); + MoveToFdNumber(&exec_fd, Comms::kSandbox2TargetExecFD); } // Make the kernel notify us with SIGCHLD when the process terminates. diff --git a/sandboxed_api/sandbox2/monitor.cc b/sandboxed_api/sandbox2/monitor.cc index db78134..8d5242b 100644 --- a/sandboxed_api/sandbox2/monitor.cc +++ b/sandboxed_api/sandbox2/monitor.cc @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -42,8 +43,11 @@ #include #include +#include "absl/cleanup/cleanup.h" #include "sandboxed_api/util/flag.h" #include "absl/memory/memory.h" +#include "absl/status/status.h" +#include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" #include "absl/time/time.h" @@ -62,7 +66,9 @@ #include "sandboxed_api/sandbox2/stack_trace.h" #include "sandboxed_api/sandbox2/syscall.h" #include "sandboxed_api/sandbox2/util.h" +#include "sandboxed_api/util/file_helpers.h" #include "sandboxed_api/util/raw_logging.h" +#include "sandboxed_api/util/temp_file.h" using std::string; @@ -119,12 +125,55 @@ void StopProcess(pid_t pid, int signo) { } } +void MaybeEnableTomoyoLsmWorkaround(Mounts& mounts, std::string& comms_fd_dev) { + static auto tomoyo_active = []() -> bool { + std::string lsm_list; + if (auto status = sapi::file::GetContents( + "/sys/kernel/security/lsm", &lsm_list, sapi::file::Defaults()); + !status.ok() && !absl::IsNotFound(status)) { + SAPI_RAW_PLOG(WARNING, "Checking active LSMs failed: %s", + std::string(status.message()).c_str()); + return false; + } + return absl::StrContains(lsm_list, "tomoyo"); + }(); + + if (!tomoyo_active) { + return; + } + SAPI_RAW_VLOG(1, "Tomoyo LSM active, enabling workaround"); + + if (mounts.ResolvePath("/dev").ok() || mounts.ResolvePath("/dev/fd").ok()) { + // Avoid shadowing /dev/fd/1022 below if /dev or /dev/fd is already mapped. + SAPI_RAW_VLOG(1, "Parent dir already mapped, skipping"); + return; + } + + auto temp_file = sapi::CreateNamedTempFileAndClose("/tmp/"); + if (!temp_file.ok()) { + SAPI_RAW_LOG(WARNING, "Failed to create empty temp file: %s", + std::string(temp_file.status().message()).c_str()); + return; + } + comms_fd_dev = std::move(*temp_file); + + // Ignore errors here, as the file itself might already be mapped. + if (auto status = mounts.AddFileAt( + comms_fd_dev, absl::StrCat("/dev/fd/", Comms::kSandbox2TargetExecFD), + false); + !status.ok()) { + SAPI_RAW_VLOG(1, "Mapping comms FD: %s", + std::string(status.message()).c_str()); + } +} + } // namespace Monitor::Monitor(Executor* executor, Policy* policy, Notify* notify) : executor_(executor), notify_(notify), policy_(policy), + // NOLINTNEXTLINE clang-diagnostic-deprecated-declarations comms_(executor_->ipc()->comms()), ipc_(executor_->ipc()), wait_for_execve_(executor->enable_sandboxing_pre_execve_) { @@ -138,9 +187,18 @@ Monitor::Monitor(Executor* executor, Policy* policy, Notify* notify) log_file_ = std::fopen(path.c_str(), "a+"); PCHECK(log_file_ != nullptr) << "Failed to open log file '" << path << "'"; } + + if (auto* ns = policy_->GetNamespace(); ns) { + // Check for the Tomoyo LSM, which is active by default in several common + // distribution kernels (esp. Debian). + MaybeEnableTomoyoLsmWorkaround(ns->mounts(), comms_fd_dev_); + } } Monitor::~Monitor() { + if (!comms_fd_dev_.empty()) { + std::remove(comms_fd_dev_.c_str()); + } if (log_file_) { std::fclose(log_file_); } @@ -160,20 +218,14 @@ void LogContainer(const std::vector& container) { } // namespace void Monitor::Run() { - std::unique_ptr - setup_notify{&setup_notification_, [](absl::Notification* notification) { - notification->Notify(); - }}; + absl::Cleanup setup_notify = [this] { setup_notification_.Notify(); }; - struct MonitorCleanup { - ~MonitorCleanup() { - getrusage(RUSAGE_THREAD, capture->result_.GetRUsageMonitor()); - capture->notify_->EventFinished(capture->result_); - capture->ipc_->InternalCleanupFdMap(); - capture->done_notification_.Notify(); - } - Monitor* capture; - } monitor_cleanup{this}; + absl::Cleanup monitor_cleanup = [this] { + getrusage(RUSAGE_THREAD, result_.GetRUsageMonitor()); + notify_->EventFinished(result_); + ipc_->InternalCleanupFdMap(); + done_notification_.Notify(); + }; if (executor_->limits()->wall_time_limit() != absl::ZeroDuration()) { auto deadline = absl::Now() + executor_->limits()->wall_time_limit(); @@ -189,10 +241,11 @@ void Monitor::Run() { return; } - if (SAPI_VLOG_IS_ON(1) && policy_->GetNamespace() != nullptr) { + Namespace* ns = policy_->GetNamespace(); + if (SAPI_VLOG_IS_ON(1) && ns != nullptr) { std::vector outside_entries; std::vector inside_entries; - policy_->GetNamespace()->mounts().RecursivelyListMounts( + ns->mounts().RecursivelyListMounts( /*outside_entries=*/&outside_entries, /*inside_entries=*/&inside_entries); SAPI_RAW_VLOG(1, "Outside entries mapped to chroot:"); @@ -211,7 +264,6 @@ void Monitor::Run() { // Get PID of the sandboxee. pid_t init_pid = 0; - Namespace* ns = policy_->GetNamespace(); bool should_have_init = ns && (ns->GetCloneFlags() & CLONE_NEWPID); pid_ = executor_->StartSubProcess(clone_flags, ns, policy_->GetCapabilities(), &init_pid); @@ -265,7 +317,7 @@ void Monitor::Run() { // Tell the parent thread (Sandbox2 object) that we're done with the initial // set-up process of the sandboxee. - setup_notify.reset(); + std::move(setup_notify).Invoke(); MainLoop(&sigtimedwait_sset); } diff --git a/sandboxed_api/sandbox2/monitor.h b/sandboxed_api/sandbox2/monitor.h index 069d93e..4010abd 100644 --- a/sandboxed_api/sandbox2/monitor.h +++ b/sandboxed_api/sandbox2/monitor.h @@ -187,6 +187,10 @@ class Monitor final { // --sandbox_danger_danger_permit_all_and_log flag. FILE* log_file_ = nullptr; + // Empty temp file used for mapping the comms fd when the Tomoyo LSM is + // active. + std::string comms_fd_dev_; + // Handle to the class responsible for proxying and validating connect() // requests. std::unique_ptr network_proxy_server_; diff --git a/sandboxed_api/sandbox2/mounts.cc b/sandboxed_api/sandbox2/mounts.cc index 6585737..4c02032 100644 --- a/sandboxed_api/sandbox2/mounts.cc +++ b/sandboxed_api/sandbox2/mounts.cc @@ -27,6 +27,7 @@ #include "google/protobuf/util/message_differencer.h" #include "absl/container/flat_hash_set.h" +#include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/ascii.h" #include "absl/strings/match.h" @@ -194,7 +195,6 @@ absl::Status Mounts::Insert(absl::string_view path, } std::string fixed_path = sapi::file::CleanPath(path); - if (!sapi::file::IsAbsolutePath(fixed_path)) { return absl::InvalidArgumentError("Only absolute paths are supported"); } diff --git a/sandboxed_api/sandbox2/mounts.h b/sandboxed_api/sandbox2/mounts.h index 734530b..c0aa704 100644 --- a/sandboxed_api/sandbox2/mounts.h +++ b/sandboxed_api/sandbox2/mounts.h @@ -81,7 +81,9 @@ class Mounts { private: friend class MountTreeTest; + absl::Status Insert(absl::string_view path, const MountTree::Node& node); + MountTree mount_tree_; };