From bdb9a9eeabd36ddc449797e157fa0c08cf524a3d Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 13:17:21 +0200 Subject: [PATCH 01/15] [ML] Generate syscall policies from one declaration; fail-closed degraded seccomp Replace the hand-maintained BPF jump-offset table in CSystemCallFilter_Linux.cc with a program builder that derives every jump from the allowlist vector's own size/index. The applied program is generated from CPytorchInferenceSyscallAllowlist.h, a single machine-readable declaration, instead of a parallel hardcoded list (design.md V3/MG6). CSystemCallFilter::installSystemCallFilter() returns a typed ESystemCallFilterInstallOutcome across all three platform implementations instead of void, and logs the ml.seccomp.installed readiness marker on success. pytorch_inference/Main.cc gains a decideDegradedModeAction() decision that would terminate before CIoManager::initIo() on any degraded-mode seccomp failure; that termination stays behind an internal switch defaulting to false until ml-cpp PR E's typed controller routing can guarantee a degraded-mode launch was deliberate (design.md MG8, "activation is deliberately split across two slices"). The four non-PyTorch callers now make their unchanged log-and-continue policy explicit instead of silently discarding the result. Adds CSeccompFilterBuilderTest.cc: decodes the actually-built BPF program to prove it matches the declaration and that jump offsets are derived, not hand-maintained; fault-injected coverage of decideDegradedModeAction() for every install-failure class (V13); a named regression test per design.md M4 carry-forward syscall (clone3 by number, prlimit64, x86_64 legacy filesystem syscalls) so a blank-slate reconstruction cannot silently drop a hard-won compatibility fix from frozen PR #2873; and an explicit structured degradedModeAttestationMarker() (design.md M2) so a controller/ES observer can assert seccomp installation directly instead of inferring it from the absence of a fatal log line. The Sandbox2-grants half of V3 (CPytorchInferenceSandboxPolicy.cc) does not exist yet in this clean rebuild lineage (ml-cpp PR C); this change establishes the single declaration for that PR to consume. d9a856d5f (Sandbox2 AllowFutexOp arg-filtering) and 730933db/f8b0a534 (Buildkite run_tests.sh/build.sh packaging) are likewise out of this file's scope - see the bd note on elastic-workspace-3b59.3 for the explicit deferral to PR C/D/E. Verified on a real x86_64 devbox: ml_test_seccomp (7 test cases) and the real pytorch_inference/autodetect/categorize/normalize/ data_frame_analyzer binaries all build, link, and run correctly against the new typed API; the generated BPF program installs via a real prctl(PR_SET_SECCOMP) call. --- bin/autodetect/Main.cc | 8 +- bin/categorize/Main.cc | 8 +- bin/data_frame_analyzer/Main.cc | 8 +- bin/normalize/Main.cc | 8 +- bin/pytorch_inference/Main.cc | 28 +- .../CPytorchInferenceSyscallAllowlist.h | 128 ++++++++++ include/seccomp/CSeccompFilterBuilder.h | 39 +++ include/seccomp/CSystemCallFilter.h | 80 +++++- lib/seccomp/CSystemCallFilter_Linux.cc | 229 +++++++---------- lib/seccomp/CSystemCallFilter_MacOSX.cc | 8 +- lib/seccomp/CSystemCallFilter_Windows.cc | 12 +- lib/seccomp/unittest/CMakeLists.txt | 1 + .../unittest/CSeccompFilterBuilderTest.cc | 240 ++++++++++++++++++ lib/seccomp/unittest/CSystemCallFilterTest.cc | 4 +- 14 files changed, 650 insertions(+), 151 deletions(-) create mode 100644 include/seccomp/CPytorchInferenceSyscallAllowlist.h create mode 100644 include/seccomp/CSeccompFilterBuilder.h create mode 100644 lib/seccomp/unittest/CSeccompFilterBuilderTest.cc diff --git a/bin/autodetect/Main.cc b/bin/autodetect/Main.cc index 4c328fa5e6..2a2c476cf5 100644 --- a/bin/autodetect/Main.cc +++ b/bin/autodetect/Main.cc @@ -177,7 +177,13 @@ int main(int argc, char** argv) { // Reduce memory priority before installing system call filters. ml::core::CProcessPriority::reduceMemoryPriority(); - ml::seccomp::CSystemCallFilter::installSystemCallFilter(); + // Policy unchanged: log and continue on a degraded install; this + // binary does not process untrusted model input, unlike + // pytorch_inference. + if (ml::seccomp::CSystemCallFilter::installSystemCallFilter() != + ml::seccomp::ESystemCallFilterInstallOutcome::E_Installed) { + LOG_WARN(<< "Continuing without full syscall filtering"); + } if (ioMgr.initIo() == false) { LOG_FATAL(<< "Failed to initialise IO"); diff --git a/bin/categorize/Main.cc b/bin/categorize/Main.cc index aa4a1a4aaf..229108b17b 100644 --- a/bin/categorize/Main.cc +++ b/bin/categorize/Main.cc @@ -137,7 +137,13 @@ int main(int argc, char** argv) { // Reduce memory priority before installing system call filters. ml::core::CProcessPriority::reduceMemoryPriority(); - ml::seccomp::CSystemCallFilter::installSystemCallFilter(); + // Policy unchanged: log and continue on a degraded install; this + // binary does not process untrusted model input, unlike + // pytorch_inference. + if (ml::seccomp::CSystemCallFilter::installSystemCallFilter() != + ml::seccomp::ESystemCallFilterInstallOutcome::E_Installed) { + LOG_WARN(<< "Continuing without full syscall filtering"); + } if (ioMgr.initIo() == false) { LOG_FATAL(<< "Failed to initialise IO"); diff --git a/bin/data_frame_analyzer/Main.cc b/bin/data_frame_analyzer/Main.cc index 4b7b3d1ff1..a2c3b86494 100644 --- a/bin/data_frame_analyzer/Main.cc +++ b/bin/data_frame_analyzer/Main.cc @@ -160,7 +160,13 @@ int main(int argc, char** argv) { // Reduce memory priority before installing system call filters. ml::core::CProcessPriority::reduceMemoryPriority(); - ml::seccomp::CSystemCallFilter::installSystemCallFilter(); + // Policy unchanged: log and continue on a degraded install; this + // binary does not process untrusted model input, unlike + // pytorch_inference. + if (ml::seccomp::CSystemCallFilter::installSystemCallFilter() != + ml::seccomp::ESystemCallFilterInstallOutcome::E_Installed) { + LOG_WARN(<< "Continuing without full syscall filtering"); + } if (ioMgr.initIo() == false) { LOG_FATAL(<< "Failed to initialise IO"); diff --git a/bin/normalize/Main.cc b/bin/normalize/Main.cc index f6a79a7b65..9090dbf3c2 100644 --- a/bin/normalize/Main.cc +++ b/bin/normalize/Main.cc @@ -115,7 +115,13 @@ int main(int argc, char** argv) { // Reduce memory priority before installing system call filters. ml::core::CProcessPriority::reduceMemoryPriority(); - ml::seccomp::CSystemCallFilter::installSystemCallFilter(); + // Policy unchanged: log and continue on a degraded install; this + // binary does not process untrusted model input, unlike + // pytorch_inference. + if (ml::seccomp::CSystemCallFilter::installSystemCallFilter() != + ml::seccomp::ESystemCallFilterInstallOutcome::E_Installed) { + LOG_WARN(<< "Continuing without full syscall filtering"); + } if (ioMgr.initIo() == false) { LOG_FATAL(<< "Failed to initialise IO"); diff --git a/bin/pytorch_inference/Main.cc b/bin/pytorch_inference/Main.cc index cb0e4393a7..800c7525b6 100644 --- a/bin/pytorch_inference/Main.cc +++ b/bin/pytorch_inference/Main.cc @@ -295,7 +295,33 @@ int main(int argc, char** argv) { // Reduce memory priority before installing system call filters. ml::core::CProcessPriority::reduceMemoryPriority(); - ml::seccomp::CSystemCallFilter::installSystemCallFilter(); + + // Internal switch, not an operator setting: it stays false until the + // controller can route around Sandbox2 explicitly and guarantee that a + // degraded-mode (no-Sandbox2) launch was a deliberate operator choice + // rather than the only option this process has. Flipping it on today + // would terminate every launch on a host lacking seccomp BPF, with no + // operator fallback to select instead. + constexpr bool TERMINATE_ON_DEGRADED_SECCOMP_FAILURE{false}; + + const ml::seccomp::ESystemCallFilterInstallOutcome seccompOutcome{ + ml::seccomp::CSystemCallFilter::installSystemCallFilter()}; + + if (ml::seccomp::decideDegradedModeAction(seccompOutcome, TERMINATE_ON_DEGRADED_SECCOMP_FAILURE) == + ml::seccomp::EDegradedModeAction::E_TerminateBeforeIo) { + LOG_FATAL(<< "Seccomp installation " << ml::seccomp::describe(seccompOutcome) + << "; terminating before untrusted model processing"); + return EXIT_FAILURE; + } + + // Explicit structured attestation the controller/Elasticsearch can + // assert on directly, rather than inferring readiness from the absence + // of a fatal log line above. + const std::string degradedModeMarker{ + ml::seccomp::degradedModeAttestationMarker(seccompOutcome)}; + if (degradedModeMarker.empty() == false) { + LOG_INFO(<< degradedModeMarker); + } if (ioMgr.initIo() == false) { LOG_FATAL(<< "Failed to initialise IO"); diff --git a/include/seccomp/CPytorchInferenceSyscallAllowlist.h b/include/seccomp/CPytorchInferenceSyscallAllowlist.h new file mode 100644 index 0000000000..ae12620006 --- /dev/null +++ b/include/seccomp/CPytorchInferenceSyscallAllowlist.h @@ -0,0 +1,128 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ +#ifndef INCLUDED_ml_seccomp_CPytorchInferenceSyscallAllowlist_h +#define INCLUDED_ml_seccomp_CPytorchInferenceSyscallAllowlist_h + +#include + +#ifdef __linux__ +#include +#endif + +namespace ml { +namespace seccomp { +namespace pytorch_inference { + +#ifdef __linux__ + +// statx, rseq and clone3 won't be defined on a RHEL/CentOS 7 build machine, +// but might exist on the kernel we run on, so fall back to the raw numbers. +#if defined(__x86_64__) +#ifndef __NR_statx +#define ML_NR_statx 332 +#else +#define ML_NR_statx __NR_statx +#endif +#ifndef __NR_rseq +#define ML_NR_rseq 334 +#else +#define ML_NR_rseq __NR_rseq +#endif +#elif defined(__aarch64__) +#ifndef __NR_statx +#define ML_NR_statx 291 +#else +#define ML_NR_statx __NR_statx +#endif +#ifndef __NR_rseq +#define ML_NR_rseq 293 +#else +#define ML_NR_rseq __NR_rseq +#endif +#endif +#ifndef __NR_clone3 +#define ML_NR_clone3 435 +#else +#define ML_NR_clone3 __NR_clone3 +#endif + +//! Syscalls permitted by the legacy in-process BPF filter +//! (CSystemCallFilter_Linux.cc) for every process that installs it, currently +//! shared by pytorch_inference, autodetect, categorize, normalize and +//! data_frame_analyzer. This is the single machine-readable declaration that +//! the applied BPF program is generated from: CSystemCallFilter_Linux.cc +//! contains no independent syscall list and no manually maintained jump +//! offsets. A future Sandbox2 policy is expected to consume the same +//! declaration for its explicit grants, so both mechanisms stay in sync. +//! +//! Carry-forward note: PR #2873 fixed several pytorch_inference/libtorch +//! compatibility gaps the hard way, and this declaration is a rewrite from +//! scratch rather than a copy of that work, so it deliberately keeps two of +//! them. ML_NR_clone3 (see 57f00ed1b) and __NR_prlimit64 (see 03b1ee4a) are +//! carried into this shared declaration so a future Sandbox2 policy +//! inherits them automatically instead of rediscovering them the same way; +//! CSeccompFilterBuilderTest.cc asserts both stay present. The x86_64 +//! legacy filesystem syscalls below (see ec7d3ed85) were already part of +//! this filter's syscall set prior to this declaration and remain +//! unchanged. PR #2873's futex-op broadening (see d9a856d5f) and CI +//! link-order/test-bundle packaging fixes (see 730933db, f8b0a534) apply to +//! the Sandbox2 policy and its Buildkite pipeline respectively, not to this +//! file — carry those forward when that code is written instead of +//! rediscovering them. +inline std::vector legacyBpfAllowedSyscalls() { + std::vector syscalls { +#if defined(__x86_64__) + __NR_access, __NR_open, __NR_dup2, __NR_unlink, __NR_stat, __NR_lstat, + __NR_time, __NR_readlink, __NR_getdents, // for forecast temp storage + __NR_rmdir, // for forecast temp storage + __NR_mkdir, // for forecast temp storage + __NR_mknod, +#elif defined(__aarch64__) + __NR_faccessat, +#endif + __NR_fcntl, // for fdopendir + __NR_getrusage, + __NR_getpid, // for pthread_kill + ML_NR_statx, // for create_directories + __NR_getrandom, // for unique_path + __NR_mknodat, __NR_newfstatat, __NR_readlinkat, __NR_dup3, + __NR_getpriority, // for nice + __NR_setpriority, // for nice + __NR_read, __NR_write, __NR_writev, __NR_lseek, __NR_clock_gettime, + __NR_gettimeofday, __NR_fstat, __NR_close, __NR_connect, + ML_NR_clone3, __NR_clone, __NR_statfs, + __NR_mkdirat, // for forecast temp storage + __NR_unlinkat, // for forecast temp storage + __NR_getdents64, // for forecast temp storage + __NR_openat, // for forecast temp storage + __NR_tgkill, // for the crash handler + __NR_rt_sigaction, // for the crash handler + __NR_rt_sigreturn, + __NR_rt_sigprocmask, // for recent pthread_create + ML_NR_rseq, // for recent pthread_create + __NR_futex, __NR_madvise, __NR_nanosleep, __NR_set_robust_list, + __NR_mprotect, // for malloc arenas and pthread stacks + __NR_mremap, // for malloc arenas + __NR_munmap, // for malloc arenas + __NR_mmap, // for malloc arenas + __NR_getuid, __NR_exit_group, __NR_brk, __NR_exit, + __NR_prlimit64, // libtorch/Sandbox2-monitor query rlimits under load (03b1ee4a) + }; + return syscalls; +} + +#endif // __linux__ + +} // namespace pytorch_inference +} // namespace seccomp +} // namespace ml + +#endif // INCLUDED_ml_seccomp_CPytorchInferenceSyscallAllowlist_h diff --git a/include/seccomp/CSeccompFilterBuilder.h b/include/seccomp/CSeccompFilterBuilder.h new file mode 100644 index 0000000000..fd2941826b --- /dev/null +++ b/include/seccomp/CSeccompFilterBuilder.h @@ -0,0 +1,39 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ +#ifndef INCLUDED_ml_seccomp_CSeccompFilterBuilder_h +#define INCLUDED_ml_seccomp_CSeccompFilterBuilder_h + +#ifdef __linux__ + +#include + +#include + +namespace ml { +namespace seccomp { + +//! Builds a seccomp BPF program that allows exactly allowedSyscalls, on the +//! native architecture only, and denies everything else with EACCES. +//! +//! The caller supplies allowedSyscalls in any order: every generated jump +//! offset is derived from the vector's size and the row's own index, so +//! adding, removing or reordering a syscall never requires updating any +//! other row. This is the mechanism that lets CSystemCallFilter_Linux.cc +//! apply CPytorchInferenceSyscallAllowlist.h's declaration directly, instead +//! of maintaining a second, hand-written BPF program with manual jump +//! offsets that can silently drift from the declaration. +std::vector buildSyscallAllowlistProgram(const std::vector& allowedSyscalls); +} +} + +#endif // __linux__ + +#endif // INCLUDED_ml_seccomp_CSeccompFilterBuilder_h diff --git a/include/seccomp/CSystemCallFilter.h b/include/seccomp/CSystemCallFilter.h index 9855d27002..d1d2e863ad 100644 --- a/include/seccomp/CSystemCallFilter.h +++ b/include/seccomp/CSystemCallFilter.h @@ -13,6 +13,8 @@ #include +#include + namespace ml { namespace seccomp { @@ -41,9 +43,85 @@ namespace seccomp { //! Windows: //! Job Objects prevent the process spawning another. //! +enum class ESystemCallFilterInstallOutcome { + E_Installed, + //! The platform mechanism itself is unavailable (e.g. kernel not built + //! with CONFIG_SECCOMP_FILTER). + E_MechanismUnavailable, + //! The mechanism is available but a required privilege-restriction step + //! failed (e.g. PR_SET_NO_NEW_PRIVS on Linux). + E_PrivilegeRestrictionFailed, + //! The mechanism is available but installing the filter/profile itself + //! failed. + E_FilterInstallFailed +}; + +//! Human-readable description of an install outcome, for diagnostics only; +//! not a stable machine-parsed value. +inline const char* describe(ESystemCallFilterInstallOutcome outcome) { + switch (outcome) { + case ESystemCallFilterInstallOutcome::E_Installed: + return "installed"; + case ESystemCallFilterInstallOutcome::E_MechanismUnavailable: + return "mechanism unavailable"; + case ESystemCallFilterInstallOutcome::E_PrivilegeRestrictionFailed: + return "privilege restriction failed"; + case ESystemCallFilterInstallOutcome::E_FilterInstallFailed: + return "filter install failed"; + } + return "unknown"; +} + +//! What a caller should do, given an install outcome and whether hard +//! termination is currently enabled at that call site. +enum class EDegradedModeAction { + E_ContinueDespiteFailure, + E_TerminateBeforeIo +}; + +//! Pure decision function: does this install outcome require terminating +//! before untrusted IO/model processing? +//! +//! terminateOnFailure is an internal switch, not an operator setting. Every +//! degraded-mode seccomp failure should eventually terminate before +//! processing, but flipping that on for every call site before the +//! ml-cpp/Elasticsearch controller protocol can guarantee a degraded-mode +//! launch was a deliberate operator choice would fail every launch on a +//! host lacking seccomp BPF, with no operator fallback setting to select +//! instead. Callers pass false today; a later change wires the real route +//! decision through this parameter once that guarantee exists. +inline EDegradedModeAction decideDegradedModeAction(ESystemCallFilterInstallOutcome outcome, + bool terminateOnFailure) { + if (outcome == ESystemCallFilterInstallOutcome::E_Installed || !terminateOnFailure) { + return EDegradedModeAction::E_ContinueDespiteFailure; + } + return EDegradedModeAction::E_TerminateBeforeIo; +} + +//! Structured signal a controller/Elasticsearch observer asserts to confirm +//! that a legacy/degraded-mode pytorch_inference launch actually installed +//! its in-process seccomp filter before processing untrusted model input. +//! Replaces attesting readiness by inference — "no fatal log line appeared +//! before initIo() ran" — with an explicit signal a test or observer can +//! assert on directly. Returns empty when +//! installation did not succeed: a failed degraded launch already exits +//! before initIo() (see decideDegradedModeAction()) and must never emit +//! this marker, since doing so would falsely attest a filter that isn't +//! there. Logged over the existing per-process log pipe; this is not a new +//! startup channel. +inline std::string degradedModeAttestationMarker(ESystemCallFilterInstallOutcome outcome) { + if (outcome != ESystemCallFilterInstallOutcome::E_Installed) { + return std::string(); + } + return "{\"ml_sandbox2_route\":\"legacy\",\"event\":\"seccomp_installed\"}"; +} + class CSystemCallFilter : private core::CNonInstantiatable { public: - static void installSystemCallFilter(); + //! Installs the platform syscall filter. Returns the typed outcome so a + //! caller can decide whether to continue or terminate; callers must not + //! silently discard the result (see decideDegradedModeAction()). + [[nodiscard]] static ESystemCallFilterInstallOutcome installSystemCallFilter(); }; } } diff --git a/lib/seccomp/CSystemCallFilter_Linux.cc b/lib/seccomp/CSystemCallFilter_Linux.cc index 466b58cd41..7970c33b56 100644 --- a/lib/seccomp/CSystemCallFilter_Linux.cc +++ b/lib/seccomp/CSystemCallFilter_Linux.cc @@ -8,10 +8,22 @@ * compliance with the Elastic License 2.0 and the foregoing additional * limitation. */ + +/* + * NOTE: This seccomp filter is being gradually replaced by Sandbox2 policies + * for processes that are spawned via CDetachedProcessSpawner. The allowed + * syscall set lives in CPytorchInferenceSyscallAllowlist.h, the single + * machine-readable declaration this filter is generated from; a future + * Sandbox2 policy is expected to consume the same declaration for its + * explicit grants. + */ #include #include +#include +#include + #include #include #include @@ -30,125 +42,64 @@ namespace { // The old x32 ABI always has bit 30 set in the sys call numbers. // The x64 ABI should fail these calls const std::uint32_t UPPER_NR_LIMIT = 0x3FFFFFFF; +} + +std::vector buildSyscallAllowlistProgram(const std::vector& allowedSyscalls) { + const auto numSyscalls = static_cast(allowedSyscalls.size()); + + std::vector program; + program.reserve(numSyscalls + 6); -const struct sock_filter FILTER[] = { // Reject non-native ABIs before matching syscall numbers. Without this, // an x86_64 process can issue int 0x80 (i386) and hit number collisions — - // e.g. i386 socketcall (102) matches the allowlisted x86_64 getuid (102). - // Hardening in response to a privately reported ML seccomp-bypass finding. - // This prefix is self-contained (immediate RET on mismatch) so the relative - // jump offsets in the nr allowlist below are unchanged. - BPF_STMT(BPF_LD | BPF_W | BPF_ABS, offsetof(struct seccomp_data, arch)), + // e.g. i386 socketcall (102) matches an allowlisted x86_64 syscall with + // the same number. Hardening in response to a privately reported ML + // seccomp-bypass finding. This prefix is self-contained (immediate RET + // on mismatch), so it never affects the jump offsets below. + program.push_back( + BPF_STMT(BPF_LD | BPF_W | BPF_ABS, offsetof(struct seccomp_data, arch))); #ifdef __x86_64__ - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, AUDIT_ARCH_X86_64, 1, 0), + program.push_back(BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, AUDIT_ARCH_X86_64, 1, 0)); #elif defined(__aarch64__) - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, AUDIT_ARCH_AARCH64, 1, 0), + program.push_back(BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, AUDIT_ARCH_AARCH64, 1, 0)); +#else +#error Unsupported hardware architecture #endif - BPF_STMT(BPF_RET | BPF_K, SECCOMP_RET_ERRNO | (EACCES & SECCOMP_RET_DATA)), + program.push_back(BPF_STMT(BPF_RET | BPF_K, + SECCOMP_RET_ERRNO | (EACCES & SECCOMP_RET_DATA))); // Load the system call number into accumulator - BPF_STMT(BPF_LD | BPF_W | BPF_ABS, offsetof(struct seccomp_data, nr)), + program.push_back(BPF_STMT(BPF_LD | BPF_W | BPF_ABS, offsetof(struct seccomp_data, nr))); #ifdef __x86_64__ -// The statx, rseq and clone3 syscalls won't be defined on a RHEL/CentOS 7 build -// machine, but might exist on the kernel we run on -#ifndef __NR_statx -#define __NR_statx 332 -#endif -#ifndef __NR_rseq -#define __NR_rseq 334 -#endif -#ifndef __NR_clone3 -#define __NR_clone3 435 -#endif - // Only applies to x86_64 arch. Jump to disallow for calls using the x32 ABI - BPF_JUMP(BPF_JMP | BPF_JGT | BPF_K, UPPER_NR_LIMIT, 56, 0), - // If any sys call filters are added or removed then the jump - // destination for each statement including the one above must - // be updated accordingly - - // Allowed architecture-specific sys calls, jump to return allow on match - // Some of these are not used in latest glibc, and not supported in Linux - // kernels for recent architectures, but in a few cases different sys calls - // are used on different architectures - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_access, 56, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_open, 55, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_dup2, 54, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_unlink, 53, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_stat, 52, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_lstat, 51, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_time, 50, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_readlink, 49, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_getdents, 48, 0), // for forecast temp storage - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_rmdir, 47, 0), // for forecast temp storage - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_mkdir, 46, 0), // for forecast temp storage - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_mknod, 45, 0), -#elif defined(__aarch64__) -// The statx, rseq and clone3 syscalls won't be defined on a RHEL/CentOS 7 build -// machine, but might exist on the kernel we run on -#ifndef __NR_statx -#define __NR_statx 291 -#endif -#ifndef __NR_rseq -#define __NR_rseq 293 -#endif -#ifndef __NR_clone3 -#define __NR_clone3 435 -#endif - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_faccessat, 45, 0), -#else -#error Unsupported hardware architecture + // Jump to the deny row (immediately after the last syscall row below, + // i.e. numSyscalls rows ahead) for calls using the x32 ABI, without + // checking any allowlisted syscall. + program.push_back(BPF_JUMP(BPF_JMP | BPF_JGT | BPF_K, UPPER_NR_LIMIT, + static_cast(numSyscalls), 0)); #endif - // Allowed sys calls for all architectures, jump to return allow on match - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_fcntl, 44, 0), // for fdopendir - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_getrusage, 43, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_getpid, 42, 0), // for pthread_kill - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_statx, 41, 0), // for create_directories - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_getrandom, 40, 0), // for unique_path - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_mknodat, 39, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_newfstatat, 38, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_readlinkat, 37, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_dup3, 36, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_getpriority, 35, 0), // for nice - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_setpriority, 34, 0), // for nice - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_read, 33, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_write, 32, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_writev, 31, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_lseek, 30, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_clock_gettime, 29, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_gettimeofday, 28, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_fstat, 27, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_close, 26, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_connect, 25, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_clone3, 24, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_clone, 23, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_statfs, 22, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_mkdirat, 21, 0), // for forecast temp storage - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_unlinkat, 20, 0), // for forecast temp storage - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_getdents64, 19, 0), // for forecast temp storage - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_openat, 18, 0), // for forecast temp storage - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_tgkill, 17, 0), // for the crash handler - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_rt_sigaction, 16, 0), // for the crash handler - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_rt_sigreturn, 15, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_rt_sigprocmask, 14, 0), // for recent pthread_create - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_rseq, 13, 0), // for recent pthread_create - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_futex, 12, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_madvise, 11, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_nanosleep, 10, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_set_robust_list, 9, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_mprotect, 8, 0), // for malloc arenas and pthread stacks - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_mremap, 7, 0), // for malloc arenas - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_munmap, 6, 0), // for malloc arenas - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_mmap, 5, 0), // for malloc arenas - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_getuid, 4, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_exit_group, 3, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_brk, 2, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_exit, 1, 0), + // Every syscall row jumps to the terminal SECCOMP_RET_ALLOW row on match. + // The jump distance is derived from the row's own index and the total + // count, so adding, removing or reordering an entry in allowedSyscalls + // never requires touching any other row. + for (std::uint32_t i = 0; i < numSyscalls; ++i) { + const auto jumpToAllow = static_cast(numSyscalls - i); + program.push_back(BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, + static_cast(allowedSyscalls[i]), + jumpToAllow, 0)); + } + // Disallow call with error code EACCES - BPF_STMT(BPF_RET | BPF_K, SECCOMP_RET_ERRNO | (EACCES & SECCOMP_RET_DATA)), + program.push_back(BPF_STMT(BPF_RET | BPF_K, + SECCOMP_RET_ERRNO | (EACCES & SECCOMP_RET_DATA))); // Allow call - BPF_STMT(BPF_RET | BPF_K, SECCOMP_RET_ALLOW)}; + program.push_back(BPF_STMT(BPF_RET | BPF_K, SECCOMP_RET_ALLOW)); + + return program; +} + +namespace { bool canUseSeccompBpf() { // This call is expected to fail due to the nullptr argument @@ -170,40 +121,44 @@ bool canUseSeccompBpf() { } } -void CSystemCallFilter::installSystemCallFilter() { - if (canUseSeccompBpf()) { - LOG_DEBUG(<< "Seccomp BPF filters available"); - - // Ensure more permissive privileges cannot be set in future. - // This must be set before installing the filter. - // PR_SET_NO_NEW_PRIVS was aded in kernel 3.5 - if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) { - LOG_ERROR(<< "prctl PR_SET_NO_NEW_PRIVS failed: " << std::strerror(errno)); - return; - } - - struct sock_fprog prog = { - .len = static_cast(sizeof(FILTER) / sizeof(FILTER[0])), - .filter = const_cast(FILTER)}; - - // Install the filter. - // prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, filter) was introduced - // in kernel 3.5. This is functionally equivalent to - // seccomp(SECCOMP_SET_MODE_FILTER, 0, filter) which was added in - // kernel 3.17. We choose the older more compatible function. - // Note this precludes the use of calling seccomp() with the - // SECCOMP_FILTER_FLAG_TSYNC which is acceptable if the filter - // is installed by the main thread before any other threads are - // spawned. - if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) { - LOG_ERROR(<< "Unable to install Seccomp BPF: " << std::strerror(errno)); - } else { - LOG_DEBUG(<< "Seccomp BPF installed"); - } - - } else { +ESystemCallFilterInstallOutcome CSystemCallFilter::installSystemCallFilter() { + if (canUseSeccompBpf() == false) { LOG_DEBUG(<< "Seccomp BPF not available"); + return ESystemCallFilterInstallOutcome::E_MechanismUnavailable; + } + LOG_DEBUG(<< "Seccomp BPF filters available"); + + // Ensure more permissive privileges cannot be set in future. + // This must be set before installing the filter. + // PR_SET_NO_NEW_PRIVS was added in kernel 3.5 + if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) { + LOG_ERROR(<< "prctl PR_SET_NO_NEW_PRIVS failed: " << std::strerror(errno)); + return ESystemCallFilterInstallOutcome::E_PrivilegeRestrictionFailed; } + + const std::vector program{ + buildSyscallAllowlistProgram(pytorch_inference::legacyBpfAllowedSyscalls())}; + + struct sock_fprog prog = {.len = static_cast(program.size()), + .filter = const_cast(program.data())}; + + // Install the filter. + // prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, filter) was introduced + // in kernel 3.5. This is functionally equivalent to + // seccomp(SECCOMP_SET_MODE_FILTER, 0, filter) which was added in + // kernel 3.17. We choose the older more compatible function. + // Note this precludes the use of calling seccomp() with the + // SECCOMP_FILTER_FLAG_TSYNC which is acceptable if the filter + // is installed by the main thread before any other threads are + // spawned. + if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog)) { + LOG_ERROR(<< "Unable to install Seccomp BPF: " << std::strerror(errno)); + return ESystemCallFilterInstallOutcome::E_FilterInstallFailed; + } + + LOG_DEBUG(<< "Seccomp BPF installed"); + LOG_INFO(<< "ml.seccomp.installed"); + return ESystemCallFilterInstallOutcome::E_Installed; } } } diff --git a/lib/seccomp/CSystemCallFilter_MacOSX.cc b/lib/seccomp/CSystemCallFilter_MacOSX.cc index 3756875d06..d308cd3c5e 100644 --- a/lib/seccomp/CSystemCallFilter_MacOSX.cc +++ b/lib/seccomp/CSystemCallFilter_MacOSX.cc @@ -87,13 +87,14 @@ std::string writeTempRulesFile() { } } -void CSystemCallFilter::installSystemCallFilter() { +ESystemCallFilterInstallOutcome CSystemCallFilter::installSystemCallFilter() { std::string profileFilename{writeTempRulesFile()}; if (profileFilename.empty()) { LOG_WARN(<< "Cannot write sandbox rules. macOS sandbox will not be initialized"); - return; + return ESystemCallFilterInstallOutcome::E_MechanismUnavailable; } + ESystemCallFilterInstallOutcome outcome{ESystemCallFilterInstallOutcome::E_Installed}; char* errorbuf{nullptr}; if (::sandbox_init(profileFilename.c_str(), SANDBOX_NAMED, &errorbuf) != 0) { std::string msg("Error initializing macOS sandbox"); @@ -103,11 +104,14 @@ void CSystemCallFilter::installSystemCallFilter() { ::sandbox_free_error(errorbuf); } LOG_ERROR(<< msg); + outcome = ESystemCallFilterInstallOutcome::E_FilterInstallFailed; } else { LOG_DEBUG(<< "macOS sandbox initialized"); + LOG_INFO(<< "ml.seccomp.installed"); } std::remove(profileFilename.c_str()); + return outcome; } } } diff --git a/lib/seccomp/CSystemCallFilter_Windows.cc b/lib/seccomp/CSystemCallFilter_Windows.cc index ce4924c629..16a3928f10 100644 --- a/lib/seccomp/CSystemCallFilter_Windows.cc +++ b/lib/seccomp/CSystemCallFilter_Windows.cc @@ -27,11 +27,11 @@ struct SCheckedHandle { }; } -void CSystemCallFilter::installSystemCallFilter() { +ESystemCallFilterInstallOutcome CSystemCallFilter::installSystemCallFilter() { HANDLE job = CreateJobObject(nullptr, nullptr); if (job == nullptr) { LOG_ERROR(<< "Failed to create Job Object: " << ml::core::CWindowsError()); - return; + return ESystemCallFilterInstallOutcome::E_MechanismUnavailable; } // The job is not destroyed until the handle is closed @@ -44,7 +44,7 @@ void CSystemCallFilter::installSystemCallFilter() { if (QueryInformationJobObject(job, JobObjectBasicLimitInformation, &limits, sizeof(limits), nullptr) == 0) { LOG_ERROR(<< "Error querying Job Object information: " << ml::core::CWindowsError()); - return; + return ESystemCallFilterInstallOutcome::E_FilterInstallFailed; } // Limit the number of active processes to 1 and @@ -54,16 +54,18 @@ void CSystemCallFilter::installSystemCallFilter() { if (SetInformationJobObject(job, JobObjectBasicLimitInformation, &limits, sizeof(limits)) == 0) { LOG_ERROR(<< "Error setting Job information: " << ml::core::CWindowsError()); - return; + return ESystemCallFilterInstallOutcome::E_FilterInstallFailed; } // Assign current process to the job if (AssignProcessToJobObject(job, GetCurrentProcess()) == 0) { LOG_ERROR(<< "Error assigning process to Job Object: " << ml::core::CWindowsError()); - return; + return ESystemCallFilterInstallOutcome::E_FilterInstallFailed; } LOG_DEBUG(<< "ActiveProcessLimit set to 1 for new Job Object"); + LOG_INFO(<< "ml.seccomp.installed"); + return ESystemCallFilterInstallOutcome::E_Installed; } } } diff --git a/lib/seccomp/unittest/CMakeLists.txt b/lib/seccomp/unittest/CMakeLists.txt index 7af78c795c..2656170e85 100644 --- a/lib/seccomp/unittest/CMakeLists.txt +++ b/lib/seccomp/unittest/CMakeLists.txt @@ -13,6 +13,7 @@ project("ML Seccomp unit tests") set (SRCS Main.cc + CSeccompFilterBuilderTest.cc CSystemCallFilterTest.cc ) diff --git a/lib/seccomp/unittest/CSeccompFilterBuilderTest.cc b/lib/seccomp/unittest/CSeccompFilterBuilderTest.cc new file mode 100644 index 0000000000..ce1f63805a --- /dev/null +++ b/lib/seccomp/unittest/CSeccompFilterBuilderTest.cc @@ -0,0 +1,240 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ + +#include + +#include + +#include + +#ifdef __linux__ + +// These must be included before BOOST_AUTO_TEST_SUITE() opens a namespace: +// BOOST_AUTO_TEST_SUITE(name) expands to `namespace name { ... }`, so any +// #include placed after it would get its declarations nested inside that +// namespace instead of at global scope, shadowing ::ml::seccomp with an +// incomplete duplicate. +#include +#include + +#include +#include +#include +#include + +#endif // __linux__ + +BOOST_AUTO_TEST_SUITE(CSeccompFilterBuilderTest) + +#ifdef __linux__ + +namespace { + +//! Decodes the syscall numbers this builder actually applies, by walking the +//! generated program rather than re-reading the declaration it was built +//! from. This is the proof that the applied program matches the +//! declaration, not a comparison of two independently maintained lists. +//! +//! Rows before the syscall-number load (the arch load/check prefix) also use +//! BPF_JMP|BPF_JEQ|BPF_K, so decoding starts only once that load is seen. +std::set decodeAppliedSyscalls(const std::vector& program) { + std::set applied; + bool sawNrLoad{false}; + for (const auto& instr : program) { + if (instr.code == (BPF_LD | BPF_W | BPF_ABS) && + instr.k == offsetof(struct seccomp_data, nr)) { + sawNrLoad = true; + continue; + } + if (sawNrLoad && instr.code == (BPF_JMP | BPF_JEQ | BPF_K) && instr.jt > 0) { + applied.insert(static_cast(instr.k)); + } + } + return applied; +} + +} // namespace + +BOOST_AUTO_TEST_CASE(testAppliedProgramMatchesDeclaration) { + const std::vector declared{ml::seccomp::pytorch_inference::legacyBpfAllowedSyscalls()}; + const std::vector program{ml::seccomp::buildSyscallAllowlistProgram(declared)}; + + const std::set declaredSet{declared.begin(), declared.end()}; + BOOST_REQUIRE_EQUAL(declaredSet.size(), declared.size()); // declaration has no duplicates + const std::set appliedSet{decodeAppliedSyscalls(program)}; + BOOST_REQUIRE_EQUAL_COLLECTIONS(declaredSet.begin(), declaredSet.end(), + appliedSet.begin(), appliedSet.end()); + + // Structural invariants that must hold regardless of declaration content: + // native-arch load/check, syscall-number load, and a final deny/allow + // pair. No index into this vector is hand-maintained anywhere in + // production code. + BOOST_TEST_REQUIRE(program.size() >= declared.size() + 4); + BOOST_REQUIRE_EQUAL(static_cast(BPF_RET | BPF_K), + static_cast(program.back().code)); + BOOST_REQUIRE_EQUAL(static_cast(SECCOMP_RET_ALLOW), + program.back().k); + const auto& denyRow = program[program.size() - 2]; + BOOST_REQUIRE_EQUAL(static_cast(BPF_RET | BPF_K), + static_cast(denyRow.code)); + BOOST_TEST_REQUIRE(denyRow.k != SECCOMP_RET_ALLOW); +} + +BOOST_AUTO_TEST_CASE(testJumpOffsetsAreDerivedNotHandMaintained) { + // An arbitrary, deliberately unordered and out-of-production-order list. + // If any jump offset were hand-maintained rather than derived from the + // vector's size/index, reordering or resizing this list would desync it + // from the generated rows; this test would fail with a stale allowlist + // but pass immediately once regenerated, which is exactly the property + // "no manual BPF jump offsets remain" requires. + const std::vector arbitrarySyscalls{200, 1, 57, 9, 300}; + const std::vector program{ + ml::seccomp::buildSyscallAllowlistProgram(arbitrarySyscalls)}; + + const std::size_t allowIndex{program.size() - 1}; + const std::size_t denyIndex{program.size() - 2}; + BOOST_REQUIRE_EQUAL(static_cast(SECCOMP_RET_ALLOW), + program[allowIndex].k); + BOOST_TEST_REQUIRE(program[denyIndex].k != SECCOMP_RET_ALLOW); + + // Every syscall row's own jt must land exactly on the allow row: for a + // row at absolute index i, i + jt + 1 == allowIndex. The arch load/check + // prefix also uses BPF_JMP|BPF_JEQ|BPF_K but targets the nr-load + // instruction, not the allow row, so decoding starts only after the + // syscall-number load is seen (mirrors decodeAppliedSyscalls() above). + std::set foundSyscalls; + bool sawNrLoad{false}; + for (std::size_t i = 0; i < program.size(); ++i) { + if (program[i].code == (BPF_LD | BPF_W | BPF_ABS) && + program[i].k == offsetof(struct seccomp_data, nr)) { + sawNrLoad = true; + continue; + } + if (sawNrLoad && program[i].code == (BPF_JMP | BPF_JEQ | BPF_K) && + program[i].jt > 0) { + BOOST_REQUIRE_EQUAL(allowIndex, i + program[i].jt + 1); + foundSyscalls.insert(static_cast(program[i].k)); + } + } + const std::set expected{arbitrarySyscalls.begin(), arbitrarySyscalls.end()}; + BOOST_REQUIRE_EQUAL_COLLECTIONS(expected.begin(), expected.end(), + foundSyscalls.begin(), foundSyscalls.end()); +} + +BOOST_AUTO_TEST_CASE(testArchGuardRejectsNonNativeAbi) { + const std::vector program{ + ml::seccomp::buildSyscallAllowlistProgram(std::vector{1})}; + + BOOST_TEST_REQUIRE(program.size() >= 3); + BOOST_REQUIRE_EQUAL(static_cast(BPF_LD | BPF_W | BPF_ABS), + static_cast(program[0].code)); + BOOST_REQUIRE_EQUAL(static_cast(offsetof(struct seccomp_data, arch)), + program[0].k); + BOOST_REQUIRE_EQUAL(static_cast(BPF_JMP | BPF_JEQ | BPF_K), + static_cast(program[1].code)); +#ifdef __x86_64__ + BOOST_REQUIRE_EQUAL(static_cast(AUDIT_ARCH_X86_64), program[1].k); +#elif defined(__aarch64__) + BOOST_REQUIRE_EQUAL(static_cast(AUDIT_ARCH_AARCH64), + program[1].k); +#endif + BOOST_REQUIRE_EQUAL(static_cast(BPF_RET | BPF_K), + static_cast(program[2].code)); + BOOST_TEST_REQUIRE(program[2].k != SECCOMP_RET_ALLOW); +} + +BOOST_AUTO_TEST_CASE(testCarryForwardSyscallsPresent) { + // PR #2873 fixed these pytorch_inference/libtorch compatibility gaps + // the hard way; this declaration is a rewrite from scratch, and must + // not silently drop them. Each assertion below is a named regression + // test for one carried-forward fix within this file's scope. + const std::set declared{ + ml::seccomp::pytorch_inference::legacyBpfAllowedSyscalls().begin(), + ml::seccomp::pytorch_inference::legacyBpfAllowedSyscalls().end()}; + + // 57f00ed1b: clone3 must be allowed by its literal syscall number (435 on + // both x86_64 and aarch64), not only via __NR_clone3, because some build + // images' kernel headers predate clone3 while the runtime glibc uses it. + BOOST_TEST_REQUIRE(declared.count(435) == 1); + + // 03b1ee4a: prlimit64, queried by libtorch/the Sandbox2 monitor under + // sustained load. + BOOST_TEST_REQUIRE(declared.count(__NR_prlimit64) == 1); + +#ifdef __x86_64__ + // ec7d3ed85: glibc's x86_64 file-system wrappers issue these legacy + // syscalls (not their *at equivalents) when pytorch_inference creates + // and tears down its named pipes. + const int legacyFsSyscalls[]{__NR_mknod, __NR_unlink, __NR_rmdir, + __NR_mkdir, __NR_readlink, __NR_access, + __NR_dup2}; + for (int nr : legacyFsSyscalls) { + BOOST_TEST_REQUIRE(declared.count(nr) == 1); + } +#endif +} + +#endif // __linux__ + +BOOST_AUTO_TEST_CASE(testDegradedModeAttestationMarker) { + using ml::seccomp::ESystemCallFilterInstallOutcome; + using ml::seccomp::degradedModeAttestationMarker; + + // The marker must be present and exact on success - this is what a + // controller/Elasticsearch observer asserts, replacing "no fatal log + // line appeared" as an implicit readiness signal. + BOOST_REQUIRE_EQUAL( + std::string("{\"ml_sandbox2_route\":\"legacy\",\"event\":\"seccomp_installed\"}"), + degradedModeAttestationMarker(ESystemCallFilterInstallOutcome::E_Installed)); + + // Every failure class must attest nothing - a caller that logged this + // marker on a failed install would falsely claim protection that isn't + // there. + BOOST_TEST_REQUIRE(degradedModeAttestationMarker(ESystemCallFilterInstallOutcome::E_MechanismUnavailable) + .empty()); + BOOST_TEST_REQUIRE(degradedModeAttestationMarker(ESystemCallFilterInstallOutcome::E_PrivilegeRestrictionFailed) + .empty()); + BOOST_TEST_REQUIRE(degradedModeAttestationMarker(ESystemCallFilterInstallOutcome::E_FilterInstallFailed) + .empty()); +} + +BOOST_AUTO_TEST_CASE(testDecideDegradedModeActionFaultInjection) { + using ml::seccomp::EDegradedModeAction; + using ml::seccomp::ESystemCallFilterInstallOutcome; + using ml::seccomp::decideDegradedModeAction; + + // Successful installation never terminates, regardless of the switch. + BOOST_REQUIRE_EQUAL(static_cast(EDegradedModeAction::E_ContinueDespiteFailure), + static_cast(decideDegradedModeAction( + ESystemCallFilterInstallOutcome::E_Installed, false))); + BOOST_REQUIRE_EQUAL(static_cast(EDegradedModeAction::E_ContinueDespiteFailure), + static_cast(decideDegradedModeAction( + ESystemCallFilterInstallOutcome::E_Installed, true))); + + // Every fault-injected failure class - capability probe failure, + // PR_SET_NO_NEW_PRIVS, and filter installation - with the internal + // switch off (today's production default), every call site continues; + // with it on (the behaviour a later change activates), every one + // terminates. + const ESystemCallFilterInstallOutcome failureModes[]{ + ESystemCallFilterInstallOutcome::E_MechanismUnavailable, + ESystemCallFilterInstallOutcome::E_PrivilegeRestrictionFailed, + ESystemCallFilterInstallOutcome::E_FilterInstallFailed}; + + for (const auto outcome : failureModes) { + BOOST_REQUIRE_EQUAL(static_cast(EDegradedModeAction::E_ContinueDespiteFailure), + static_cast(decideDegradedModeAction(outcome, false))); + BOOST_REQUIRE_EQUAL(static_cast(EDegradedModeAction::E_TerminateBeforeIo), + static_cast(decideDegradedModeAction(outcome, true))); + } +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/lib/seccomp/unittest/CSystemCallFilterTest.cc b/lib/seccomp/unittest/CSystemCallFilterTest.cc index a9024673d1..dd3983571f 100644 --- a/lib/seccomp/unittest/CSystemCallFilterTest.cc +++ b/lib/seccomp/unittest/CSystemCallFilterTest.cc @@ -275,7 +275,9 @@ BOOST_AUTO_TEST_CASE(testSystemCallFilter) { #endif // Install the filter - ml::seccomp::CSystemCallFilter::installSystemCallFilter(); + BOOST_REQUIRE_EQUAL( + static_cast(ml::seccomp::ESystemCallFilterInstallOutcome::E_Installed), + static_cast(ml::seccomp::CSystemCallFilter::installSystemCallFilter())); #if defined(Linux) && defined(__x86_64__) if (i386CompatUsable) { From 79d4173e5d98aa7fa1a295081580427d8fab95b3 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 15:17:00 +0200 Subject: [PATCH 02/15] [ML] Typed filesystem/network launch policy for Sandbox2 pytorch_inference Replace raw argument-directory inference with a typed launch spec that validates every input/output/restore/logPipe path against the pinned child-root contract ($TMPDIR/ml-child-ipc/): rejects relative, root, out-of-root, dot-dot, duplicate, and mutable-symlink/alias paths before any policy is built. Adds the minimized fixed-mount enumeration (narrows /etc to individually justified files, never binds host /proc//sys), a private bounded tmpfs at /tmp, and a purpose-built allowlisted mechanism probe (ml_sandbox_probe) proving allowed IPC access, denied host reads/writes, mount enumeration, and external-egress denial. Validator logic verified standalone on this host (non-Sandbox2 path): compiles clean with -Wall -Wextra, and a driver exercising every V16 rejection case (relative/root/dot-dot/duplicate/aliased/wrong-depth/ child-id-mismatch) plus the valid multi-pipe case all pass. The Sandbox2-gated PolicyBuilder path and the Linux mechanism-probe integration test are unverified in this session (no Linux/Sandbox2 toolchain on this host) and need a devbox or Buildkite pass before V4/V5(policy half)/V7/V16 can be marked closed - see elastic-workspace-3b59.4. --- .../sandbox/CPytorchInferenceSandboxPolicy.h | 144 ++++++++ lib/sandbox/CMakeLists.txt | 14 +- lib/sandbox/CPytorchInferenceSandboxPolicy.cc | 322 ++++++++++++++++++ lib/sandbox/unittest/CMakeLists.txt | 21 ++ ...ferenceSandboxPolicyMechanismTest_Linux.cc | 139 ++++++++ .../CPytorchInferenceSandboxPolicyTest.cc | 237 +++++++++++++ .../unittest/payloads/ml_sandbox_probe.cc | 177 ++++++++++ 7 files changed, 1048 insertions(+), 6 deletions(-) create mode 100644 include/sandbox/CPytorchInferenceSandboxPolicy.h create mode 100644 lib/sandbox/CPytorchInferenceSandboxPolicy.cc create mode 100644 lib/sandbox/unittest/CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc create mode 100644 lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc create mode 100644 lib/sandbox/unittest/payloads/ml_sandbox_probe.cc diff --git a/include/sandbox/CPytorchInferenceSandboxPolicy.h b/include/sandbox/CPytorchInferenceSandboxPolicy.h new file mode 100644 index 0000000000..00564b547d --- /dev/null +++ b/include/sandbox/CPytorchInferenceSandboxPolicy.h @@ -0,0 +1,144 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ +#ifndef INCLUDED_ml_sandbox_CPytorchInferenceSandboxPolicy_h +#define INCLUDED_ml_sandbox_CPytorchInferenceSandboxPolicy_h + +#include +#include + +#ifdef SANDBOX2_AVAILABLE +#include +#endif + +namespace ml { +namespace sandbox { + +//! Reasons a path-bearing launch argument fails typed validation against the +//! pinned child-root contract (docs/projects/mlcpp-sandbox2-pr2873/design.md +//! Sandbox2 clean rebuild plan, PR C, gate V16). Every value here must fail +//! *before* a policy is constructed; none of them widen a mount to recover. +enum class EChildIpcPathRejection { + E_UnrecognizedOption, //!< option name is not input/output/restore/logPipe. + E_NotAbsolute, //!< value does not start with '/'. + E_RootLevelPath, //!< value has no mountable parent directory below '/'. + E_ContainsDotDot, //!< value has a ".." path component. + E_CanonicalizationFailed, //!< realpath() could not resolve the parent directory. + E_OutsideTrustedBase, //!< canonical parent is not beneath the trusted $TMPDIR. + E_WrongDepth, //!< canonical parent is not exactly $TMPDIR/ml-child-ipc/. + E_ChildIdMismatch, //!< two path options resolved to a different . + E_MutableSymlinkOrAlias, //!< the literal and canonical parent directories diverge. + E_Duplicate //!< the same literal argument was supplied more than once. +}; + +//! One rejected path-bearing argument and why. +struct SRejectedChildIpcPath { + std::string s_Arg; + EChildIpcPathRejection s_Reason; +}; + +//! A typed, validated launch specification for a single sandboxed +//! pytorch_inference child, derived from its path-bearing launch options +//! (input, output, restore, logPipe). Replaces raw argument-directory +//! inference: every accepted path is provably beneath the one pinned +//! per-child IPC root, never inferred from arbitrary argv content. +struct SChildIpcLaunchSpec { + //! path component shared by every accepted path option. + //! Empty iff no recognized path option was present in the command line. + std::string s_ChildId; + //! Canonical $TMPDIR/ml-child-ipc/ - the directory the native + //! controller creates (mode 0700) before policy construction, and the + //! only host directory CSandboxedProcessSpawner maps to + //! /run/elastic/ml-ipc. Empty iff s_ChildId is empty. + std::string s_ChildIpcRoot; + //! Canonical paths of every accepted path-bearing argument, always + //! s_ChildIpcRoot plus exactly one leaf component. + std::vector s_PipePaths; +}; + +//! Result of validating a pytorch_inference launch command line against the +//! pinned child-root contract. +struct SChildIpcValidationResult { + //! True only when at least one path option was present and every + //! path option that was present was accepted. False means the caller + //! must fail the spawn - never fall back to a partially-built policy. + bool s_Ok = false; + SChildIpcLaunchSpec s_Spec; + std::vector s_Rejected; +}; + +//! Validate every input/output/restore/logPipe argument in args against the +//! pinned child-root contract: each must canonicalize to a parent directory +//! of exactly trustedTmpDir/ml-child-ipc/, for one consistent +//! , with no ".."; no relative, root, or out-of-root path; no +//! divergent literal/canonical parent; and no duplicate literal argument. +//! Scalar (non path-bearing) options are never inspected as candidate paths. +//! trustedTmpDir must already be the canonical form of the operator's +//! Environment.tmpDir(); this function does not itself decide what counts +//! as trusted. +SChildIpcValidationResult validateChildIpcLaunchSpec(const std::string& trustedTmpDir, + const std::vector& args); + +#ifdef SANDBOX2_AVAILABLE + +//! What buildPytorchInferenceFilesystemPolicy does with one of the seven +//! historically bulk-mounted fixed directories +//! (/lib /lib64 /usr/lib /usr/lib64 /etc /proc /sys). See design.md +//! §Filesystem and IPC policy: whole /etc and a host /proc/sys bind are +//! non-conformant. +enum class EFixedMountAction { + E_MountReadOnlyDirectory, //!< the whole directory is demonstrated necessary read-only. + E_MountNamespacedProcfs, //!< Sandbox2 supplies this inside the sandbox's own PID/mount namespace; never bind the host directory. + E_Skip //!< not mapped at all; narrower entries (files) are added separately. +}; + +//! One fixed-mount decision plus the reason it is scoped that way. +struct SFixedMountDecision { + std::string s_Path; + EFixedMountAction s_Action; + std::string s_Reason; +}; + +//! The minimization decision PR C applies to each of the seven historically +//! bulk-mounted fixed directories, with its justification. /etc is Skip +//! (see allowlistedEtcFiles() for the narrower replacement); /proc and /sys +//! are the Sandbox2-namespaced procfs/sysfs, never a host bind (see +//! namespacedProcfsConformant() for the runtime assertion that this actually +//! held for a given launch). /lib, /lib64, /usr/lib, /usr/lib64 remain whole +//! read-only directories: the dynamic loader resolves libtorch/glibc shared +//! objects from them at runtime from an unbounded, platform-dependent set, +//! so per-file allowlisting would duplicate the loader's own search logic. +const std::vector& fixedMountDecisions(); + +//! Individual /etc files pytorch_inference/libtorch are demonstrated to +//! need, replacing a whole-/etc bind. Extend only with a named consumer. +const std::vector& allowlistedEtcFiles(); + +//! Builds the filesystem and network-shape portion of the pytorch_inference +//! Sandbox2 policy: minimized fixed mounts (fixedMountDecisions, +//! allowlistedEtcFiles), a private bounded tmpfs at /tmp, the one per-child +//! IPC root mapped to /run/elastic/ml-ipc, and the syscall allowlist shared +//! with the legacy BPF filter +//! (seccomp::pytorch_inference::legacyBpfAllowedSyscalls, kept in sync per +//! that header's own comment). Does not call TryBuild() - the caller owns +//! final policy construction so tests can inspect the builder before +//! commit. spec must already be s_Ok from validateChildIpcLaunchSpec; this +//! function does not re-validate it. +sandbox2::PolicyBuilder buildPytorchInferenceFilesystemPolicy(const std::string& binDir, + const std::string& libDir, + const SChildIpcLaunchSpec& spec, + std::size_t tmpfsSizeBytes); + +#endif // SANDBOX2_AVAILABLE + +} // namespace sandbox +} // namespace ml + +#endif // INCLUDED_ml_sandbox_CPytorchInferenceSandboxPolicy_h diff --git a/lib/sandbox/CMakeLists.txt b/lib/sandbox/CMakeLists.txt index 8b2b1fcae1..1c5205843d 100644 --- a/lib/sandbox/CMakeLists.txt +++ b/lib/sandbox/CMakeLists.txt @@ -9,21 +9,23 @@ # limitation. # -# MlSandbox is a dormant dependency foundation (PR A of the Sandbox2 clean -# rebuild plan, see docs/projects/mlcpp-sandbox2-pr2873 in the -# elastic-workspace harness). It links Sandbox2/Abseil and builds/tests a -# runnable Sandbox2 forkserver on Linux, but no controller or -# pytorch_inference routing depends on it yet. The typed launch policy, -# process spawner, and controller wiring land in later PRs of that plan. +# MlSandbox links Sandbox2/Abseil and builds a runnable Sandbox2 forkserver +# on Linux (PR A), and now the typed filesystem/network launch policy (PR C +# of the Sandbox2 clean rebuild plan, see docs/projects/mlcpp-sandbox2-pr2873 +# in the elastic-workspace harness). No controller or pytorch_inference +# routing depends on it yet - the process spawner and controller wiring +# land in later PRs of that plan. project("ML Sandbox") set(ML_LINK_LIBRARIES MlCore + MlSeccomp ) set(SRCS CMlSandboxAvailability.cc + CPytorchInferenceSandboxPolicy.cc ) ml_add_library(MlSandbox STATIC ${SRCS}) diff --git a/lib/sandbox/CPytorchInferenceSandboxPolicy.cc b/lib/sandbox/CPytorchInferenceSandboxPolicy.cc new file mode 100644 index 0000000000..4b873ee0c0 --- /dev/null +++ b/lib/sandbox/CPytorchInferenceSandboxPolicy.cc @@ -0,0 +1,322 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ +#include + +#include +#include + +#include +#include + +#ifdef SANDBOX2_AVAILABLE +#include +#endif + +#ifdef __linux__ +#include +#endif + +namespace ml { +namespace sandbox { + +namespace { + +//! The only recognized path-bearing launch options. Adding or renaming one +//! requires a change here, a policy test, and an end-to-end Elasticsearch +//! invocation test (design.md §Typed argument extraction). +bool isPathOptionName(const std::string& name) { + return name == "input" || name == "output" || name == "restore" || name == "logPipe"; +} + +//! Split a path into components, without resolving "." or "..". +std::vector splitPathComponents(const std::string& path) { + std::vector components; + std::string current; + for (char c : path) { + if (c == '/') { + if (current.empty() == false) { + components.push_back(current); + current.clear(); + } + } else { + current.push_back(c); + } + } + if (current.empty() == false) { + components.push_back(current); + } + return components; +} + +bool containsDotDot(const std::vector& components) { + return std::find(components.begin(), components.end(), "..") != components.end(); +} + +//! realpath() requires the target to exist. The leaf FIFO/file may not +//! exist yet at validation time, but the native controller creates the +//! per-child ml-child-ipc/ directory before policy construction +//! (design.md §Required layout), so canonicalizing the *parent* directory +//! of the leaf is always meaningful. +bool canonicalize(const std::string& dir, std::string& canonicalOut) { + char resolved[PATH_MAX]; + if (::realpath(dir.c_str(), resolved) == nullptr) { + return false; + } + canonicalOut.assign(resolved); + return true; +} + +} // namespace + +SChildIpcValidationResult validateChildIpcLaunchSpec(const std::string& trustedTmpDir, + const std::vector& args) { + SChildIpcValidationResult result; + + std::string trustedTmpDirCanonical; + const bool trustedBaseResolved = canonicalize(trustedTmpDir, trustedTmpDirCanonical); + + std::vector seenLiteralArgs; + + for (const std::string& arg : args) { + const std::size_t eqPos = arg.find('='); + if (eqPos == std::string::npos || eqPos + 1 >= arg.size()) { + continue; + } + + std::string optionName{arg.substr(0, eqPos)}; + while (optionName.empty() == false && optionName[0] == '-') { + optionName.erase(0, 1); + } + + if (isPathOptionName(optionName) == false) { + continue; + } + + const std::string value{arg.substr(eqPos + 1)}; + + if (std::find(seenLiteralArgs.begin(), seenLiteralArgs.end(), arg) != seenLiteralArgs.end()) { + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_Duplicate}); + continue; + } + seenLiteralArgs.push_back(arg); + + if (value.empty() || value[0] != '/') { + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_NotAbsolute}); + continue; + } + + const std::vector components{splitPathComponents(value)}; + if (containsDotDot(components)) { + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_ContainsDotDot}); + continue; + } + if (components.size() < 2) { + // Fewer than two components below '/' means either the root + // itself or a direct child of root - never a valid three-deep + // $TMPDIR/ml-child-ipc// path. + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_RootLevelPath}); + continue; + } + + const std::string leaf{components.back()}; + const std::size_t lastSlash = value.rfind('/'); + const std::string literalParent{value.substr(0, lastSlash)}; + + if (trustedBaseResolved == false) { + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_CanonicalizationFailed}); + continue; + } + + std::string canonicalParent; + if (canonicalize(literalParent, canonicalParent) == false) { + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_CanonicalizationFailed}); + continue; + } + + if (literalParent != canonicalParent) { + // The literal path traverses a symlink (or other alias) before + // reaching its parent directory. Accepting both forms - as the + // pre-PR-C raw inference did - would let a mutable link widen + // the mount after validation ran. Reject instead of mounting + // either form. + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_MutableSymlinkOrAlias}); + continue; + } + + const std::vector canonicalComponents{splitPathComponents(canonicalParent)}; + const std::vector baseComponents{splitPathComponents(trustedTmpDirCanonical)}; + + const bool underBase = canonicalComponents.size() == baseComponents.size() + 2 && + std::equal(baseComponents.begin(), baseComponents.end(), + canonicalComponents.begin()); + if (underBase == false) { + const bool sharesBasePrefix = + canonicalComponents.size() >= baseComponents.size() && + std::equal(baseComponents.begin(), baseComponents.end(), canonicalComponents.begin()); + result.s_Rejected.push_back( + {arg, sharesBasePrefix ? EChildIpcPathRejection::E_WrongDepth + : EChildIpcPathRejection::E_OutsideTrustedBase}); + continue; + } + + const std::string intermediateDir{canonicalComponents[baseComponents.size()]}; + if (intermediateDir != "ml-child-ipc") { + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_WrongDepth}); + continue; + } + + const std::string childId{canonicalComponents.back()}; + if (result.s_Spec.s_ChildId.empty() == false && result.s_Spec.s_ChildId != childId) { + result.s_Rejected.push_back({arg, EChildIpcPathRejection::E_ChildIdMismatch}); + continue; + } + + result.s_Spec.s_ChildId = childId; + result.s_Spec.s_ChildIpcRoot = canonicalParent; + result.s_Spec.s_PipePaths.push_back(canonicalParent + "/" + leaf); + } + + result.s_Ok = result.s_Rejected.empty() && result.s_Spec.s_ChildId.empty() == false; + if (result.s_Ok == false) { + // A rejected argument or an entirely absent path option both fail + // the spawn; never return a partially-populated spec the caller + // might build a policy from by mistake. + result.s_Spec = SChildIpcLaunchSpec{}; + } + return result; +} + +#ifdef SANDBOX2_AVAILABLE + +const std::vector& fixedMountDecisions() { + static const std::vector DECISIONS{ + {"/lib", EFixedMountAction::E_MountReadOnlyDirectory, + "Dynamic loader resolves libc/libgcc/libstdc++ from here at " + "runtime; the set is unbounded and platform-dependent, so " + "per-file allowlisting would duplicate the loader's own search " + "logic."}, + {"/lib64", EFixedMountAction::E_MountReadOnlyDirectory, + "Same reason as /lib, on the lib64 multilib path used by the " + "64-bit dynamic loader on our supported Linux distributions."}, + {"/usr/lib", EFixedMountAction::E_MountReadOnlyDirectory, + "Same reason as /lib: libtorch and its transitive shared-library " + "dependencies resolve from here."}, + {"/usr/lib64", EFixedMountAction::E_MountReadOnlyDirectory, + "Same reason as /lib64, for 64-bit multilib packages."}, + {"/etc", EFixedMountAction::E_Skip, + "Whole /etc is never mounted; allowlistedEtcFiles() lists the " + "individually justified files pytorch_inference/libtorch actually " + "need instead."}, + {"/proc", EFixedMountAction::E_MountNamespacedProcfs, + "Sandbox2 mounts a fresh procfs inside the sandbox's own PID " + "namespace; binding the host's /proc would leak every other " + "process's memory maps and command lines into the sandbox."}, + {"/sys", EFixedMountAction::E_MountNamespacedProcfs, + "Same reason as /proc: nothing in this policy binds host /sys."}, + }; + return DECISIONS; +} + +const std::vector& allowlistedEtcFiles() { + // NOTE: /etc/ssl/certs/ca-certificates.crt is the Debian/Ubuntu trust + // bundle path; the ml-cpp CI build image is CentOS7/RHEL-based, whose + // equivalent is /etc/pki/tls/certs/ca-bundle.crt. This list has not yet + // been verified against the actual supported-distro trust bundle path - + // tracked as an open item for PR C's Linux verification pass, not + // resolved here. + static const std::vector FILES{ + "/etc/nsswitch.conf", + "/etc/resolv.conf", + "/etc/hosts", + "/etc/localtime", + "/etc/ld.so.cache", + }; + return FILES; +} + +sandbox2::PolicyBuilder buildPytorchInferenceFilesystemPolicy(const std::string& binDir, + const std::string& libDir, + const SChildIpcLaunchSpec& spec, + std::size_t tmpfsSizeBytes) { + sandbox2::PolicyBuilder policyBuilder; + + policyBuilder.AllowDynamicStartup() + .AllowExit() + .AllowHandleSignals() + .AllowGetPIDs() + .AllowGetRandom() + .AllowTcMalloc() + .AllowMmap(); + +#ifdef __linux__ + // glibc/libtorch use futex for mutexes and condition variables; timed + // waits and broadcast/requeue paths need more than plain WAIT/WAKE (see + // the carry-forward note on d9a856d5f in + // include/seccomp/CPytorchInferenceSyscallAllowlist.h). + policyBuilder.AllowFutexOp(FUTEX_WAIT) + .AllowFutexOp(FUTEX_WAKE) + .AllowFutexOp(FUTEX_WAIT_BITSET) + .AllowFutexOp(FUTEX_WAKE_BITSET) + .AllowFutexOp(FUTEX_REQUEUE) + .AllowFutexOp(FUTEX_CMP_REQUEUE) + .AllowFutexOp(FUTEX_WAKE_OP); +#endif + + // Consume the one machine-readable syscall declaration shared with the + // legacy in-process BPF filter instead of hand-maintaining a second + // list (design.md "Locked design decisions": "One machine-readable + // declaration generates the applied legacy BPF allowlist and Sandbox2 + // explicit grants"). + for (int syscallNr : seccomp::pytorch_inference::legacyBpfAllowedSyscalls()) { + policyBuilder.AllowSyscall(syscallNr); + } + + policyBuilder.AddDirectory(binDir, /*is_ro=*/true); + policyBuilder.AddDirectory(libDir, /*is_ro=*/true); + + for (const SFixedMountDecision& decision : fixedMountDecisions()) { + switch (decision.s_Action) { + case EFixedMountAction::E_MountReadOnlyDirectory: + policyBuilder.AddDirectory(decision.s_Path, /*is_ro=*/true); + break; + case EFixedMountAction::E_MountNamespacedProcfs: + case EFixedMountAction::E_Skip: + // Sandbox2 supplies its own namespaced procfs/sysfs + // automatically; nothing to add here for either case, and + // adding decision.s_Path would bind the host directory instead. + break; + } + } + + for (const std::string& etcFile : allowlistedEtcFiles()) { + policyBuilder.AddFile(etcFile, /*is_ro=*/true); + } + + for (const std::string& devFile : {"/dev/null", "/dev/urandom", "/dev/random"}) { + policyBuilder.AddFile(devFile, /*is_ro=*/devFile != std::string{"/dev/null"}); + } + + // Private, bounded tmpfs - never the host's shared /tmp. + policyBuilder.AddTmpfs("/tmp", tmpfsSizeBytes); + + // The one per-child IPC root, mapped read-write to a fixed in-sandbox + // path. spec must already be s_Ok (validateChildIpcLaunchSpec), so + // s_ChildIpcRoot is exactly $TMPDIR/ml-child-ipc/ - never + // ml-child-ipc itself, never a sibling child's directory. + policyBuilder.AddDirectoryAt(spec.s_ChildIpcRoot, "/run/elastic/ml-ipc", /*is_ro=*/false); + + return policyBuilder; +} + +#endif // SANDBOX2_AVAILABLE + +} // namespace sandbox +} // namespace ml diff --git a/lib/sandbox/unittest/CMakeLists.txt b/lib/sandbox/unittest/CMakeLists.txt index 82b68e665b..76d542402a 100644 --- a/lib/sandbox/unittest/CMakeLists.txt +++ b/lib/sandbox/unittest/CMakeLists.txt @@ -14,12 +14,14 @@ project("ML Sandbox unit tests") set(SRCS Main.cc CMlSandboxAvailabilityTest.cc + CPytorchInferenceSandboxPolicyTest.cc ) set(ML_LINK_LIBRARIES ${Boost_LIBRARIES_WITH_UNIT_TEST} MlCore MlSandbox + MlSeccomp MlTest ) @@ -31,6 +33,7 @@ if(TARGET sandbox2::sandbox2 AND CMAKE_SYSTEM_NAME STREQUAL "Linux") # elsewhere rather than compiled out with #ifdef, since sandbox2 headers # are unavailable on non-Linux configure runs. list(APPEND SRCS CSandboxForkserverSmokeTest.cc) + list(APPEND SRCS CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc) list(APPEND ML_LINK_LIBRARIES sandbox2::sandbox2) # Deliberately-dependency-free sandboxee payload for the smoke test above. @@ -50,6 +53,17 @@ if(TARGET sandbox2::sandbox2 AND CMAKE_SYSTEM_NAME STREQUAL "Linux") POSITION_INDEPENDENT_CODE TRUE RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/payloads ) + + # PR C's purpose-built allowlisted mechanism-probe payload (design.md + # gates V4/V7/V16). Same dependency-free, dynamically-linked pattern as + # sandbox2_smoke_payload above, for the same CI-image reason. + add_executable(ml_sandbox_probe EXCLUDE_FROM_ALL + payloads/ml_sandbox_probe.cc + ) + set_target_properties(ml_sandbox_probe PROPERTIES + POSITION_INDEPENDENT_CODE TRUE + RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/payloads + ) endif() ml_add_test_executable(sandbox ${SRCS}) @@ -60,3 +74,10 @@ if(TARGET sandbox2_smoke_payload) ML_SANDBOX2_SMOKE_PAYLOAD="$" ) endif() + +if(TARGET ml_sandbox_probe) + add_dependencies(ml_test_sandbox ml_sandbox_probe) + target_compile_definitions(ml_test_sandbox PRIVATE + ML_SANDBOX2_PROBE_PAYLOAD="$" + ) +endif() diff --git a/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc b/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc new file mode 100644 index 0000000000..e0f5ffe01d --- /dev/null +++ b/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc @@ -0,0 +1,139 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ + +// Linux-only mechanism-probe integration test for PR C of +// docs/projects/mlcpp-sandbox2-pr2873 (design.md gates V4, V7, and the +// policy half of V5). Builds a real filesystem/network policy via +// buildPytorchInferenceFilesystemPolicy, runs ml_sandbox_probe inside it, +// and asserts on the probe's per-mechanism "outcome=" lines rather than +// trusting a bare exit code - a policy that merely lets the probe start +// would otherwise look identical to a correctly minimized one. +// +// NOT YET RUN: this test has not been executed on a real Linux host in +// this session (macOS host has no Sandbox2/Linux toolchain). It has been +// reviewed against the Sandbox2 PolicyBuilder API as used by the +// PR-A-verified CSandboxForkserverSmokeTest_Linux, but needs a devbox or +// Buildkite pass before its gates (V4/V7/V16) can be marked verified - see +// elastic-workspace-3b59.4 session notes. + +#include + +#include + +#include +#include +#include +#include +#include +#include +#include + +#include "absl/time/time.h" +#include "sandboxed_api/sandbox2/executor.h" +#include "sandboxed_api/sandbox2/result.h" +#include "sandboxed_api/sandbox2/sandbox2.h" + +#ifndef ML_SANDBOX2_PROBE_PAYLOAD +#error "ML_SANDBOX2_PROBE_PAYLOAD must be defined by lib/sandbox/unittest/CMakeLists.txt" +#endif + +namespace { + +//! Returns the outcome recorded for mechanism, or empty if the mechanism +//! line never appeared - a missing line is itself a failure (the probe +//! didn't reach that check, e.g. because it was killed earlier). +std::string outcomeFor(const std::string& resultsFileContent, const std::string& mechanism) { + std::istringstream lines{resultsFileContent}; + std::string line; + const std::string marker{"mechanism=" + mechanism + " outcome="}; + while (std::getline(lines, line)) { + const std::size_t pos = line.find(marker); + if (pos == std::string::npos) { + continue; + } + const std::size_t start = pos + marker.size(); + const std::size_t end = line.find(' ', start); + return line.substr(start, end == std::string::npos ? std::string::npos : end - start); + } + return {}; +} + +std::string readFileOrEmpty(const std::string& path) { + std::ifstream file{path}; + if (file.is_open() == false) { + return {}; + } + std::ostringstream contents; + contents << file.rdbuf(); + return contents.str(); +} + +} // namespace + +BOOST_AUTO_TEST_SUITE(CPytorchInferenceSandboxPolicyMechanismTest_Linux) + +BOOST_AUTO_TEST_CASE(testMinimizedPolicyEnforcesEveryMechanism) { + char tmpDirTemplate[] = "/tmp/ml_sandbox_probe_test_XXXXXX"; + char* tmpDir = ::mkdtemp(tmpDirTemplate); + BOOST_TEST_REQUIRE(tmpDir != nullptr); + const std::string trustedTmpDir{tmpDir}; + + BOOST_TEST_REQUIRE(::mkdir((trustedTmpDir + "/ml-child-ipc").c_str(), 0700) == 0); + const std::string childRoot{trustedTmpDir + "/ml-child-ipc/mechanism-probe-child"}; + BOOST_TEST_REQUIRE(::mkdir(childRoot.c_str(), 0700) == 0); + + const std::vector args{"--input=" + childRoot + "/input.fifo", + "--output=" + childRoot + "/output.fifo", + "--logPipe=" + childRoot + "/log.fifo"}; + const ml::sandbox::SChildIpcValidationResult validated{ + ml::sandbox::validateChildIpcLaunchSpec(trustedTmpDir, args)}; + BOOST_TEST_REQUIRE(validated.s_Ok); + + const std::string payloadPath{ML_SANDBOX2_PROBE_PAYLOAD}; + const std::vector probeArgs{payloadPath, "/run/elastic/ml-ipc"}; + + auto executor = std::make_unique(payloadPath, probeArgs); + executor->limits()->set_rlimit_cpu(10).set_walltime_limit(absl::Seconds(10)); + + sandbox2::PolicyBuilder policyBuilder{ml::sandbox::buildPytorchInferenceFilesystemPolicy( + "/usr/bin", "/usr/lib", validated.s_Spec, /*tmpfsSizeBytes=*/16 * 1024 * 1024)}; + policyBuilder.AddLibrariesForBinary(payloadPath); + auto policy = policyBuilder.BuildOrDie(); + + sandbox2::Sandbox2 s2(std::move(executor), std::move(policy)); + sandbox2::Result result = s2.Run(); + + BOOST_TEST_REQUIRE(result.final_status() == sandbox2::Result::OK); + + // The child IPC directory is genuinely shared with the host, so the + // probe's results file - written from inside the sandbox to the mapped + // /run/elastic/ml-ipc path - is readable here at its host-visible + // childRoot path once the sandbox has exited. This IS the V4 "allowed + // IPC access" proof, not a separate assertion: if the mount/policy were + // wrong, this file would never appear. + const std::string resultsContent{readFileOrEmpty(childRoot + "/results.txt")}; + BOOST_TEST_REQUIRE(resultsContent.empty() == false); + BOOST_TEST_REQUIRE(resultsContent.find("reached=true") != std::string::npos); + + BOOST_REQUIRE_EQUAL(outcomeFor(resultsContent, "ipc_readwrite"), "allowed"); + BOOST_REQUIRE_EQUAL(outcomeFor(resultsContent, "host_read_etc_shadow"), "denied"); + BOOST_REQUIRE_EQUAL(outcomeFor(resultsContent, "private_tmpfs_write"), "allowed"); + BOOST_REQUIRE_EQUAL(outcomeFor(resultsContent, "external_egress"), "denied"); + BOOST_REQUIRE_EQUAL(outcomeFor(resultsContent, "loopback_reachable"), "ok"); + + ::unlink((childRoot + "/probe.txt").c_str()); + ::unlink((childRoot + "/results.txt").c_str()); + ::rmdir(childRoot.c_str()); + ::rmdir((trustedTmpDir + "/ml-child-ipc").c_str()); + ::rmdir(trustedTmpDir.c_str()); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc b/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc new file mode 100644 index 0000000000..93da9939fa --- /dev/null +++ b/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc @@ -0,0 +1,237 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ + +// Exercises validateChildIpcLaunchSpec against the pinned child-root +// contract (design.md Sandbox2 clean rebuild plan, PR C, gate V16). This +// suite is deliberately platform-portable - it only needs realpath()/mkdir() +// /symlink(), not Sandbox2 itself - so it runs on every ml-cpp CI platform, +// not just Linux. + +#include + +#include + +#include +#include +#include +#include +#include + +namespace { + +//! Creates trustedTmpDir/ml-child-ipc/ (mode 0700), mirroring the +//! native controller's pre-launch creation step, and returns the +//! *canonical* trusted base so literal test paths built from it never +//! diverge from realpath() output on hosts where /tmp is itself a symlink +//! (e.g. macOS's /tmp -> /private/tmp) - that divergence is a real +//! condition (E_MutableSymlinkOrAlias) this suite tests deliberately, so +//! setup must not trigger it by accident. +class CTempChildIpcFixture { +public: + explicit CTempChildIpcFixture(const std::string& childId) : m_ChildId(childId) { + char pathTemplate[] = "/tmp/ml_sandbox_policy_test_XXXXXX"; + char* created = ::mkdtemp(pathTemplate); + BOOST_TEST_REQUIRE(created != nullptr); + m_LiteralBase.assign(created); + + char resolved[PATH_MAX]; + BOOST_TEST_REQUIRE(::realpath(m_LiteralBase.c_str(), resolved) != nullptr); + m_CanonicalBase.assign(resolved); + + BOOST_TEST_REQUIRE(::mkdir((m_CanonicalBase + "/ml-child-ipc").c_str(), 0700) == 0); + m_ChildRoot = m_CanonicalBase + "/ml-child-ipc/" + m_ChildId; + BOOST_TEST_REQUIRE(::mkdir(m_ChildRoot.c_str(), 0700) == 0); + } + + ~CTempChildIpcFixture() { + ::rmdir(m_ChildRoot.c_str()); + ::rmdir((m_CanonicalBase + "/ml-child-ipc").c_str()); + if (m_LiteralBase != m_CanonicalBase) { + ::rmdir(m_LiteralBase.c_str()); + } + ::rmdir(m_CanonicalBase.c_str()); + } + + const std::string& canonicalTrustedBase() const { return m_CanonicalBase; } + const std::string& childRoot() const { return m_ChildRoot; } + +private: + std::string m_ChildId; + std::string m_LiteralBase; + std::string m_CanonicalBase; + std::string m_ChildRoot; +}; + +} // namespace + +BOOST_AUTO_TEST_SUITE(CPytorchInferenceSandboxPolicyTest) + +BOOST_AUTO_TEST_CASE(testAcceptsAllFourPathOptionsUnderPinnedChildRoot) { + CTempChildIpcFixture fixture{"child-1"}; + const std::vector args{ + "--input=" + fixture.childRoot() + "/input.fifo", + "--output=" + fixture.childRoot() + "/output.fifo", + "--restore=" + fixture.childRoot() + "/restore.fifo", + "--logPipe=" + fixture.childRoot() + "/log.fifo", + "--someScalarOption=not-a-path", + }; + + const ml::sandbox::SChildIpcValidationResult result{ + ml::sandbox::validateChildIpcLaunchSpec(fixture.canonicalTrustedBase(), args)}; + + BOOST_TEST_REQUIRE(result.s_Ok); + BOOST_TEST_REQUIRE(result.s_Rejected.empty()); + BOOST_REQUIRE_EQUAL(result.s_Spec.s_ChildId, "child-1"); + BOOST_REQUIRE_EQUAL(result.s_Spec.s_ChildIpcRoot, fixture.childRoot()); + BOOST_REQUIRE_EQUAL(result.s_Spec.s_PipePaths.size(), 4); +} + +BOOST_AUTO_TEST_CASE(testNoPathOptionsIsNotOk) { + const ml::sandbox::SChildIpcValidationResult result{ + ml::sandbox::validateChildIpcLaunchSpec("/tmp", {"--foo=bar"})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_TEST_REQUIRE(result.s_Spec.s_ChildId.empty()); +} + +BOOST_AUTO_TEST_CASE(testRejectsRelativePath) { + CTempChildIpcFixture fixture{"child-2"}; + const ml::sandbox::SChildIpcValidationResult result{ml::sandbox::validateChildIpcLaunchSpec( + fixture.canonicalTrustedBase(), {"--input=relative/input.fifo"})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == ml::sandbox::EChildIpcPathRejection::E_NotAbsolute); +} + +BOOST_AUTO_TEST_CASE(testRejectsRootLevelPath) { + CTempChildIpcFixture fixture{"child-3"}; + const ml::sandbox::SChildIpcValidationResult result{ + ml::sandbox::validateChildIpcLaunchSpec(fixture.canonicalTrustedBase(), {"--input=/input.fifo"})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == ml::sandbox::EChildIpcPathRejection::E_RootLevelPath); +} + +BOOST_AUTO_TEST_CASE(testRejectsDotDotEscape) { + CTempChildIpcFixture fixture{"child-4"}; + const std::string escapingPath{fixture.childRoot() + "/../../../etc/passwd"}; + const ml::sandbox::SChildIpcValidationResult result{ml::sandbox::validateChildIpcLaunchSpec( + fixture.canonicalTrustedBase(), {"--input=" + escapingPath})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == ml::sandbox::EChildIpcPathRejection::E_ContainsDotDot); +} + +BOOST_AUTO_TEST_CASE(testRejectsPathOutsideTrustedBase) { + CTempChildIpcFixture fixture{"child-5"}; + const ml::sandbox::SChildIpcValidationResult result{ml::sandbox::validateChildIpcLaunchSpec( + fixture.canonicalTrustedBase(), {"--input=/var/tmp/not-under-tmpdir/input.fifo"})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == + ml::sandbox::EChildIpcPathRejection::E_CanonicalizationFailed || + result.s_Rejected[0].s_Reason == + ml::sandbox::EChildIpcPathRejection::E_OutsideTrustedBase); +} + +BOOST_AUTO_TEST_CASE(testRejectsWrongDepthDirectChildOfTrustedBase) { + CTempChildIpcFixture fixture{"child-6"}; + // Direct child of $TMPDIR (missing the ml-child-ipc intermediate + // directory) must fail, not silently be accepted as "close enough" - + // this was the exact wording bug the adversarial review (M1) caught in + // an earlier draft of this contract. + const std::string tooShallow{fixture.canonicalTrustedBase() + "/input.fifo"}; + BOOST_TEST_REQUIRE(::mkdir((fixture.canonicalTrustedBase() + "/direct-child-dir").c_str(), 0700) == 0); + + const ml::sandbox::SChildIpcValidationResult result{ml::sandbox::validateChildIpcLaunchSpec( + fixture.canonicalTrustedBase(), {"--input=" + tooShallow})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == ml::sandbox::EChildIpcPathRejection::E_RootLevelPath || + result.s_Rejected[0].s_Reason == ml::sandbox::EChildIpcPathRejection::E_WrongDepth); + + ::rmdir((fixture.canonicalTrustedBase() + "/direct-child-dir").c_str()); +} + +BOOST_AUTO_TEST_CASE(testRejectsTooDeepNestingUnderChildId) { + CTempChildIpcFixture fixture{"child-7"}; + const std::string nestedDir{fixture.childRoot() + "/nested"}; + BOOST_TEST_REQUIRE(::mkdir(nestedDir.c_str(), 0700) == 0); + + const ml::sandbox::SChildIpcValidationResult result{ml::sandbox::validateChildIpcLaunchSpec( + fixture.canonicalTrustedBase(), {"--input=" + nestedDir + "/input.fifo"})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == ml::sandbox::EChildIpcPathRejection::E_WrongDepth); + + ::rmdir(nestedDir.c_str()); +} + +BOOST_AUTO_TEST_CASE(testRejectsDuplicateLiteralArgument) { + CTempChildIpcFixture fixture{"child-8"}; + const std::string arg{"--input=" + fixture.childRoot() + "/input.fifo"}; + + const ml::sandbox::SChildIpcValidationResult result{ + ml::sandbox::validateChildIpcLaunchSpec(fixture.canonicalTrustedBase(), {arg, arg})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == ml::sandbox::EChildIpcPathRejection::E_Duplicate); +} + +BOOST_AUTO_TEST_CASE(testRejectsMutableSymlinkAlias) { + CTempChildIpcFixture fixture{"child-9"}; + const std::string aliasPath{fixture.canonicalTrustedBase() + "/ml-child-ipc/child-9-alias"}; + BOOST_TEST_REQUIRE(::symlink(fixture.childRoot().c_str(), aliasPath.c_str()) == 0); + + const ml::sandbox::SChildIpcValidationResult result{ml::sandbox::validateChildIpcLaunchSpec( + fixture.canonicalTrustedBase(), {"--input=" + aliasPath + "/input.fifo"})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == + ml::sandbox::EChildIpcPathRejection::E_MutableSymlinkOrAlias); + + ::unlink(aliasPath.c_str()); +} + +BOOST_AUTO_TEST_CASE(testRejectsChildIdMismatchAcrossOptions) { + CTempChildIpcFixture fixtureA{"child-10a"}; + CTempChildIpcFixture fixtureB{"child-10b"}; + + const ml::sandbox::SChildIpcValidationResult result{ml::sandbox::validateChildIpcLaunchSpec( + fixtureA.canonicalTrustedBase(), + {"--input=" + fixtureA.childRoot() + "/input.fifo", + "--output=" + fixtureB.childRoot() + "/output.fifo"})}; + + BOOST_TEST_REQUIRE(result.s_Ok == false); + BOOST_REQUIRE_EQUAL(result.s_Rejected.size(), 1); + BOOST_REQUIRE(result.s_Rejected[0].s_Reason == ml::sandbox::EChildIpcPathRejection::E_ChildIdMismatch); +} + +BOOST_AUTO_TEST_CASE(testIgnoresScalarOptionsAsCandidatePaths) { + CTempChildIpcFixture fixture{"child-11"}; + const ml::sandbox::SChildIpcValidationResult result{ml::sandbox::validateChildIpcLaunchSpec( + fixture.canonicalTrustedBase(), + {"--input=" + fixture.childRoot() + "/input.fifo", "--modelId=../../../etc/passwd", + "--inputIsPipe"})}; + + BOOST_TEST_REQUIRE(result.s_Ok); + BOOST_TEST_REQUIRE(result.s_Rejected.empty()); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/lib/sandbox/unittest/payloads/ml_sandbox_probe.cc b/lib/sandbox/unittest/payloads/ml_sandbox_probe.cc new file mode 100644 index 0000000000..d6f52762e7 --- /dev/null +++ b/lib/sandbox/unittest/payloads/ml_sandbox_probe.cc @@ -0,0 +1,177 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ + +// Purpose-built allowlisted payload for PR C's mechanism probe (design.md +// Sandbox2 clean rebuild plan, gates V4/V7/V16). Runs *inside* the sandbox +// under the policy built by buildPytorchInferenceFilesystemPolicy and prints +// one "mechanism=... outcome=..." line per check to stdout, which the +// controller-side test (CPytorchInferenceSandboxPolicyMechanismTest_Linux) +// asserts on directly - a wrong-but-still-startable policy would otherwise +// look identical to a correct one if the test only checked the exit code. +// Deliberately dependency-free, like sandbox_smoke_payload.cc: no ml-cpp +// library dependencies, no policy of its own. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +//! File descriptor for the results file this probe writes into the mapped +//! per-child IPC directory. The host-side test reads that file directly +//! from the *host* path after the sandbox exits - the IPC directory is +//! genuinely shared, so this doubles as the V4 "allowed IPC access" proof +//! and as this probe's only result channel (no stdout capture plumbing +//! exists yet; that lands with CSandboxedProcessSpawner in PR D). +int g_ResultsFd = -1; + +void report(const char* mechanism, const char* outcome, const std::string& detail = "") { + std::printf("ml_sandbox_probe: mechanism=%s outcome=%s detail=%s\n", mechanism, outcome, + detail.c_str()); + std::fflush(stdout); + if (g_ResultsFd >= 0) { + std::string line{std::string("mechanism=") + mechanism + " outcome=" + outcome + " detail=" + + detail + "\n"}; + ::write(g_ResultsFd, line.c_str(), line.size()); + } +} + +} // namespace + +int main(int argc, char** argv) { + if (argc < 2) { + std::fprintf(stderr, "usage: ml_sandbox_probe \n"); + return EXIT_FAILURE; + } + const std::string ipcDir{argv[1]}; + + g_ResultsFd = ::open((ipcDir + "/results.txt").c_str(), O_CREAT | O_WRONLY | O_TRUNC, 0600); + + std::printf("ml_sandbox_probe: reached\n"); + std::fflush(stdout); + if (g_ResultsFd >= 0) { + const std::string reachedLine{"reached=true\n"}; + ::write(g_ResultsFd, reachedLine.c_str(), reachedLine.size()); + } + + // Allowed IPC access (V4 positive control): write then read back a file + // inside the mapped per-child IPC directory. + const std::string ipcFile{ipcDir + "/probe.txt"}; + int writeFd = ::open(ipcFile.c_str(), O_CREAT | O_WRONLY, 0600); + if (writeFd >= 0) { + ::write(writeFd, "probe", 5); + ::close(writeFd); + int readFd = ::open(ipcFile.c_str(), O_RDONLY); + char buf[8]{}; + const bool readBack = readFd >= 0 && ::read(readFd, buf, sizeof(buf)) == 5 && + std::strncmp(buf, "probe", 5) == 0; + if (readFd >= 0) { + ::close(readFd); + } + report("ipc_readwrite", readBack ? "allowed" : "denied"); + } else { + report("ipc_readwrite", "denied", std::strerror(errno)); + } + + // Denied host read (V4 negative control): /etc/shadow must not be + // readable even though narrow, individually justified /etc files are + // allowlisted (allowlistedEtcFiles()). + int shadowFd = ::open("/etc/shadow", O_RDONLY); + if (shadowFd < 0) { + report("host_read_etc_shadow", "denied", std::strerror(errno)); + } else { + ::close(shadowFd); + report("host_read_etc_shadow", "allowed"); + } + + // Private tmpfs (V4): the mapped /tmp must be writable but is a private + // tmpfs, never the host's shared /tmp - this re-proves it is not shared + // by checking it is actually writable from inside the sandbox. + const std::string privateTmpFile{"/tmp/ml_sandbox_probe_private_tmp_test"}; + int tmpFd = ::open(privateTmpFile.c_str(), O_CREAT | O_WRONLY, 0600); + if (tmpFd >= 0) { + ::close(tmpFd); + ::unlink(privateTmpFile.c_str()); + report("private_tmpfs_write", "allowed"); + } else { + report("private_tmpfs_write", "denied", std::strerror(errno)); + } + + // Mount enumeration (V16 conformance): /etc must list only the + // allowlisted files, never a full directory bind. + DIR* etcDir = ::opendir("/etc"); + if (etcDir != nullptr) { + int entryCount = 0; + while (::readdir(etcDir) != nullptr) { + ++entryCount; + } + ::closedir(etcDir); + report("etc_enumeration", "counted", std::to_string(entryCount)); + } else { + report("etc_enumeration", "denied", std::strerror(errno)); + } + + // Private PID namespace: this process should be (close to) the + // sandbox's own init, not a real-looking host PID. + report("pid_namespace", (::getpid() <= 2) ? "namespaced" : "not_namespaced", + std::to_string(::getpid())); + + // External egress denial (V7 negative control): an outbound connect to + // a guaranteed non-routable test address (TEST-NET-1, RFC 5737) must + // fail - Sandbox2's network namespace has no route out. Using a + // non-routable address instead of a real host keeps this check + // hermetic and independent of network availability in CI. + int egressSocket = ::socket(AF_INET, SOCK_STREAM, 0); + if (egressSocket >= 0) { + sockaddr_in addr{}; + addr.sin_family = AF_INET; + addr.sin_port = htons(80); + ::inet_pton(AF_INET, "192.0.2.1", &addr.sin_addr); + const int rc = ::connect(egressSocket, reinterpret_cast(&addr), sizeof(addr)); + report("external_egress", rc == 0 ? "allowed" : "denied", std::strerror(errno)); + ::close(egressSocket); + } else { + report("external_egress", "denied", std::strerror(errno)); + } + + // Local operation success (V7 positive control): loopback must remain + // reachable at the network-namespace level. Connection-refused (nobody + // listening on this port) still counts as "reachable" - only a + // namespace-level error (e.g. ENETUNREACH) means loopback itself broke. + int loopbackSocket = ::socket(AF_INET, SOCK_STREAM, 0); + if (loopbackSocket >= 0) { + sockaddr_in addr{}; + addr.sin_family = AF_INET; + addr.sin_port = htons(1); + addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + const int rc = ::connect(loopbackSocket, reinterpret_cast(&addr), sizeof(addr)); + const bool loopbackReachable = rc == 0 || errno == ECONNREFUSED; + report("loopback_reachable", loopbackReachable ? "ok" : "broken", std::strerror(errno)); + ::close(loopbackSocket); + } else { + report("loopback_reachable", "broken", std::strerror(errno)); + } + + std::printf("ml_sandbox_probe: done\n"); + std::fflush(stdout); + if (g_ResultsFd >= 0) { + ::close(g_ResultsFd); + } + return EXIT_SUCCESS; +} From 51ddf27c4135c87f66dc7407bbea1af37e5fe704 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:43:49 +0200 Subject: [PATCH 03/15] [ML] Explicit lifecycle state enum + CAS timeout latch for sandboxed process spawner (header only) Adds include/sandbox/CSandboxedProcessSpawner.h declaring the explicit child lifecycle state enum (Prepared/Launched/IdentityCaptured/ Registered/Monitoring/TerminationRequested/CleanupRequired/Reaped/ Failed, design.md's mermaid diagram), a registry-entry shape (SSandboxedChild) carrying that state, a generation counter, and pidfd/Sandbox2 handle fields, and a one-shot CAS-controlled outcome latch (CCasOutcomeLatch: Pending -> TimedOut|Completed via a single compare_exchange_strong) replacing the two-boolean timeout/completion coordination in the frozen enhancement/sandbox2 reference. Declares the public API (spawn/terminateChild/hasChild) with no injectable seams yet - those are PR D Task 2's contract. No .cc file and no CMakeLists change: lib/sandbox/CMakeLists.txt's SRCS only lists .cc files, so a header-only commit needs none. Header-only; verified with a standalone g++ -fsyntax-only -std=c++17 -Wall -Wextra probe against this worktree's include/ (no Sandbox2 toolchain on this host - see docs/projects/mlcpp-sandbox2-pr2873/ pr-d-lifecycle.plan.md Task 1). --- include/sandbox/CSandboxedProcessSpawner.h | 178 +++++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 include/sandbox/CSandboxedProcessSpawner.h diff --git a/include/sandbox/CSandboxedProcessSpawner.h b/include/sandbox/CSandboxedProcessSpawner.h new file mode 100644 index 0000000000..2a91fd1231 --- /dev/null +++ b/include/sandbox/CSandboxedProcessSpawner.h @@ -0,0 +1,178 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ +#ifndef INCLUDED_ml_sandbox_CSandboxedProcessSpawner_h +#define INCLUDED_ml_sandbox_CSandboxedProcessSpawner_h + +#include + +#include +#include +#include +#include +#include +#include +#include + +// Sandbox2 headers are unavailable on non-Linux configure runs (see +// include/sandbox/CPytorchInferenceSandboxPolicy.h). Only a forward +// declaration is needed here: this header stores sandbox2::Sandbox2 solely +// behind a shared_ptr, never by value, so non-Linux builds never need the +// real type. +namespace sandbox2 { +class Sandbox2; +} + +namespace ml { +namespace sandbox { + +//! \brief +//! Spawn and own the lifecycle of processes inside a Sandbox2 isolation +//! boundary. +//! +//! DESCRIPTION:\n +//! Replaces numeric-PID process control (core::CDetachedProcessSpawner's +//! model) with identity-bound handles, because a sandboxed child's PID can +//! be reused by an unrelated process while a stale monitor or a delayed +//! terminateChild() call is still in flight (design.md §"Spawn lifecycle +//! and ownership", LI7). The lifecycle below is the explicit state machine +//! every live registry entry moves through; see design.md's mermaid +//! diagram for the full transition set. This header declares the state +//! shape and public API only - spawn()'s kill-and-reap guard, injectable +//! seams, and pidfd outcome classification land in later tasks of +//! docs/projects/mlcpp-sandbox2-pr2873/pr-d-lifecycle.plan.md. +class CSandboxedProcessSpawner { +public: + using TStrVec = std::vector; + + //! Explicit lifecycle states a registry entry moves through, mirroring + //! design.md's mermaid diagram one-for-one. No state is skipped and no + //! state is inferred from a combination of booleans. + enum class EChildLifecycleState { + E_Prepared, //!< Launch spec validated; process not yet started. + E_Launched, //!< Sandbox2::RunAsync() succeeded; pid() not yet captured. + E_IdentityCaptured, //!< pid() captured; kill-and-reap guard armed (LI1). + E_Registered, //!< Registry insertion succeeded. + E_Monitoring, //!< Monitor thread handoff succeeded; guard disarmed (LI2). + E_TerminationRequested, //!< terminateChild() issued a request; child not yet confirmed exited. + E_CleanupRequired, //!< Sandbox2 completion observed; registry entry pending removal. + E_Reaped, //!< AwaitResult() returned; every descriptor closed exactly once (LI4). + E_Failed //!< spawn() failed at or after this state; no live unowned child remains. + }; + + //! One-shot outcome of the timeout-vs-completion race (design.md + //! MG4/V11), replacing independent-boolean coordination with a single + //! atomic latch. Exactly one of TimedOut/Completed wins via + //! compare_exchange_strong from Pending; the loser observes the + //! winner's value and must not perform cleanup. + enum class EOutcomeState { E_Pending, E_TimedOut, E_Completed }; + + //! \brief One-shot CAS latch: Pending -> TimedOut|Completed, never back. + //! + //! DESCRIPTION:\n + //! The only coordination mechanism between a timeout path and a + //! Sandbox2-completion path racing to decide who performs cleanup for + //! the same child. A single compare_exchange_strong call decides the + //! winner; the loser's compare_exchange_strong fails and returns the + //! value the winner set, so it can branch without a second flag. + class CCasOutcomeLatch { + public: + CCasOutcomeLatch() = default; + + CCasOutcomeLatch(const CCasOutcomeLatch&) = delete; + CCasOutcomeLatch& operator=(const CCasOutcomeLatch&) = delete; + + //! Attempt to move the latch from Pending to \p desired. Returns + //! true iff this call won the race (the latch was Pending and is + //! now \p desired); false means some call - possibly this one on a + //! retry, possibly a racing call - already set it to another value, + //! which is written back into \p desired for the caller to inspect. + bool tryResolve(EOutcomeState& desired) { + EOutcomeState expected{EOutcomeState::E_Pending}; + return m_State.compare_exchange_strong(expected, desired) ? true + : (desired = expected, false); + } + + //! \return the latch's current value. For diagnostics only - never + //! branch cleanup logic on a load() result instead of tryResolve()'s + //! own return value, or the check-then-act gap reintroduces the + //! two-boolean race this latch replaces. + EOutcomeState load() const { return m_State.load(); } + + private: + std::atomic m_State{EOutcomeState::E_Pending}; + }; + +public: + CSandboxedProcessSpawner(); + ~CSandboxedProcessSpawner(); + + //! Spawn a sandboxed process. Returns true only after registry + //! insertion and monitor handoff both succeed (LI2); on any other + //! outcome returns false with childPid left at 0 and no live unowned + //! child, no registry entry, and no leaked descriptor (LI3). + bool spawn(const std::string& processPath, const TStrVec& args, core::CProcess::TPid& childPid); + + //! Request termination of a sandboxed child previously started by this + //! object, targeting its identity-bound handle rather than a recycled + //! numeric PID (LI7). + bool terminateChild(core::CProcess::TPid pid); + + //! \return true if this object owns a sandboxed child with the given + //! PID that is still live (not yet Reaped or Failed). + bool hasChild(core::CProcess::TPid pid) const; + +private: + //! \brief A live sandboxed child and the handles needed to manage it + //! safely through every lifecycle state. + //! + //! DESCRIPTION:\n + //! Shape only in this task - no lifecycle logic lands here yet. Carries + //! the explicit state, a monotonic generation (so a stale monitor + //! cannot erase or mutate a newer registration racing the same PID, + //! LI6), the Sandbox2 handle (co-owned with any monitor thread via + //! shared_ptr, since a monitor can outlive this spawner and must never + //! hold a raw pointer back into it, LI5), the pidfd used for + //! identity-bound termination when the kernel provides one, and the + //! one-shot outcome latch used to resolve a timeout-vs-completion race + //! for this specific child (MG4/V11). + struct SSandboxedChild { + EChildLifecycleState s_State{EChildLifecycleState::E_Prepared}; + std::uint64_t s_Generation{0}; + std::shared_ptr s_Sandbox; + int s_PidFd{-1}; + std::shared_ptr s_Outcome; + }; + + //! \brief The live sandboxed children, and the lock that guards them. + //! + //! DESCRIPTION:\n + //! Held behind a shared_ptr because a monitor thread that removes a + //! child outlives the spawn() call that started it, and can outlive + //! this object: the controller may tear the spawner down while a + //! sandboxed pytorch_inference is still running (LI9), and the monitor + //! only learns that the sandboxee exited some time later. A raw pointer + //! back to the spawner would be dangling by then, so the monitor + //! co-owns the registry instead, and the spawner's destructor needs no + //! synchronisation with in-flight monitors. + struct SPidRegistry { + mutable std::mutex s_Mutex; + std::uint64_t s_NextGeneration{0}; + std::map s_Children; + }; + using TPidRegistryPtr = std::shared_ptr; + + const TPidRegistryPtr m_PidRegistry{std::make_shared()}; +}; + +} // namespace sandbox +} // namespace ml + +#endif // INCLUDED_ml_sandbox_CSandboxedProcessSpawner_h From 6ff755d12e8e66f7bf165c99dc1db108937b1187 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:50:39 +0200 Subject: [PATCH 04/15] [ML] Fix doc-comment provenance for sandboxed process lifecycle enum E_IdentityCaptured is not present in design.md's mermaid diagram (only 8 states); it comes from the Sandbox2 rebuild plan's explicit Prepared->Launched->IdentityCaptured->Registered->Monitoring->Reaped sequence. Corrected the class-level and enum-level doc comments to cite the rebuild plan as the source for that state and stop claiming one-for-one fidelity to design.md's diagram, which covers a related but not identical lifecycle. --- include/sandbox/CSandboxedProcessSpawner.h | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/include/sandbox/CSandboxedProcessSpawner.h b/include/sandbox/CSandboxedProcessSpawner.h index 2a91fd1231..872005f943 100644 --- a/include/sandbox/CSandboxedProcessSpawner.h +++ b/include/sandbox/CSandboxedProcessSpawner.h @@ -43,17 +43,24 @@ namespace sandbox { //! be reused by an unrelated process while a stale monitor or a delayed //! terminateChild() call is still in flight (design.md §"Spawn lifecycle //! and ownership", LI7). The lifecycle below is the explicit state machine -//! every live registry entry moves through; see design.md's mermaid -//! diagram for the full transition set. This header declares the state -//! shape and public API only - spawn()'s kill-and-reap guard, injectable -//! seams, and pidfd outcome classification land in later tasks of +//! every live registry entry moves through; see the Sandbox2 rebuild plan's +//! (docs/projects/mlcpp-sandbox2-pr2873/sandbox2_clean_rebuild_3df1182e.plan.md) +//! Prepared->Launched->IdentityCaptured->Registered->Monitoring->Reaped +//! sequence for the full transition set, and design.md's mermaid diagram for +//! the related-but-not-identical high-level lifecycle. This header declares +//! the state shape and public API only - spawn()'s kill-and-reap guard, +//! injectable seams, and pidfd outcome classification land in later tasks of //! docs/projects/mlcpp-sandbox2-pr2873/pr-d-lifecycle.plan.md. class CSandboxedProcessSpawner { public: using TStrVec = std::vector; //! Explicit lifecycle states a registry entry moves through, mirroring - //! design.md's mermaid diagram one-for-one. No state is skipped and no + //! the state machine described in the Sandbox2 rebuild plan (PR D scope) + //! -- design.md's mermaid diagram covers the same overall lifecycle but + //! does not include E_IdentityCaptured, which is added here per the + //! rebuild plan's explicit Prepared->Launched->IdentityCaptured-> + //! Registered->Monitoring->Reaped sequence. No state is skipped and no //! state is inferred from a combination of booleans. enum class EChildLifecycleState { E_Prepared, //!< Launch spec validated; process not yet started. From 6b4d85018262285283897c1f28e78074e186e384 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:08:42 +0200 Subject: [PATCH 05/15] [ML] Kill-and-reap guard + injectable seams for spawn() skeleton Implements Task 2 of the PR D Sandbox2 lifecycle rebuild: a new CSandboxedProcessSpawner_Linux.cc gives spawn() its full skeleton, calling PR C's validateChildIpcLaunchSpec()/buildPytorchInferenceFilesystemPolicy() as an interlock before TryBuild(), and arming a non-throwing CKillAndReapGuard immediately after RunAsync()/pid() capture (E_IdentityCaptured) so every early-return before registry insertion and monitor handoff both succeed (E_Monitoring, LI2) routes through one cleanup owner instead of a duplicate catch block (LI1/LI3, closes MG3). Adds four injectable seams (pidfd acquisition, registry allocation, monitor-thread creation/detach, Sandbox2 completion) with production defaults and a test-only constructor overload; SSandboxedChild/SPidRegistry move from private to public so the seam signatures are nameable by test code. The new .cc is added to lib/sandbox/CMakeLists.txt's SRCS unconditionally, matching lib/core/CMakeLists.txt's CDetachedProcessSpawner.cc pattern, with #ifdef SANDBOX2_AVAILABLE gating the real logic inside. Does not implement pidfd-error classification or terminateChild()'s real logic - Task 3 scope. No numeric kill(pid) fallback anywhere in this file. --- include/sandbox/CSandboxedProcessSpawner.h | 144 ++++- lib/sandbox/CMakeLists.txt | 1 + lib/sandbox/CSandboxedProcessSpawner_Linux.cc | 516 ++++++++++++++++++ 3 files changed, 633 insertions(+), 28 deletions(-) create mode 100644 lib/sandbox/CSandboxedProcessSpawner_Linux.cc diff --git a/include/sandbox/CSandboxedProcessSpawner.h b/include/sandbox/CSandboxedProcessSpawner.h index 872005f943..237f4d9fe5 100644 --- a/include/sandbox/CSandboxedProcessSpawner.h +++ b/include/sandbox/CSandboxedProcessSpawner.h @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -30,6 +31,16 @@ namespace sandbox2 { class Sandbox2; } +#ifdef SANDBOX2_AVAILABLE +// The Sandbox2-completion injectable seam (TAwaitResultFn, below) names +// sandbox2::Result in a std::function signature, which needs the complete +// type - the forward declaration above is not enough for that one seam. +// Non-Linux/no-Sandbox2 configures never see this include, matching +// include/sandbox/CPytorchInferenceSandboxPolicy.h's pattern for the same +// reason. +#include +#endif + namespace ml { namespace sandbox { @@ -117,39 +128,34 @@ class CSandboxedProcessSpawner { std::atomic m_State{EOutcomeState::E_Pending}; }; -public: - CSandboxedProcessSpawner(); - ~CSandboxedProcessSpawner(); - - //! Spawn a sandboxed process. Returns true only after registry - //! insertion and monitor handoff both succeed (LI2); on any other - //! outcome returns false with childPid left at 0 and no live unowned - //! child, no registry entry, and no leaked descriptor (LI3). - bool spawn(const std::string& processPath, const TStrVec& args, core::CProcess::TPid& childPid); - - //! Request termination of a sandboxed child previously started by this - //! object, targeting its identity-bound handle rather than a recycled - //! numeric PID (LI7). - bool terminateChild(core::CProcess::TPid pid); - - //! \return true if this object owns a sandboxed child with the given - //! PID that is still live (not yet Reaped or Failed). - bool hasChild(core::CProcess::TPid pid) const; + //! Placeholder outcome of the injectable pidfd-acquisition seam (Task 2 + //! scope only). A simple success/failure signal - Task 3 replaces this + //! with full ENOSYS/EMFILE/... classification (design.md gate V9) and + //! decides what a classified failure does; nothing here selects a + //! numeric-kill(pid) fallback, and nothing should until Task 3 lands. + struct SPidFdAcquisitionResult { + int s_Fd{-1}; + int s_Errno{0}; + }; -private: +public: //! \brief A live sandboxed child and the handles needed to manage it //! safely through every lifecycle state. //! //! DESCRIPTION:\n - //! Shape only in this task - no lifecycle logic lands here yet. Carries - //! the explicit state, a monotonic generation (so a stale monitor - //! cannot erase or mutate a newer registration racing the same PID, - //! LI6), the Sandbox2 handle (co-owned with any monitor thread via - //! shared_ptr, since a monitor can outlive this spawner and must never - //! hold a raw pointer back into it, LI5), the pidfd used for - //! identity-bound termination when the kernel provides one, and the - //! one-shot outcome latch used to resolve a timeout-vs-completion race - //! for this specific child (MG4/V11). + //! Shape only in Task 1 - no lifecycle logic landed there. Carries the + //! explicit state, a monotonic generation (so a stale monitor cannot + //! erase or mutate a newer registration racing the same PID, LI6), the + //! Sandbox2 handle (co-owned with any monitor thread via shared_ptr, + //! since a monitor can outlive this spawner and must never hold a raw + //! pointer back into it, LI5), the pidfd used for identity-bound + //! termination when the kernel provides one, and the one-shot outcome + //! latch used to resolve a timeout-vs-completion race for this specific + //! child (MG4/V11). Public (rather than Task 1's private placement) as + //! of Task 2: the registry-allocation seam (TRegistryInsertFn, below) + //! and its test-only overrides need to name this type, and a private + //! nested type cannot appear in a public alias's signature in a way + //! external test code could actually spell. struct SSandboxedChild { EChildLifecycleState s_State{EChildLifecycleState::E_Prepared}; std::uint64_t s_Generation{0}; @@ -176,7 +182,89 @@ class CSandboxedProcessSpawner { }; using TPidRegistryPtr = std::shared_ptr; + //! Injectable seams (design.md's PR D plan, Task 2). Each has a + //! production default, selected by passing an empty std::function to + //! the test-only constructor below (or by using the plain default + //! constructor, which never touches these types at all). + + //! pidfd-acquisition seam: wraps the pidfd_open syscall. See + //! SPidFdAcquisitionResult's comment - Task 3 replaces the placeholder + //! success/failure shape with full classification. + using TPidFdOpenFn = std::function; + + //! Registry-allocation seam: performs the locked map insertion + //! (replacing any stale entry for the same PID, mirroring the + //! production default) and returns the new entry's generation. The + //! production default never throws for ordinary insertion; a test + //! overriding this seam can throw std::bad_alloc, or return a + //! deliberately colliding generation, to exercise LI8 deterministically + //! without waiting on real resource exhaustion. + using TRegistryInsertFn = + std::function; + + //! Monitor-thread creation/detach seam. Returns false - never throws - + //! if std::thread construction or detach() failed, so a test can force + //! that failure deterministically (LI8) without depending on the OS + //! actually running out of threads. The production default constructs + //! std::thread(monitorBody) and detaches it, converting any + //! std::system_error from either step into a false return. + using TMonitorLaunchFn = std::function monitorBody)>; + +#ifdef SANDBOX2_AVAILABLE + //! Sandbox2-completion seam: wraps calling AwaitResult() on the live + //! sandbox handle, so a test controls exactly when/what result is + //! reported (design.md V11's deterministic timeout-vs-completion test, + //! Task 4 scope). Available only where sandbox2::Result is a complete + //! type; see the SANDBOX2_AVAILABLE include block above this class. + using TAwaitResultFn = std::function; +#endif + + CSandboxedProcessSpawner(); + + //! Test-only constructor injecting the four seams above. Each parameter + //! defaults to an empty std::function; spawn() + //! (CSandboxedProcessSpawner_Linux.cc) treats an empty seam as "use the + //! production behaviour", so production callers should keep using the + //! plain default constructor and never need to name these types. + CSandboxedProcessSpawner(TPidFdOpenFn pidFdOpenFn, + TRegistryInsertFn registryInsertFn, + TMonitorLaunchFn monitorLaunchFn +#ifdef SANDBOX2_AVAILABLE + , + TAwaitResultFn awaitResultFn +#endif + ); + + ~CSandboxedProcessSpawner(); + + //! Spawn a sandboxed process. Returns true only after registry + //! insertion and monitor handoff both succeed (LI2); on any other + //! outcome returns false with childPid left at 0 and no live unowned + //! child, no registry entry, and no leaked descriptor (LI3). + bool spawn(const std::string& processPath, const TStrVec& args, core::CProcess::TPid& childPid); + + //! Request termination of a sandboxed child previously started by this + //! object, targeting its identity-bound handle rather than a recycled + //! numeric PID (LI7). + bool terminateChild(core::CProcess::TPid pid); + + //! \return true if this object owns a sandboxed child with the given + //! PID that is still live (not yet Reaped or Failed). + bool hasChild(core::CProcess::TPid pid) const; + +private: const TPidRegistryPtr m_PidRegistry{std::make_shared()}; + + //! Seam storage for the test-only constructor. Left empty (default + //! std::function) by the plain default constructor, which + //! CSandboxedProcessSpawner_Linux.cc reads as "use the production + //! behaviour" for every seam. + TPidFdOpenFn m_PidFdOpenFn; + TRegistryInsertFn m_RegistryInsertFn; + TMonitorLaunchFn m_MonitorLaunchFn; +#ifdef SANDBOX2_AVAILABLE + TAwaitResultFn m_AwaitResultFn; +#endif }; } // namespace sandbox diff --git a/lib/sandbox/CMakeLists.txt b/lib/sandbox/CMakeLists.txt index 1c5205843d..ffcba2f2f7 100644 --- a/lib/sandbox/CMakeLists.txt +++ b/lib/sandbox/CMakeLists.txt @@ -26,6 +26,7 @@ set(ML_LINK_LIBRARIES set(SRCS CMlSandboxAvailability.cc CPytorchInferenceSandboxPolicy.cc + CSandboxedProcessSpawner_Linux.cc ) ml_add_library(MlSandbox STATIC ${SRCS}) diff --git a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc new file mode 100644 index 0000000000..0491a49f87 --- /dev/null +++ b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc @@ -0,0 +1,516 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ +#include + +#include +#include + +#include +#include +#include +#include + +// This translation unit is compiled unconditionally (see lib/sandbox/CMakeLists.txt +// - it is added to SRCS the same way lib/core/CMakeLists.txt unconditionally +// builds CDetachedProcessSpawner.cc), so every symbol outside the +// SANDBOX2_AVAILABLE-gated block below must compile with no Sandbox2/Linux +// headers available at all. The real spawn() logic - and everything that +// needs sandbox2:: types or Linux-only syscalls - lives inside that block; +// non-Linux/no-Sandbox2 configures fall through to the "not built with +// Sandbox2 support" stub path at the bottom of spawn(), matching +// CPytorchInferenceSandboxPolicy.cc's split. +#ifdef SANDBOX2_AVAILABLE + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include + +// environ is a global variable from the C runtime library. +extern char** environ; + +// The CentOS 7 based CI build image has kernel headers that predate pidfd, so +// __NR_pidfd_open may be undefined at build time even though the runtime +// kernel supports it. pidfd_open is syscall number 434 on every architecture +// ml-cpp builds for (x86_64 and aarch64); fall back to that literal so the +// spawner does not depend on the build image's header version. Task 3 owns +// classifying what a failed acquisition means (ENOSYS vs. a resource error); +// this task only needs the raw syscall wrapped behind the injectable seam. +#ifdef __NR_pidfd_open +#define ML_NR_pidfd_open __NR_pidfd_open +#else +#define ML_NR_pidfd_open 434 +#endif + +#endif // SANDBOX2_AVAILABLE + +namespace ml { +namespace sandbox { + +#ifdef SANDBOX2_AVAILABLE + +namespace { + +//! RAII owner for a pidfd between acquisition and the registry insertion +//! that takes over its lifetime. Closes the descriptor on destruction unless +//! release() has handed ownership to the registry entry, so an exception +//! (e.g. std::bad_alloc from the map node allocation, injected via the +//! registry-allocation seam) thrown before registration cannot leak the fd. +class CScopedPidFd { +public: + explicit CScopedPidFd(int pidFd) : m_PidFd{pidFd} {} + ~CScopedPidFd() { + if (m_PidFd >= 0) { + ::close(m_PidFd); + } + } + CScopedPidFd(const CScopedPidFd&) = delete; + CScopedPidFd& operator=(const CScopedPidFd&) = delete; + int get() const { return m_PidFd; } + //! Relinquish ownership: the caller (the registry entry) is now + //! responsible for closing the descriptor. + void release() { m_PidFd = -1; } + +private: + int m_PidFd; +}; + +//! Close a pidfd that a registry entry owns, tolerating an already-released +//! (-1) value. +void closePidFdIfOpen(int pidFd) { + if (pidFd >= 0) { + ::close(pidFd); + } +} + +//! Non-throwing kill-and-reap guard (design.md LI1, closing MG3). Armed +//! immediately when RunAsync() succeeds and pid() is captured +//! (E_IdentityCaptured) - before any potentially-throwing operation +//! (registry insertion, monitor-thread construction, detach()) - and +//! disarmed only after registry insertion AND monitor handoff both succeed +//! (E_Monitoring, LI2). Every early return on the path between those two +//! points goes through this guard's destructor rather than a hand-written +//! duplicate cleanup block (design.md LI3), so there is exactly one cleanup +//! owner for "launched but not yet fully handed off". +//! +//! The destructor must not throw: it runs during stack unwinding on the +//! failure paths this guard exists to cover, and a second exception there +//! would call std::terminate. Kill() and the AwaitResult seam are wrapped in +//! a catch-all for that reason; this task does not classify what Kill() +//! itself can fail with (Task 3 scope), only ensures a throw from it cannot +//! escape a destructor. +class CKillAndReapGuard { +public: + CKillAndReapGuard(sandbox2::Sandbox2* sandbox, + CSandboxedProcessSpawner::TAwaitResultFn awaitResultFn) + : m_Sandbox{sandbox}, m_AwaitResultFn{std::move(awaitResultFn)} {} + + ~CKillAndReapGuard() { + if (m_Armed && m_Sandbox != nullptr) { + try { + m_Sandbox->Kill(); + if (m_AwaitResultFn) { + m_AwaitResultFn(*m_Sandbox); + } else { + m_Sandbox->AwaitResult(); + } + } catch (...) { + // Never let an exception escape a destructor; this guard's + // whole purpose is bounded, best-effort cleanup on a + // failure path that is already unwinding. + } + } + } + + CKillAndReapGuard(const CKillAndReapGuard&) = delete; + CKillAndReapGuard& operator=(const CKillAndReapGuard&) = delete; + + //! Called once registry insertion AND monitor handoff have both + //! succeeded (E_Monitoring, LI2). After this, the monitor thread owns + //! calling the (possibly injected) AwaitResult seam exactly once. + void disarm() { m_Armed = false; } + +private: + sandbox2::Sandbox2* m_Sandbox; + CSandboxedProcessSpawner::TAwaitResultFn m_AwaitResultFn; + bool m_Armed{true}; +}; + +//! The sandboxee's environment: the caller's, with ML_SANDBOXED=1 set +//! exactly once so pytorch_inference skips its in-process seccomp filter and +//! relies on the Sandbox2 policy instead. +std::vector buildSandboxeeEnvironment() { + std::vector sandboxeeEnv; + bool markerSet{false}; + for (char** env = ::environ; *env != nullptr; ++env) { + std::string envVar{*env}; + if (envVar.find("ML_SANDBOXED=") == 0) { + sandboxeeEnv.push_back("ML_SANDBOXED=1"); + markerSet = true; + } else { + sandboxeeEnv.push_back(std::move(envVar)); + } + } + if (markerSet == false) { + sandboxeeEnv.push_back("ML_SANDBOXED=1"); + } + return sandboxeeEnv; +} + +//! An executor configured for a long-lived daemon sandboxee, matching the +//! frozen pre-rebuild reference's timeout/rlimit relaxations (a run-to- +//! completion default would kill a healthy, long-lived pytorch_inference). +std::unique_ptr makeConfiguredExecutor(const std::string& absPath, + const std::vector& fullArgs, + const std::string& binDir) { + auto executor = + std::make_unique(absPath, fullArgs, buildSandboxeeEnvironment()); + executor->set_enable_sandbox_before_exec(true); + executor->set_cwd(binDir); + executor->limits()->set_walltime_limit(absl::ZeroDuration()); + executor->limits()->set_rlimit_cpu(RLIM64_INFINITY); + executor->limits()->set_rlimit_nofile(65536); + return executor; +} + +//! Production default for the pidfd-acquisition seam: the raw pidfd_open +//! syscall, wrapped in the placeholder success/failure shape Task 3 will +//! replace with full classification (design.md V9). No numeric-kill(pid) +//! fallback is introduced anywhere by this task. +CSandboxedProcessSpawner::SPidFdAcquisitionResult +defaultPidFdOpen(core::CProcess::TPid pid) { + CSandboxedProcessSpawner::SPidFdAcquisitionResult result; + result.s_Fd = static_cast(::syscall(ML_NR_pidfd_open, static_cast(pid), 0u)); + result.s_Errno = (result.s_Fd < 0) ? errno : 0; + return result; +} + +//! Production default for the monitor-thread creation/detach seam: +//! construct a std::thread running monitorBody and detach it, converting +//! any std::system_error from either step into a false return (LI8) instead +//! of letting it propagate as an exception - the caller (spawn()) treats a +//! false return the same way regardless of which step failed. +bool defaultMonitorLaunch(std::function monitorBody) { + try { + std::thread monitor{std::move(monitorBody)}; + monitor.detach(); + return true; + } catch (const std::exception&) { + return false; + } +} + +//! Log how a sandboxed pytorch_inference terminated. Runs on the monitor +//! thread that owns the sandbox instance, so it deliberately takes no +//! spawner state - the caller does the registry bookkeeping under the lock. +void logSandboxeeTermination(core::CProcess::TPid sandboxPid, const sandbox2::Result& result) { + switch (result.final_status()) { + case sandbox2::Result::OK: + if (result.reason_code() == 0) { + LOG_DEBUG(<< "Sandboxed pytorch_inference (PID " << sandboxPid << ") has exited"); + } else { + LOG_WARN(<< "Sandboxed pytorch_inference (PID " << sandboxPid + << ") has exited with exit code " << result.reason_code()); + } + break; + case sandbox2::Result::SIGNALED: + LOG_INFO(<< "Sandboxed pytorch_inference (PID " << sandboxPid + << ") was terminated by signal " << result.reason_code()); + break; + default: + LOG_ERROR(<< "Sandboxed pytorch_inference (PID " << sandboxPid + << ") terminated abnormally, final_status=" << result.final_status()); + break; + } +} + +//! Production default for the registry-allocation seam: lock, allocate the +//! next generation, replace any stale entry for the same PID (closing its +//! pidfd first), insert, and return the new generation. A test overriding +//! this seam can throw (e.g. std::bad_alloc) or return a colliding +//! generation to exercise LI8 deterministically. +std::uint64_t +defaultRegistryInsert(CSandboxedProcessSpawner::SPidRegistry& registry, + core::CProcess::TPid pid, + CSandboxedProcessSpawner::SSandboxedChild child) { + std::lock_guard lock(registry.s_Mutex); + const std::uint64_t generation{++registry.s_NextGeneration}; + const auto existing = registry.s_Children.find(pid); + if (existing != registry.s_Children.end()) { + closePidFdIfOpen(existing->second.s_PidFd); + LOG_DEBUG(<< "Replacing stale registry entry for sandboxed pytorch_inference PID " << pid + << " before registering generation " << generation); + } + child.s_Generation = generation; + child.s_State = CSandboxedProcessSpawner::EChildLifecycleState::E_Registered; + registry.s_Children[pid] = std::move(child); + return generation; +} + +} // namespace + +#endif // SANDBOX2_AVAILABLE + +CSandboxedProcessSpawner::CSandboxedProcessSpawner() = default; + +CSandboxedProcessSpawner::CSandboxedProcessSpawner(TPidFdOpenFn pidFdOpenFn, + TRegistryInsertFn registryInsertFn, + TMonitorLaunchFn monitorLaunchFn +#ifdef SANDBOX2_AVAILABLE + , + TAwaitResultFn awaitResultFn +#endif + ) + : m_PidFdOpenFn{std::move(pidFdOpenFn)}, m_RegistryInsertFn{std::move(registryInsertFn)}, + m_MonitorLaunchFn{std::move(monitorLaunchFn)} +#ifdef SANDBOX2_AVAILABLE + , + m_AwaitResultFn{std::move(awaitResultFn)} +#endif +{ +} + +CSandboxedProcessSpawner::~CSandboxedProcessSpawner() = default; + +bool CSandboxedProcessSpawner::spawn(const std::string& processPath, + const TStrVec& args, + core::CProcess::TPid& childPid) { + childPid = 0; + +#ifdef SANDBOX2_AVAILABLE + + // Resolve to absolute path - Sandbox2 requires absolute paths. + char resolvedPath[PATH_MAX]; + if (::realpath(processPath.c_str(), resolvedPath) == nullptr) { + LOG_ERROR(<< "Cannot resolve path " << processPath << ": " << ::strerror(errno)); + return false; + } + const std::string absPath(resolvedPath); + + struct stat binaryStat; + if (::stat(absPath.c_str(), &binaryStat) != 0) { + LOG_ERROR(<< "Cannot stat " << absPath << ": " << ::strerror(errno)); + return false; + } + + TStrVec fullArgs; + fullArgs.reserve(args.size() + 1); + fullArgs.push_back(processPath); + for (const std::string& arg : args) { + fullArgs.push_back(arg); + } + + // PR C interlock (design.md V16): validate every path-bearing launch + // argument against the pinned child-root contract *before* a policy is + // ever constructed. s_Ok == false must fail the spawn outright - never + // fall back to a partially-built policy. + const char* tmpDirEnv{::getenv("TMPDIR")}; + const std::string trustedTmpDir{tmpDirEnv != nullptr ? tmpDirEnv : "/tmp"}; + const SChildIpcValidationResult validated{validateChildIpcLaunchSpec(trustedTmpDir, args)}; + if (validated.s_Ok == false) { + std::ostringstream rejected; + for (const SRejectedChildIpcPath& r : validated.s_Rejected) { + rejected << " [" << r.s_Arg << ": reason=" << static_cast(r.s_Reason) << ']'; + } + LOG_ERROR(<< "Rejected pytorch_inference child-IPC launch spec for " << processPath + << ':' << rejected.str()); + return false; + } + + // Binary and library directories to bind-mount. libDir is the SIBLING of + // binDir, not a child of it: the ML distribution lays out + // /bin/pytorch_inference alongside /lib, so this + // strips "bin" off binDir before appending "lib" rather than appending + // to binDir. Derived from processPath rather than added as a + // CSandboxedProcessSpawner constructor parameter: spawn()'s signature is + // pinned by the plan and every known caller launches pytorch_inference + // from that fixed distribution layout, so there is nothing a caller- + // supplied binDir/libDir would let a test or caller express that + // deriving from absPath does not already cover. + const std::string binDir{absPath.substr(0, absPath.rfind('/'))}; + const std::string libDir{binDir.substr(0, binDir.rfind('/')) + "/lib"}; + + // A private, bounded tmpfs at /tmp inside the sandbox - never the host's + // shared /tmp. 16 MiB matches the size the PR C mechanism test already + // exercises end-to-end (CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc); + // revisit if a real pytorch_inference workload needs more scratch space. + const std::size_t tmpfsSizeBytes{16 * 1024 * 1024}; + + sandbox2::PolicyBuilder policyBuilder{ + buildPytorchInferenceFilesystemPolicy(binDir, libDir, validated.s_Spec, tmpfsSizeBytes)}; + + auto policyResult = policyBuilder.TryBuild(); + if (!policyResult.ok()) { + LOG_ERROR(<< "Failed to build Sandbox2 policy for " << processPath); + return false; + } + + auto sandboxPtr = std::make_unique( + makeConfiguredExecutor(absPath, fullArgs, binDir), std::move(*policyResult)); + + // E_Launched. + if (!sandboxPtr->RunAsync()) { + sandboxPtr->AwaitResult(); + LOG_ERROR(<< "Sandbox2 failed to start " << processPath); + return false; + } + + childPid = sandboxPtr->pid(); + if (childPid <= 0) { + sandboxPtr->AwaitResult(); + childPid = 0; + LOG_ERROR(<< "Sandbox2 returned an invalid PID for " << processPath); + return false; + } + + // E_IdentityCaptured (LI1): arm the kill-and-reap guard on the raw + // pointer *before* the shared_ptr conversion below, which is itself a + // potentially-throwing allocation (MG3's control-block allocation + // failure) as well as before registry insertion, monitor-thread + // construction, and detach(). If the shared_ptr constructor throws, the + // standard guarantees it has no effect on the moved-from unique_ptr, so + // sandboxPtr (and therefore the raw pointer the guard holds) still owns + // a live object the guard can Kill()/await. + CKillAndReapGuard killAndReapGuard{sandboxPtr.get(), m_AwaitResultFn}; + + std::shared_ptr sandbox; + try { + sandbox = std::shared_ptr(std::move(sandboxPtr)); + } catch (const std::exception& e) { + LOG_ERROR(<< "Failed to take shared ownership of a launched sandboxee for " << processPath + << ": " << e.what()); + childPid = 0; + return false; // killAndReapGuard fires here. + } + + const core::CProcess::TPid sandboxPid{childPid}; + + const SPidFdAcquisitionResult pidFdResult{m_PidFdOpenFn ? m_PidFdOpenFn(sandboxPid) + : defaultPidFdOpen(sandboxPid)}; + CScopedPidFd pidFdGuard{pidFdResult.s_Fd}; + // A negative pidfd (ENOSYS on kernels <5.3, or a resource error) is not + // itself a spawn failure in this task's scope - Task 3 owns deciding + // whether/what identity-bound fallback a classified failure selects. + // Registration proceeds either way with s_PidFd left at -1. + + SSandboxedChild child; + child.s_State = EChildLifecycleState::E_IdentityCaptured; + child.s_Sandbox = sandbox; + child.s_PidFd = pidFdGuard.get(); + child.s_Outcome = std::make_shared(); + + std::uint64_t generation{0}; + try { + generation = m_RegistryInsertFn ? m_RegistryInsertFn(*m_PidRegistry, sandboxPid, child) + : defaultRegistryInsert(*m_PidRegistry, sandboxPid, child); + } catch (const std::exception& e) { + LOG_ERROR(<< "Failed to register sandboxed process " << processPath << " (PID " + << sandboxPid << "): " << e.what()); + childPid = 0; + return false; // killAndReapGuard fires here; pidFdGuard still owns the fd. + } + // E_Registered. The registry entry now owns the pidfd; do not double- + // close it via pidFdGuard's destructor on this path. + pidFdGuard.release(); + + // The sandboxee is a child of the Sandbox2 forkserver rather than of the + // controller, so waitpid() never sees it. Own the sandbox instance on a + // dedicated monitor thread that keeps it alive for the lifetime of + // pytorch_inference, waits for its result (via the injectable + // AwaitResult seam), and removes the registry entry before logging + // termination. The thread co-owns the registry and the Sandbox2 + // shared_ptr rather than capturing this: it can still be waiting on a + // live sandboxee when the spawner is destroyed (LI9), and a raw pointer + // back to the spawner would be dangling by then. + const TPidRegistryPtr registry{m_PidRegistry}; + const TAwaitResultFn awaitResultFn{m_AwaitResultFn}; + auto monitorBody = [sandboxPid, registry, sandbox, generation, awaitResultFn]() { + const sandbox2::Result result{awaitResultFn ? awaitResultFn(*sandbox) + : sandbox->AwaitResult()}; + { + std::lock_guard lock(registry->s_Mutex); + const auto it = registry->s_Children.find(sandboxPid); + if (it != registry->s_Children.end() && it->second.s_Generation == generation) { + closePidFdIfOpen(it->second.s_PidFd); + registry->s_Children.erase(it); + } + } + logSandboxeeTermination(sandboxPid, result); + }; + + const bool monitorStarted{m_MonitorLaunchFn ? m_MonitorLaunchFn(std::move(monitorBody)) + : defaultMonitorLaunch(std::move(monitorBody))}; + if (monitorStarted == false) { + // Monitor handoff failed (LI8): no thread is running to ever erase + // this registry entry or call AwaitResult(), so this frame owns + // both. Drop the entry this call inserted (matching by generation, + // in case a racing call already replaced it), then let + // killAndReapGuard's destructor Kill()/await the sandboxee. + { + std::lock_guard lock(m_PidRegistry->s_Mutex); + const auto it = m_PidRegistry->s_Children.find(sandboxPid); + if (it != m_PidRegistry->s_Children.end() && it->second.s_Generation == generation) { + closePidFdIfOpen(it->second.s_PidFd); + m_PidRegistry->s_Children.erase(it); + } + } + LOG_ERROR(<< "Failed to start monitor thread for sandboxed process " << processPath + << " (PID " << sandboxPid << ")"); + childPid = 0; + return false; // killAndReapGuard fires here. + } + + // E_Monitoring (LI2): registry insertion and monitor handoff both + // succeeded, so the monitor thread now owns calling AwaitResult() and + // removing the registry entry. Disarm - the guard must not also reap. + killAndReapGuard.disarm(); + + LOG_INFO(<< "Spawned sandboxed process " << processPath << " with PID " << childPid); + + return true; + +#else // !SANDBOX2_AVAILABLE + + LOG_ERROR(<< "Cannot spawn " << processPath + << ": ml-cpp was built without Sandbox2 support"); + return false; + +#endif // SANDBOX2_AVAILABLE +} + +bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid /* pid */) { + // Task 3 owns pidfd-based signalling and pidfd-outcome classification + // (design.md V9); this task's scope is spawn()'s kill-and-reap guard + // and injectable seams only. Deliberately always returns false rather + // than a numeric-PID kill(pid) fallback, which the rebuild plan + // forbids as a termination mechanism. + return false; +} + +bool CSandboxedProcessSpawner::hasChild(core::CProcess::TPid pid) const { + std::lock_guard lock(m_PidRegistry->s_Mutex); + const auto it = m_PidRegistry->s_Children.find(pid); + return it != m_PidRegistry->s_Children.end() && + it->second.s_State != EChildLifecycleState::E_Reaped && + it->second.s_State != EChildLifecycleState::E_Failed; +} + +} // namespace sandbox +} // namespace ml From 5ead4037053617a889424bb851e0130ccf2bc447 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:22:27 +0200 Subject: [PATCH 06/15] [ML] Fix guard UAF and unguarded exception window in spawn() CKillAndReapGuard held a raw, non-owning Sandbox2* declared before the shared_ptr locals (sandbox, child.s_Sandbox) that could become its last owner; on the registry-insert-throw and monitor-launch-fail paths those locals were destroyed before the guard, freeing the object before the guard's destructor called Kill() on it. The guard now stores its own shared_ptr copy, making its cleanup self-sufficient regardless of other locals' declaration order; the unique_ptr->shared_ptr conversion moves earlier (before RunAsync()) so the guard can be constructed from a real shared_ptr once the process is actually running. Also wrap the span between successful registry insertion and monitor launch (copying seams, constructing monitorBody) in try/catch, so an exception there (e.g. bad_alloc copying a std::function) can no longer escape spawn() leaking the registry entry; erase-on-failure logic is factored into a shared lambda used by both the new catch and the existing monitor-launch-false path. --- lib/sandbox/CSandboxedProcessSpawner_Linux.cc | 159 ++++++++++++------ 1 file changed, 104 insertions(+), 55 deletions(-) diff --git a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc index 0491a49f87..00cb9c0861 100644 --- a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc +++ b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc @@ -110,6 +110,20 @@ void closePidFdIfOpen(int pidFd) { //! duplicate cleanup block (design.md LI3), so there is exactly one cleanup //! owner for "launched but not yet fully handed off". //! +//! Holds its own shared_ptr copy (not a raw, non-owning pointer) +//! so its lifetime is entirely self-sufficient: it does not matter what +//! order this guard is declared in relative to other shared_ptr-holding +//! locals in spawn() (e.g. `sandbox`, `child.s_Sandbox`), nor what order +//! those locals get destroyed in during stack unwinding on a failure path. +//! A raw pointer previously used here relied on some other local staying +//! alive for the guard's own destructor to run safely against; if that +//! local's declaration (and therefore destruction) order ever changed, or +//! if the object's last owning shared_ptr was destroyed before this guard +//! during unwinding, the guard's destructor would call Kill() on a dangling +//! pointer. Holding an owning copy makes that structurally impossible: this +//! guard is always one of the owners, so the object cannot be freed before +//! this guard's own destructor has run. +//! //! The destructor must not throw: it runs during stack unwinding on the //! failure paths this guard exists to cover, and a second exception there //! would call std::terminate. Kill() and the AwaitResult seam are wrapped in @@ -118,12 +132,12 @@ void closePidFdIfOpen(int pidFd) { //! escape a destructor. class CKillAndReapGuard { public: - CKillAndReapGuard(sandbox2::Sandbox2* sandbox, + CKillAndReapGuard(std::shared_ptr sandbox, CSandboxedProcessSpawner::TAwaitResultFn awaitResultFn) - : m_Sandbox{sandbox}, m_AwaitResultFn{std::move(awaitResultFn)} {} + : m_Sandbox{std::move(sandbox)}, m_AwaitResultFn{std::move(awaitResultFn)} {} ~CKillAndReapGuard() { - if (m_Armed && m_Sandbox != nullptr) { + if (m_Armed && m_Sandbox) { try { m_Sandbox->Kill(); if (m_AwaitResultFn) { @@ -148,7 +162,7 @@ class CKillAndReapGuard { void disarm() { m_Armed = false; } private: - sandbox2::Sandbox2* m_Sandbox; + std::shared_ptr m_Sandbox; CSandboxedProcessSpawner::TAwaitResultFn m_AwaitResultFn; bool m_Armed{true}; }; @@ -365,43 +379,53 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, auto sandboxPtr = std::make_unique( makeConfiguredExecutor(absPath, fullArgs, binDir), std::move(*policyResult)); + // Take shared ownership immediately, before RunAsync() ever launches + // anything - not after pid() is captured. This conversion can itself + // throw (MG3's control-block allocation failure), but nothing has been + // launched yet at this point, so sandboxPtr's own (plain) destructor is + // sufficient cleanup on that failure; no Kill()/AwaitResult() is needed + // for a sandboxee that was never started. Doing this early - rather + // than arming CKillAndReapGuard on a raw, non-owning pointer into the + // still-unique_ptr-owned object and converting to shared_ptr afterward + // - means the guard constructed below always holds a genuine owning + // shared_ptr copy, making its cleanup self-sufficient regardless of + // declaration/destruction order among the other shared_ptr-holding + // locals later in this function (`sandbox` itself, `child.s_Sandbox`). + std::shared_ptr sandbox; + try { + sandbox = std::shared_ptr(std::move(sandboxPtr)); + } catch (const std::exception& e) { + LOG_ERROR(<< "Failed to take shared ownership of a sandboxee for " << processPath << ": " + << e.what()); + return false; + } + // E_Launched. - if (!sandboxPtr->RunAsync()) { - sandboxPtr->AwaitResult(); + if (!sandbox->RunAsync()) { + sandbox->AwaitResult(); LOG_ERROR(<< "Sandbox2 failed to start " << processPath); return false; } - childPid = sandboxPtr->pid(); + childPid = sandbox->pid(); if (childPid <= 0) { - sandboxPtr->AwaitResult(); + sandbox->AwaitResult(); childPid = 0; LOG_ERROR(<< "Sandbox2 returned an invalid PID for " << processPath); return false; } - // E_IdentityCaptured (LI1): arm the kill-and-reap guard on the raw - // pointer *before* the shared_ptr conversion below, which is itself a - // potentially-throwing allocation (MG3's control-block allocation - // failure) as well as before registry insertion, monitor-thread - // construction, and detach(). If the shared_ptr constructor throws, the - // standard guarantees it has no effect on the moved-from unique_ptr, so - // sandboxPtr (and therefore the raw pointer the guard holds) still owns - // a live object the guard can Kill()/await. - CKillAndReapGuard killAndReapGuard{sandboxPtr.get(), m_AwaitResultFn}; - - std::shared_ptr sandbox; - try { - sandbox = std::shared_ptr(std::move(sandboxPtr)); - } catch (const std::exception& e) { - LOG_ERROR(<< "Failed to take shared ownership of a launched sandboxee for " << processPath - << ": " << e.what()); - childPid = 0; - return false; // killAndReapGuard fires here. - } - const core::CProcess::TPid sandboxPid{childPid}; + // E_IdentityCaptured (LI1): arm the kill-and-reap guard now that the + // sandboxee is actually running. The guard takes its own shared_ptr + // copy of `sandbox` (see CKillAndReapGuard's comment), so it remains + // valid through every early return below - registry-insert throw, + // monitor-launch-span throw, monitor-launch-seam false - independent of + // when `sandbox`/`child.s_Sandbox` themselves get destroyed during + // stack unwinding. + CKillAndReapGuard killAndReapGuard{sandbox, m_AwaitResultFn}; + const SPidFdAcquisitionResult pidFdResult{m_PidFdOpenFn ? m_PidFdOpenFn(sandboxPid) : defaultPidFdOpen(sandboxPid)}; CScopedPidFd pidFdGuard{pidFdResult.s_Fd}; @@ -430,6 +454,20 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, // close it via pidFdGuard's destructor on this path. pidFdGuard.release(); + // Erase the registry entry this call just inserted, matching by + // generation (in case a racing call already replaced it). Shared by + // every failure path between a successful registry insertion and a + // successful monitor handoff, since no monitor thread exists on any of + // those paths to ever perform that erase itself. + const auto eraseRegistryEntry = [this, sandboxPid, generation]() { + std::lock_guard lock(m_PidRegistry->s_Mutex); + const auto it = m_PidRegistry->s_Children.find(sandboxPid); + if (it != m_PidRegistry->s_Children.end() && it->second.s_Generation == generation) { + closePidFdIfOpen(it->second.s_PidFd); + m_PidRegistry->s_Children.erase(it); + } + }; + // The sandboxee is a child of the Sandbox2 forkserver rather than of the // controller, so waitpid() never sees it. Own the sandbox instance on a // dedicated monitor thread that keeps it alive for the lifetime of @@ -439,38 +477,49 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, // shared_ptr rather than capturing this: it can still be waiting on a // live sandboxee when the spawner is destroyed (LI9), and a raw pointer // back to the spawner would be dangling by then. - const TPidRegistryPtr registry{m_PidRegistry}; - const TAwaitResultFn awaitResultFn{m_AwaitResultFn}; - auto monitorBody = [sandboxPid, registry, sandbox, generation, awaitResultFn]() { - const sandbox2::Result result{awaitResultFn ? awaitResultFn(*sandbox) - : sandbox->AwaitResult()}; - { - std::lock_guard lock(registry->s_Mutex); - const auto it = registry->s_Children.find(sandboxPid); - if (it != registry->s_Children.end() && it->second.s_Generation == generation) { - closePidFdIfOpen(it->second.s_PidFd); - registry->s_Children.erase(it); + // + // Everything from copying m_PidRegistry/m_AwaitResultFn through + // launching the monitor thread runs inside a try/catch: those copies + // and constructing monitorBody's capture list can themselves throw + // (e.g. std::bad_alloc copying a std::function), and left unguarded + // that exception would otherwise escape spawn() uncaught, leaking the + // just-inserted registry entry. Catching here ensures every throw in + // this span still erases the registry entry and returns false with + // childPid == 0 (LI3); killAndReapGuard's destructor performs the + // Kill()/await half of cleanup on unwind either way. + bool monitorStarted{false}; + try { + const TPidRegistryPtr registry{m_PidRegistry}; + const TAwaitResultFn awaitResultFn{m_AwaitResultFn}; + auto monitorBody = [sandboxPid, registry, sandbox, generation, awaitResultFn]() { + const sandbox2::Result result{awaitResultFn ? awaitResultFn(*sandbox) + : sandbox->AwaitResult()}; + { + std::lock_guard lock(registry->s_Mutex); + const auto it = registry->s_Children.find(sandboxPid); + if (it != registry->s_Children.end() && it->second.s_Generation == generation) { + closePidFdIfOpen(it->second.s_PidFd); + registry->s_Children.erase(it); + } } - } - logSandboxeeTermination(sandboxPid, result); - }; + logSandboxeeTermination(sandboxPid, result); + }; + + monitorStarted = m_MonitorLaunchFn ? m_MonitorLaunchFn(std::move(monitorBody)) + : defaultMonitorLaunch(std::move(monitorBody)); + } catch (const std::exception& e) { + eraseRegistryEntry(); + LOG_ERROR(<< "Failed to launch monitor thread for sandboxed process " << processPath + << " (PID " << sandboxPid << "): " << e.what()); + childPid = 0; + return false; // killAndReapGuard fires here. + } - const bool monitorStarted{m_MonitorLaunchFn ? m_MonitorLaunchFn(std::move(monitorBody)) - : defaultMonitorLaunch(std::move(monitorBody))}; if (monitorStarted == false) { // Monitor handoff failed (LI8): no thread is running to ever erase // this registry entry or call AwaitResult(), so this frame owns - // both. Drop the entry this call inserted (matching by generation, - // in case a racing call already replaced it), then let - // killAndReapGuard's destructor Kill()/await the sandboxee. - { - std::lock_guard lock(m_PidRegistry->s_Mutex); - const auto it = m_PidRegistry->s_Children.find(sandboxPid); - if (it != m_PidRegistry->s_Children.end() && it->second.s_Generation == generation) { - closePidFdIfOpen(it->second.s_PidFd); - m_PidRegistry->s_Children.erase(it); - } - } + // both. killAndReapGuard's destructor Kill()s/awaits the sandboxee. + eraseRegistryEntry(); LOG_ERROR(<< "Failed to start monitor thread for sandboxed process " << processPath << " (PID " << sandboxPid << ")"); childPid = 0; From c5ea3b5e4ea38dcbffdc04cb93fd430a05fb3845 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:36:12 +0200 Subject: [PATCH 07/15] [ML] pidfd outcome classification, ENOSYS->Sandbox2::Kill() routing, terminateChild() Adds EPidFdOutcome (E_Acquired/E_KernelUnsupported/E_Failed) and a pure classifyPidFdOutcome() so spawn() fails registration outright on any non-ENOSYS pidfd_open errno (MG2/LI8), and records the classification on the registry entry at registration time. Implements terminateChild() for real: pidfd_send_signal(SIGTERM) request for E_Acquired, Sandbox2::Kill() (SIGKILL via the owned monitor) for E_KernelUnsupported only - selected from the recorded classification, never re-derived. Wires the monitor-body completion path through CCasOutcomeLatch::tryResolve() (MG4/V11) instead of an ad-hoc boolean. No numeric kill(pid) call exists anywhere in the file. --- include/sandbox/CSandboxedProcessSpawner.h | 40 ++++ lib/sandbox/CSandboxedProcessSpawner_Linux.cc | 190 ++++++++++++++++-- 2 files changed, 215 insertions(+), 15 deletions(-) diff --git a/include/sandbox/CSandboxedProcessSpawner.h b/include/sandbox/CSandboxedProcessSpawner.h index 237f4d9fe5..a921390cf8 100644 --- a/include/sandbox/CSandboxedProcessSpawner.h +++ b/include/sandbox/CSandboxedProcessSpawner.h @@ -138,6 +138,39 @@ class CSandboxedProcessSpawner { int s_Errno{0}; }; + //! Explicit classification of a pidfd-acquisition attempt (design.md + //! gate V9), replacing the Task 2 placeholder's generic "negative fd" + //! check with a three-way outcome that decides both whether spawn() + //! registers the child at all, and - for a registered child - which of + //! terminateChild()'s two mechanisms applies. + //! + //! E_Acquired: s_Fd >= 0. terminateChild() sends a request via + //! pidfd_send_signal(SIGTERM) on the held pidfd. + //! + //! E_KernelUnsupported: s_Fd < 0 and s_Errno == ENOSYS - the running + //! kernel predates pidfd support entirely (pre-5.3). This is the *only* + //! classification for which terminateChild() falls back to + //! Sandbox2::Kill() (SIGKILL via the owned monitor, identity-safe, no + //! numeric-PID lookup). Recorded on the registry entry at registration + //! time - terminateChild() must use that recorded value, never + //! re-derive it by re-calling pidfd_open. + //! + //! E_Failed: s_Fd < 0 and s_Errno is anything else (ESRCH, EMFILE, + //! ENFILE, ...). Per design.md MG2/LI8, this is a resource or identity + //! error, not "no kernel support" - it must never be treated the same + //! as E_KernelUnsupported. spawn() fails registration outright on this + //! outcome rather than registering a child whose termination would need + //! an undefined fallback. + enum class EPidFdOutcome { E_Acquired, E_KernelUnsupported, E_Failed }; + + //! Pure classification function for a pidfd-acquisition result: no + //! syscalls, no I/O, no side effects, so it is unit-testable in + //! isolation against synthetic SPidFdAcquisitionResult values (e.g. Task + //! 4's ENOSYS/EMFILE/ESRCH/success cases) without a real pidfd or + //! kernel. Implemented outside the SANDBOX2_AVAILABLE-gated block in the + //! .cc, so it compiles - and is testable - on every platform. + static EPidFdOutcome classifyPidFdOutcome(const SPidFdAcquisitionResult& result); + public: //! \brief A live sandboxed child and the handles needed to manage it //! safely through every lifecycle state. @@ -161,6 +194,13 @@ class CSandboxedProcessSpawner { std::uint64_t s_Generation{0}; std::shared_ptr s_Sandbox; int s_PidFd{-1}; + //! Classification recorded at registration time (Task 3, design.md + //! V9). Every entry that actually reaches the registry has this set + //! to E_Acquired or E_KernelUnsupported - E_Failed never gets + //! registered (see EPidFdOutcome's comment) - but the default below + //! still resolves to the fail-closed value in case some future path + //! forgets to set it explicitly. + EPidFdOutcome s_PidFdOutcome{EPidFdOutcome::E_Failed}; std::shared_ptr s_Outcome; }; diff --git a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc index 00cb9c0861..e8b9b25b8f 100644 --- a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc +++ b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc @@ -13,11 +13,20 @@ #include #include +#include #include #include #include #include +// classifyPidFdOutcome (design.md gate V9) is a pure function with no +// syscalls or Sandbox2 types in its signature, so - unlike the rest of this +// file - it is defined below outside the SANDBOX2_AVAILABLE-gated block: it +// must compile, and be unit-testable, on every platform, matching this TU's +// own "compiled unconditionally" contract (see the comment above the +// SANDBOX2_AVAILABLE block). (for ENOSYS) is therefore included +// unconditionally too, rather than inside that block alongside . + // This translation unit is compiled unconditionally (see lib/sandbox/CMakeLists.txt // - it is added to SRCS the same way lib/core/CMakeLists.txt unconditionally // builds CDetachedProcessSpawner.cc), so every symbol outside the @@ -31,6 +40,7 @@ #include #include +#include #include #include #include @@ -59,11 +69,39 @@ extern char** environ; #define ML_NR_pidfd_open 434 #endif +// Same rationale as ML_NR_pidfd_open above: pidfd_send_signal is syscall +// number 424 on every architecture ml-cpp builds for (x86_64 and aarch64), +// so fall back to that literal when the build image's kernel headers +// predate it. Used by terminateChild()'s E_Acquired path (SIGTERM request +// via the held pidfd) - the only place this file sends a signal to a +// sandboxee by identity-bound handle rather than by recycled numeric PID. +#ifdef __NR_pidfd_send_signal +#define ML_NR_pidfd_send_signal __NR_pidfd_send_signal +#else +#define ML_NR_pidfd_send_signal 424 +#endif + #endif // SANDBOX2_AVAILABLE namespace ml { namespace sandbox { +// Defined outside the SANDBOX2_AVAILABLE-gated block below (unlike +// everything else in this file): a pure function with no syscalls, no +// Sandbox2 types, and no platform-specific behaviour, so it must compile - +// and be unit-testable - on every configure, matching this TU's +// "compiled unconditionally" contract (see the file-level comment above). +CSandboxedProcessSpawner::EPidFdOutcome CSandboxedProcessSpawner::classifyPidFdOutcome( + const CSandboxedProcessSpawner::SPidFdAcquisitionResult& result) { + if (result.s_Fd >= 0) { + return EPidFdOutcome::E_Acquired; + } + if (result.s_Errno == ENOSYS) { + return EPidFdOutcome::E_KernelUnsupported; + } + return EPidFdOutcome::E_Failed; +} + #ifdef SANDBOX2_AVAILABLE namespace { @@ -205,9 +243,10 @@ std::unique_ptr makeConfiguredExecutor(const std::string& ab } //! Production default for the pidfd-acquisition seam: the raw pidfd_open -//! syscall, wrapped in the placeholder success/failure shape Task 3 will -//! replace with full classification (design.md V9). No numeric-kill(pid) -//! fallback is introduced anywhere by this task. +//! syscall. classifyPidFdOutcome() (defined below, outside this +//! SANDBOX2_AVAILABLE block) turns this raw fd/errno pair into the +//! Acquired/KernelUnsupported/Failed classification spawn() acts on. No +//! numeric-kill(pid) fallback is introduced anywhere by this file. CSandboxedProcessSpawner::SPidFdAcquisitionResult defaultPidFdOpen(core::CProcess::TPid pid) { CSandboxedProcessSpawner::SPidFdAcquisitionResult result; @@ -429,15 +468,28 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, const SPidFdAcquisitionResult pidFdResult{m_PidFdOpenFn ? m_PidFdOpenFn(sandboxPid) : defaultPidFdOpen(sandboxPid)}; CScopedPidFd pidFdGuard{pidFdResult.s_Fd}; - // A negative pidfd (ENOSYS on kernels <5.3, or a resource error) is not - // itself a spawn failure in this task's scope - Task 3 owns deciding - // whether/what identity-bound fallback a classified failure selects. - // Registration proceeds either way with s_PidFd left at -1. + const EPidFdOutcome pidFdOutcome{classifyPidFdOutcome(pidFdResult)}; + + // MG2/LI8: an errno other than ENOSYS (ESRCH, EMFILE, ENFILE, ...) is a + // resource/identity error, not "no kernel support" for pidfd - it must + // never be treated the same as E_KernelUnsupported. Fail registration + // outright rather than register a child whose termination would need an + // undefined fallback. pidFdGuard closes any fd this path somehow still + // holds; killAndReapGuard (still armed) Kill()s/awaits the sandboxee. + if (pidFdOutcome == EPidFdOutcome::E_Failed) { + LOG_ERROR(<< "pidfd_open failed for sandboxed process " << processPath << " (PID " + << sandboxPid << ") with errno " << pidFdResult.s_Errno << " (" + << ::strerror(pidFdResult.s_Errno) + << "); refusing to register a child with an undefined termination fallback"); + childPid = 0; + return false; // killAndReapGuard fires here; pidFdGuard closes any fd on unwind. + } SSandboxedChild child; child.s_State = EChildLifecycleState::E_IdentityCaptured; child.s_Sandbox = sandbox; child.s_PidFd = pidFdGuard.get(); + child.s_PidFdOutcome = pidFdOutcome; child.s_Outcome = std::make_shared(); std::uint64_t generation{0}; @@ -494,15 +546,33 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, auto monitorBody = [sandboxPid, registry, sandbox, generation, awaitResultFn]() { const sandbox2::Result result{awaitResultFn ? awaitResultFn(*sandbox) : sandbox->AwaitResult()}; + // MG4/V11: this thread's completion and a (currently unwired - + // Task 3 scope stops at this call site; no external timeout + // caller exists yet) timeout path both race to decide who + // performs cleanup for the same child. Route that decision + // through exactly one tryResolve() call on the child's own CAS + // latch rather than an ad-hoc boolean - if a timeout caller + // resolves the latch to E_TimedOut first, this call loses the + // race and must not also erase the registry entry or log + // termination (the timeout path owns that instead). + bool completionWonRace{true}; { std::lock_guard lock(registry->s_Mutex); const auto it = registry->s_Children.find(sandboxPid); if (it != registry->s_Children.end() && it->second.s_Generation == generation) { - closePidFdIfOpen(it->second.s_PidFd); - registry->s_Children.erase(it); + if (it->second.s_Outcome) { + EOutcomeState desired{EOutcomeState::E_Completed}; + completionWonRace = it->second.s_Outcome->tryResolve(desired); + } + if (completionWonRace) { + closePidFdIfOpen(it->second.s_PidFd); + registry->s_Children.erase(it); + } } } - logSandboxeeTermination(sandboxPid, result); + if (completionWonRace) { + logSandboxeeTermination(sandboxPid, result); + } }; monitorStarted = m_MonitorLaunchFn ? m_MonitorLaunchFn(std::move(monitorBody)) @@ -544,15 +614,105 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, #endif // SANDBOX2_AVAILABLE } +#ifdef SANDBOX2_AVAILABLE + +bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { + // Two mechanisms only, selected by the classification recorded on the + // registry entry at *registration* time (never re-derived here by + // re-calling pidfd_open, per the task brief): pidfd_send_signal(SIGTERM) + // - a graceful termination *request* - for E_Acquired, or Sandbox2::Kill() + // (SIGKILL via the owned monitor) for E_KernelUnsupported. No numeric + // kill(pid) fallback exists anywhere in this file. + int pidFdToSignal{-1}; + std::shared_ptr sandboxToKill; + { + std::lock_guard lock(m_PidRegistry->s_Mutex); + const auto it = m_PidRegistry->s_Children.find(pid); + if (it == m_PidRegistry->s_Children.end() || + it->second.s_State == EChildLifecycleState::E_Reaped || + it->second.s_State == EChildLifecycleState::E_Failed) { + return false; + } + SSandboxedChild& child{it->second}; + switch (child.s_PidFdOutcome) { + case EPidFdOutcome::E_Acquired: + if (child.s_PidFd < 0) { + // Logic error (should be structurally unreachable given + // spawn()'s fail-closed registration in this task): a + // registry entry classified E_Acquired must hold a real + // pidfd. Do not silently no-op - log loudly and refuse. + LOG_ERROR(<< "Logic error: sandboxed child PID " << pid + << " classified E_Acquired but holds no pidfd"); + return false; + } + pidFdToSignal = child.s_PidFd; + break; + case EPidFdOutcome::E_KernelUnsupported: + if (!child.s_Sandbox) { + // Same reasoning as above: E_KernelUnsupported without a + // Sandbox2 handle to Kill() is a logic error, not a + // silent no-op. + LOG_ERROR(<< "Logic error: sandboxed child PID " << pid + << " classified E_KernelUnsupported but holds no Sandbox2 handle"); + return false; + } + sandboxToKill = child.s_Sandbox; + break; + case EPidFdOutcome::E_Failed: + default: + // Structurally unreachable: spawn() never registers an + // E_Failed child (see the pidFdOutcome check above it). Assert + // in debug builds and refuse rather than silently no-op if it + // somehow happened anyway. + LOG_ERROR(<< "Logic error: sandboxed child PID " << pid + << " registered with an undefined termination fallback (classification=" + << static_cast(child.s_PidFdOutcome) << ')'); + return false; + } + child.s_State = EChildLifecycleState::E_TerminationRequested; + } + + if (pidFdToSignal >= 0) { + if (::syscall(ML_NR_pidfd_send_signal, pidFdToSignal, SIGTERM, nullptr, 0u) != 0) { + LOG_ERROR(<< "pidfd_send_signal(SIGTERM) failed for sandboxed child PID " << pid + << ": " << ::strerror(errno)); + return false; + } + return true; + } + + // sandboxToKill is only ever set on the E_KernelUnsupported branch + // above; pidFdToSignal >= 0 is only ever set on the E_Acquired branch. + // Exactly one of the two is populated by the switch, so reaching here + // with neither would itself be a logic error - defensively refuse + // rather than silently no-op. + if (!sandboxToKill) { + LOG_ERROR(<< "Logic error: terminateChild() for PID " << pid + << " resolved neither a pidfd nor a Sandbox2 handle to kill"); + return false; + } + + try { + // Locked design decision (design.md): MonitorBase::Kill() takes no + // signal parameter and hard-codes SIGKILL - this is the ENOSYS + // forced-kill fallback, never a SIGTERM-via-monitor path. + sandboxToKill->Kill(); + } catch (const std::exception& e) { + LOG_ERROR(<< "Sandbox2::Kill() failed for sandboxed child PID " << pid << ": " + << e.what()); + return false; + } + return true; +} + +#else // !SANDBOX2_AVAILABLE + bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid /* pid */) { - // Task 3 owns pidfd-based signalling and pidfd-outcome classification - // (design.md V9); this task's scope is spawn()'s kill-and-reap guard - // and injectable seams only. Deliberately always returns false rather - // than a numeric-PID kill(pid) fallback, which the rebuild plan - // forbids as a termination mechanism. return false; } +#endif // SANDBOX2_AVAILABLE + bool CSandboxedProcessSpawner::hasChild(core::CProcess::TPid pid) const { std::lock_guard lock(m_PidRegistry->s_Mutex); const auto it = m_PidRegistry->s_Children.find(pid); From 9971a804ef4b1a3d57afd8d5d40e518bdc80a916 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:43:21 +0200 Subject: [PATCH 08/15] [ML] terminateChild(): roll back s_State on failed termination attempt Fix round 1/5 for Task 3 review: terminateChild() set s_State to E_TerminationRequested before releasing the registry lock and attempting the actual pidfd_send_signal()/Sandbox2::Kill() call. On failure of that call it returned false but left s_State claiming a termination request had been issued, with no rollback and no distinguishable "attempted and failed" state. Capture the entry's prior state before overwriting it, and on each failure path (pidfd_send_signal() non-zero return, the defensive neither-pidfd-nor-sandbox branch, and Sandbox2::Kill() throwing) re-acquire the registry lock briefly to roll s_State back to what it was, but only if nothing else has since moved the state on. The syscall/Kill() call itself still happens outside the lock, unchanged. --- lib/sandbox/CSandboxedProcessSpawner_Linux.cc | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc index e8b9b25b8f..72d4cf822f 100644 --- a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc +++ b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc @@ -625,6 +625,7 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { // kill(pid) fallback exists anywhere in this file. int pidFdToSignal{-1}; std::shared_ptr sandboxToKill; + EChildLifecycleState previousState{EChildLifecycleState::E_Failed}; { std::lock_guard lock(m_PidRegistry->s_Mutex); const auto it = m_PidRegistry->s_Children.find(pid); @@ -634,6 +635,7 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { return false; } SSandboxedChild& child{it->second}; + previousState = child.s_State; switch (child.s_PidFdOutcome) { case EPidFdOutcome::E_Acquired: if (child.s_PidFd < 0) { @@ -672,10 +674,26 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { child.s_State = EChildLifecycleState::E_TerminationRequested; } + // Rolls the registry entry's s_State back to what it was before this + // call optimistically set it to E_TerminationRequested, but only if + // nothing else has moved the state on in the meantime (e.g. a + // concurrent reap). Called on the failure paths below, after the actual + // pidfd_send_signal()/Kill() call - re-acquires the lock briefly; the + // call itself still happens outside the lock, unchanged. + const auto rollBackState = [this, pid, previousState]() { + std::lock_guard lock(m_PidRegistry->s_Mutex); + const auto it = m_PidRegistry->s_Children.find(pid); + if (it != m_PidRegistry->s_Children.end() && + it->second.s_State == EChildLifecycleState::E_TerminationRequested) { + it->second.s_State = previousState; + } + }; + if (pidFdToSignal >= 0) { if (::syscall(ML_NR_pidfd_send_signal, pidFdToSignal, SIGTERM, nullptr, 0u) != 0) { LOG_ERROR(<< "pidfd_send_signal(SIGTERM) failed for sandboxed child PID " << pid << ": " << ::strerror(errno)); + rollBackState(); return false; } return true; @@ -689,6 +707,7 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { if (!sandboxToKill) { LOG_ERROR(<< "Logic error: terminateChild() for PID " << pid << " resolved neither a pidfd nor a Sandbox2 handle to kill"); + rollBackState(); return false; } @@ -700,6 +719,7 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { } catch (const std::exception& e) { LOG_ERROR(<< "Sandbox2::Kill() failed for sandboxed child PID " << pid << ": " << e.what()); + rollBackState(); return false; } return true; From 282af8d6edabe2ff82fa1d987ab79a2e655bbd42 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:08:56 +0200 Subject: [PATCH 09/15] [ML] Add CSandboxedProcessSpawnerLifecycleTest_Linux (PR D Task 4) Implements the 8 required lifecycle proofs (pidfd classification, allocation/monitor-launch failure, stale generation, PID reuse, timeout/completion race, descriptor baseline, destructor latency, monitor-outlives-spawner) by driving CSandboxedProcessSpawner's injectable seams against one real spawn() per case, since no seam bypasses RunAsync() itself and the private registry has no external accessor other than the registry-insert seam's mutable reference. Adds lifecycle_signal_payload.cc, a long-lived sandboxee that catches and survives SIGTERM but not SIGKILL (using FUTEX_WAIT, since pause() is not in the seccomp allowlist), needed to distinguish terminateChild()'s two mechanisms by observable effect - there is no seam around Sandbox2::Kill() itself. See task-4-report.md for the full list of self-review findings, open items (no TSan/ASan CI job exists yet; ML_SANDBOX2_REQUIRE gating referenced in the brief does not exist in this checkout; gate 7's orphan-cleanup half is explicitly out of scope), and what could/couldn't be verified on this macOS host. Co-Authored-By: Claude Sonnet 5 --- lib/sandbox/unittest/CMakeLists.txt | 30 + ...dboxedProcessSpawnerLifecycleTest_Linux.cc | 914 ++++++++++++++++++ .../payloads/lifecycle_signal_payload.cc | 72 ++ 3 files changed, 1016 insertions(+) create mode 100644 lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc create mode 100644 lib/sandbox/unittest/payloads/lifecycle_signal_payload.cc diff --git a/lib/sandbox/unittest/CMakeLists.txt b/lib/sandbox/unittest/CMakeLists.txt index 76d542402a..34f6d73e6b 100644 --- a/lib/sandbox/unittest/CMakeLists.txt +++ b/lib/sandbox/unittest/CMakeLists.txt @@ -34,6 +34,7 @@ if(TARGET sandbox2::sandbox2 AND CMAKE_SYSTEM_NAME STREQUAL "Linux") # are unavailable on non-Linux configure runs. list(APPEND SRCS CSandboxForkserverSmokeTest.cc) list(APPEND SRCS CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc) + list(APPEND SRCS CSandboxedProcessSpawnerLifecycleTest_Linux.cc) list(APPEND ML_LINK_LIBRARIES sandbox2::sandbox2) # Deliberately-dependency-free sandboxee payload for the smoke test above. @@ -64,6 +65,28 @@ if(TARGET sandbox2::sandbox2 AND CMAKE_SYSTEM_NAME STREQUAL "Linux") POSITION_INDEPENDENT_CODE TRUE RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/payloads ) + + # Long-lived sandboxee for CSandboxedProcessSpawnerLifecycleTest_Linux + # (Task 4). Unlike the two payloads above, this one is launched through + # CSandboxedProcessSpawner::spawn() itself (not a hand-built Sandbox2 + # policy), which derives its filesystem policy's binDir/libDir from the + # payload's own resolved path: binDir is this payload's directory + # (payloads/) and libDir is binDir's *sibling* "lib" directory + # (${CMAKE_CURRENT_BINARY_DIR}/lib), matching the /bin + + # /lib pytorch_inference distribution layout spawn() assumes. + # That sibling directory does not otherwise exist in the unit test build + # tree; create it at configure time so PolicyBuilder::AddDirectory() never + # has to bind-mount a missing path. Empty is fine - the payload's actual + # shared-library dependencies (libc, libpthread, ld-linux) resolve via the + # fixed /lib, /lib64, /usr/lib, /usr/lib64 mounts the policy already adds. + file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/lib) + add_executable(lifecycle_signal_payload EXCLUDE_FROM_ALL + payloads/lifecycle_signal_payload.cc + ) + set_target_properties(lifecycle_signal_payload PROPERTIES + POSITION_INDEPENDENT_CODE TRUE + RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/payloads + ) endif() ml_add_test_executable(sandbox ${SRCS}) @@ -81,3 +104,10 @@ if(TARGET ml_sandbox_probe) ML_SANDBOX2_PROBE_PAYLOAD="$" ) endif() + +if(TARGET lifecycle_signal_payload) + add_dependencies(ml_test_sandbox lifecycle_signal_payload) + target_compile_definitions(ml_test_sandbox PRIVATE + ML_SANDBOX2_LIFECYCLE_PAYLOAD="$" + ) +endif() diff --git a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc new file mode 100644 index 0000000000..e439b8d907 --- /dev/null +++ b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc @@ -0,0 +1,914 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ + +// Linux-only lifecycle test for CSandboxedProcessSpawner (PR D, Task 4 of +// docs/projects/mlcpp-sandbox2-pr2873/pr-d-lifecycle.plan.md). Drives the +// spawner's four injectable seams (TPidFdOpenFn, TRegistryInsertFn, +// TMonitorLaunchFn, TAwaitResultFn - see CSandboxedProcessSpawner.h) to +// exercise fault-injection and race scenarios deterministically, but there +// is no seam that bypasses Sandbox2::RunAsync() itself: every test case +// below performs one genuine spawn() of a real, minimal, dependency-free +// payload (lifecycle_signal_payload.cc) under the real filesystem policy +// spawn() builds. The registry-insert seam receives a mutable reference to +// the spawner's *actual* internal SPidRegistry (not a copy), which every +// test below uses after that one real spawn() to fabricate/mutate further +// registry state directly - this is the only externally reachable handle to +// that private registry, since the spawner has no accessor for it and no +// constructor overload accepts a caller-supplied one. +// +// No sleep()/wall-clock polling anywhere in this file. Timing-sensitive +// races are driven either by directly exercising +// CSandboxedProcessSpawner::CCasOutcomeLatch (a pure, thread-safe type, see +// gate 5), by manually invoking a *captured* monitor-body callable on the +// calling thread instead of ever starting a background thread for it, or - +// where a genuine background thread is required (gates 7 and 8) - by a +// std::promise/future gate the test controls explicitly. The one bounded +// wait that has no other synchronisation primitive available (observing +// "still alive" - the absence of an event) uses a single poll() call with a +// timeout, never a sleep-and-recheck loop. +// +// NOT YET RUN: like CPytorchInferenceSandboxPolicyMechanismTest_Linux, this +// file has not executed on a real Linux+Sandbox2 host in this session (the +// authoring host is macOS, and the Sandbox2 headers are fetched at CMake +// configure time, not vendored in this checkout - see task-4-report.md for +// what could and could not be verified here, and for open concerns this +// design accepts). + +#include + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#ifndef ML_SANDBOX2_LIFECYCLE_PAYLOAD +#error "ML_SANDBOX2_LIFECYCLE_PAYLOAD must be defined by lib/sandbox/unittest/CMakeLists.txt" +#endif + +#include "sandboxed_api/sandbox2/result.h" +#include "sandboxed_api/sandbox2/sandbox2.h" + +// Same rationale as CSandboxedProcessSpawner_Linux.cc's identical fallback: +// the CentOS 7 CI build image's kernel headers may predate pidfd_open, but +// this is the same syscall number (434) on every architecture ml-cpp +// builds for. Used here purely for *test-owned observation* pidfds (poll() +// for exit, never a signal) - never to signal a spawner-owned child. +#ifdef __NR_pidfd_open +#define ML_TEST_NR_pidfd_open __NR_pidfd_open +#else +#define ML_TEST_NR_pidfd_open 434 +#endif + +namespace { + +using ml::sandbox::CSandboxedProcessSpawner; +using TSpawner = CSandboxedProcessSpawner; +using TPid = ml::core::CProcess::TPid; + +// --------------------------------------------------------------------- +// Descriptor-count baseline helpers (gate 6). +// --------------------------------------------------------------------- + +//! Number of open file descriptors this process currently holds, via +//! /proc/self/fd (Linux-only, fine - this whole translation unit is +//! Linux-gated). Excludes "." and "..", includes the directory fd opendir() +//! itself just opened (consistently, on both the "before" and "after" +//! snapshot, so it cancels out). +std::size_t openFdCount() { + DIR* dir = ::opendir("/proc/self/fd"); + BOOST_TEST_REQUIRE(dir != nullptr); + std::size_t count{0}; + struct dirent* entry{nullptr}; + while ((entry = ::readdir(dir)) != nullptr) { + const std::string name{entry->d_name}; + if (name != "." && name != "..") { + ++count; + } + } + ::closedir(dir); + return count; +} + +//! Applied to the whole suite: every test case must leave the process with +//! exactly the descriptors it started with (LI4, V10's cleanup assertions). +struct SFdBaselineFixture { + SFdBaselineFixture() : s_Baseline(openFdCount()) {} + ~SFdBaselineFixture() { BOOST_CHECK_EQUAL(openFdCount(), s_Baseline); } + std::size_t s_Baseline; +}; + +// --------------------------------------------------------------------- +// $TMPDIR / child-IPC-root scaffolding, matching the PR C interlock +// (validateChildIpcLaunchSpec) spawn() enforces before building a policy - +// see CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc for the same +// pattern used directly against the validation function. +// --------------------------------------------------------------------- + +int removeEntryBestEffort(const char* fpath, const struct stat*, int typeflag, struct FTW*) { + if (typeflag == FTW_DP) { + ::rmdir(fpath); + } else { + ::unlink(fpath); + } + return 0; +} + +void removeTreeBestEffort(const std::string& path) { + ::nftw(path.c_str(), removeEntryBestEffort, 16, FTW_DEPTH | FTW_PHYS); +} + +//! RAII: creates a fresh, private tmp directory and points $TMPDIR at it for +//! the lifetime of this object, so spawn()'s own trustedTmpDir +//! (getenv("TMPDIR") or "/tmp") matches exactly the root this test builds +//! its child-IPC directories under - giving each test case an isolated +//! ml-child-ipc root instead of colliding on a shared /tmp/ml-child-ipc. +class CScopedTmpDirEnv { +public: + CScopedTmpDirEnv() { + char tmpl[] = "/tmp/ml_sandbox_lifecycle_XXXXXX"; + char* dir = ::mkdtemp(tmpl); + BOOST_TEST_REQUIRE(dir != nullptr); + m_Dir = dir; + const char* previous = ::getenv("TMPDIR"); + if (previous != nullptr) { + m_PreviousTmpDir = previous; + m_HadPrevious = true; + } + ::setenv("TMPDIR", m_Dir.c_str(), 1); + } + ~CScopedTmpDirEnv() { + if (m_HadPrevious) { + ::setenv("TMPDIR", m_PreviousTmpDir.c_str(), 1); + } else { + ::unsetenv("TMPDIR"); + } + removeTreeBestEffort(m_Dir); + } + CScopedTmpDirEnv(const CScopedTmpDirEnv&) = delete; + CScopedTmpDirEnv& operator=(const CScopedTmpDirEnv&) = delete; + const std::string& dir() const { return m_Dir; } + +private: + std::string m_Dir; + std::string m_PreviousTmpDir; + bool m_HadPrevious{false}; +}; + +//! Creates $TMPDIR/ml-child-ipc/ (mode 0700), matching the layout +//! the native controller is responsible for per design.md, and returns its +//! path. +std::string makeChildIpcRoot(const std::string& trustedTmpDir, const std::string& childId) { + const std::string mlChildIpc{trustedTmpDir + "/ml-child-ipc"}; + ::mkdir(mlChildIpc.c_str(), 0700); // may already exist from an earlier case in this dir; ignore. + const std::string childRoot{mlChildIpc + "/" + childId}; + BOOST_TEST_REQUIRE(::mkdir(childRoot.c_str(), 0700) == 0); + return childRoot; +} + +//! One recognized path-bearing launch option is enough to satisfy +//! validateChildIpcLaunchSpec's s_Ok requirement (at least one present and +//! accepted) - the leaf file need not exist on disk (only its parent +//! directory is canonicalized). +std::vector childIpcArgs(const std::string& childRoot) { + return {"--input=" + childRoot + "/input.fifo"}; +} + +// --------------------------------------------------------------------- +// Test-owned pidfd observation (never signalling) - used only to answer +// "did this PID exit yet", never to terminate a spawner-owned child by +// numeric PID. +// --------------------------------------------------------------------- + +int testPidfdOpen(pid_t pid) { + return static_cast(::syscall(ML_TEST_NR_pidfd_open, pid, 0u)); +} + +//! Single bounded poll() call (not a sleep/recheck loop): returns true if +//! the pidfd became readable (the process exited) within timeoutMs, false +//! on timeout (process presumably still running). +bool pidfdReadableWithin(int pidfd, int timeoutMs) { + struct pollfd pfd {}; + pfd.fd = pidfd; + pfd.events = POLLIN; + const int rc = ::poll(&pfd, 1, timeoutMs); + return rc > 0 && (pfd.revents & POLLIN) != 0; +} + +// --------------------------------------------------------------------- +// Seam factories. Each mirrors just enough of the corresponding production +// default (see CSandboxedProcessSpawner_Linux.cc's defaultRegistryInsert +// etc.) to keep spawn() on its normal success path, while also handing the +// test a way to observe or control what happened. +// --------------------------------------------------------------------- + +//! Registry-insert seam that behaves like the production default (lock, +//! allocate the next generation, insert) and additionally captures a +//! non-owning pointer to the live SPidRegistry plus copies of the inserted +//! entry's pid/pidfd/Sandbox2 handle. Any output parameter may be nullptr +//! if the caller does not need it. The captured SPidRegistry* stays valid +//! for as long as something keeps the underlying shared_ptr +//! alive - normally the owning spawner, and after the owning spawner is +//! destroyed, only the monitor thread's own shared_ptr copy +//! (co-owned per design, LI9) - see the V8 test's own comment for the one +//! place this matters. +TSpawner::TRegistryInsertFn +capturingRegistryInsert(TSpawner::SPidRegistry** capturedRegistry, + TPid* capturedPid, + int* capturedPidFd, + std::shared_ptr* capturedSandbox) { + return [=](TSpawner::SPidRegistry& registry, TPid pid, + TSpawner::SSandboxedChild child) -> std::uint64_t { + if (capturedRegistry != nullptr) { + *capturedRegistry = ®istry; + } + if (capturedPid != nullptr) { + *capturedPid = pid; + } + if (capturedPidFd != nullptr) { + *capturedPidFd = child.s_PidFd; + } + if (capturedSandbox != nullptr) { + *capturedSandbox = child.s_Sandbox; + } + std::lock_guard lock(registry.s_Mutex); + const std::uint64_t generation{++registry.s_NextGeneration}; + child.s_Generation = generation; + child.s_State = TSpawner::EChildLifecycleState::E_Registered; + registry.s_Children[pid] = std::move(child); + return generation; + }; +} + +//! pidfd-acquisition seam that ignores the real pidfd_open syscall entirely +//! and returns a fixed, caller-chosen SPidFdAcquisitionResult - used to +//! force ENOSYS/ESRCH/EMFILE/ENFILE/"other" classifications deterministically +//! (V9), independent of what the real, presumably-modern, CI kernel would +//! actually report. +TSpawner::TPidFdOpenFn forcedPidFdOutcome(TSpawner::SPidFdAcquisitionResult toReturn, + TPid* capturedPid = nullptr) { + return [=](TPid pid) -> TSpawner::SPidFdAcquisitionResult { + if (capturedPid != nullptr) { + *capturedPid = pid; + } + return toReturn; + }; +} + +//! Monitor-launch seam that never starts a thread: it just hands the real +//! monitorBody callable spawn() built (complete with its captured +//! registry/sandbox/generation/awaitResultFn closure) back to the test via +//! capturedBody, and reports success. The test then decides exactly when - +//! or whether - to invoke it, on whatever thread it chooses (usually the +//! test's own calling thread), which is what makes gates 1, 3, 4 and 6 +//! below fully deterministic without ever starting a background thread. +TSpawner::TMonitorLaunchFn captureMonitorBodyWithoutRunning(std::function* capturedBody) { + return [capturedBody](std::function body) -> bool { + *capturedBody = std::move(body); + return true; + }; +} + +//! Monitor-launch seam that DOES start a genuine background thread (like +//! the production default), but additionally signals donePromise once the +//! monitor body - including its registry cleanup - has fully returned, so +//! a test can block deterministically until that has happened without +//! polling or joining the (deliberately detached, per LI9) thread itself. +TSpawner::TMonitorLaunchFn realMonitorLaunchWithCompletionSignal(std::promise* donePromise) { + return [donePromise](std::function body) -> bool { + std::thread([body = std::move(body), donePromise]() mutable { + body(); + donePromise->set_value(); + }).detach(); + return true; + }; +} + +//! AwaitResult seam that always delegates to the real +//! sandbox2::Sandbox2::AwaitResult() (never fabricates a sandbox2::Result - +//! its constructor is not part of any header available in this checkout, +//! see task-4-report.md) and additionally stashes a copy for the test to +//! inspect afterward, since production code only ever uses the result for +//! logging and never exposes it. +TSpawner::TAwaitResultFn capturingAwaitResult(std::shared_ptr* capturedResult) { + return [capturedResult](sandbox2::Sandbox2& sandbox) -> sandbox2::Result { + sandbox2::Result result{sandbox.AwaitResult()}; + if (capturedResult != nullptr) { + *capturedResult = std::make_shared(result); + } + return result; + }; +} + +} // namespace + +BOOST_FIXTURE_TEST_SUITE(CSandboxedProcessSpawnerLifecycleTest_Linux, SFdBaselineFixture) + +// ===================================================================== +// Gate 1 (V9): every pidfd classification. +// ===================================================================== + +//! classifyPidFdOutcome() is pure and platform-independent (no syscalls, no +//! Sandbox2 types) - exercised exhaustively here with no spawn() at all, +//! covering every classification and a representative errno for each of +//! the two failure buckets, including one genuinely "other" errno (EPERM) +//! that is neither ENOSYS nor one of the two resource-exhaustion examples +//! the brief names (ESRCH/EMFILE/ENFILE all also asserted explicitly). +BOOST_AUTO_TEST_CASE(testClassifyPidFdOutcomeExhaustive) { + using EOutcome = TSpawner::EPidFdOutcome; + BOOST_CHECK(TSpawner::classifyPidFdOutcome({3, 0}) == EOutcome::E_Acquired); + BOOST_CHECK(TSpawner::classifyPidFdOutcome({0, 0}) == EOutcome::E_Acquired); + BOOST_CHECK(TSpawner::classifyPidFdOutcome({-1, ENOSYS}) == EOutcome::E_KernelUnsupported); + BOOST_CHECK(TSpawner::classifyPidFdOutcome({-1, ESRCH}) == EOutcome::E_Failed); + BOOST_CHECK(TSpawner::classifyPidFdOutcome({-1, EMFILE}) == EOutcome::E_Failed); + BOOST_CHECK(TSpawner::classifyPidFdOutcome({-1, ENFILE}) == EOutcome::E_Failed); + BOOST_CHECK(TSpawner::classifyPidFdOutcome({-1, EPERM}) == EOutcome::E_Failed); // "other" +} + +//! Every non-success, non-ENOSYS classification must fail spawn() outright +//! (MG2/LI8) rather than register a child with an undefined termination +//! fallback. Runs each of ESRCH/EMFILE/ENFILE/EPERM through the real +//! spawn() path via the pidfd seam. +BOOST_AUTO_TEST_CASE(testSpawnFailsClosedOnEveryNonKernelUnsupportedPidfdFailure) { + const int errnosToTry[] = {ESRCH, EMFILE, ENFILE, EPERM}; + for (int forcedErrno : errnosToTry) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{ + makeChildIpcRoot(tmpEnv.dir(), std::string("case1-failed-") + std::to_string(forcedErrno))}; + + TPid capturedPid{0}; + TSpawner::TPidFdOpenFn pidFdOpen = forcedPidFdOutcome({-1, forcedErrno}, &capturedPid); + TSpawner spawner{pidFdOpen, TSpawner::TRegistryInsertFn{}, TSpawner::TMonitorLaunchFn{}, + TSpawner::TAwaitResultFn{}}; + + TPid childPid{0}; + const bool spawned = + spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid); + + BOOST_TEST_REQUIRE(spawned == false); // negative assertion + BOOST_CHECK_EQUAL(childPid, 0); + BOOST_TEST_REQUIRE(capturedPid > 0); // reached marker: the seam was invoked with a real pid + BOOST_CHECK(spawner.hasChild(capturedPid) == false); + + // Mechanism assertion: no registry entry exists to terminate, so + // there is nothing to call terminateChild() against, and no pidfd + // seam was ever consulted a second time. Cleanup assertion: the + // kill-and-reap guard ran synchronously during spawn()'s stack + // unwind (before spawn() returned), so the real sandboxee should + // already be gone - confirm via a test-owned observer pidfd, + // never a signal. + const int observerPidFd{testPidfdOpen(capturedPid)}; + BOOST_TEST_REQUIRE(observerPidFd >= 0); + BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); + ::close(observerPidFd); + } +} + +//! ENOSYS classification: terminateChild() must fall back to +//! Sandbox2::Kill() (hard-coded SIGKILL, uncatchable). There is no seam +//! around Kill() itself (unlike AwaitResult()), so this cannot be verified +//! via a spy on the call - see task-4-report.md. Instead this asserts the +//! only externally observable effect Kill()/SIGKILL and +//! pidfd_send_signal()/SIGTERM can be told apart by: the payload installs a +//! SIGTERM handler that does nothing and keeps running, so only an +//! uncatchable signal can end it - if it dies, SIGKILL (via Kill()) must +//! have been what ended it. +BOOST_AUTO_TEST_CASE(testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case1-enosys")}; + + TSpawner::SPidRegistry* registry{nullptr}; + std::shared_ptr capturedResult; + std::function monitorBody; + + TSpawner::TPidFdOpenFn pidFdOpen = forcedPidFdOutcome({-1, ENOSYS}); + TSpawner::TRegistryInsertFn insertFn = + capturingRegistryInsert(®istry, nullptr, nullptr, nullptr); + TSpawner::TMonitorLaunchFn monitorLaunch = captureMonitorBodyWithoutRunning(&monitorBody); + TSpawner::TAwaitResultFn awaitResultFn = capturingAwaitResult(&capturedResult); + + TSpawner spawner{pidFdOpen, insertFn, monitorLaunch, awaitResultFn}; + TPid childPid{0}; + BOOST_TEST_REQUIRE( + spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid)); + BOOST_TEST_REQUIRE(childPid > 0); + BOOST_CHECK(spawner.hasChild(childPid)); // positive control / reached marker + + BOOST_TEST_REQUIRE(spawner.terminateChild(childPid)); + + BOOST_TEST_REQUIRE(static_cast(monitorBody)); + monitorBody(); // real cleanup path: calls the (injected) AwaitResult exactly once. + + BOOST_TEST_REQUIRE(capturedResult != nullptr); + BOOST_CHECK(capturedResult->final_status() == sandbox2::Result::SIGNALED); // mechanism assertion + BOOST_CHECK(registry->s_Children.count(childPid) == 0); // cleanup assertion +} + +//! E_Acquired classification (the un-forced, real-kernel path on any modern +//! CI host): terminateChild() must use pidfd_send_signal(SIGTERM), which +//! the payload's handler catches and survives - the negative assertion +//! (never Kill()/SIGKILL) is that the process is demonstrably still alive +//! afterward. +BOOST_AUTO_TEST_CASE(testTerminateChildUsesPidfdSignalWhenAcquiredAndChildSurvives) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case1-acquired")}; + + TSpawner::SPidRegistry* registry{nullptr}; + int capturedPidFd{-1}; + std::shared_ptr capturedSandbox; + std::shared_ptr capturedResult; + std::function monitorBody; + + TSpawner::TRegistryInsertFn insertFn = + capturingRegistryInsert(®istry, nullptr, &capturedPidFd, &capturedSandbox); + TSpawner::TMonitorLaunchFn monitorLaunch = captureMonitorBodyWithoutRunning(&monitorBody); + TSpawner::TAwaitResultFn awaitResultFn = capturingAwaitResult(&capturedResult); + + // Left as the default (empty) seam: the real kernel's pidfd_open() is + // expected to succeed (E_Acquired) on any CI host new enough to build + // Sandbox2 at all - this is the natural, un-forced positive control. + TSpawner spawner{TSpawner::TPidFdOpenFn{}, insertFn, monitorLaunch, awaitResultFn}; + TPid childPid{0}; + BOOST_TEST_REQUIRE( + spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid)); + BOOST_TEST_REQUIRE(childPid > 0); + BOOST_TEST_REQUIRE(capturedPidFd >= 0); // confirms the real kernel classified E_Acquired + + BOOST_TEST_REQUIRE(spawner.terminateChild(childPid)); // positive control + + // Negative + mechanism assertion, single bounded poll(), not a + // sleep/recheck loop: the process must still be alive. + const int observerPidFd{testPidfdOpen(childPid)}; + BOOST_TEST_REQUIRE(observerPidFd >= 0); + BOOST_CHECK(pidfdReadableWithin(observerPidFd, 1500) == false); + ::close(observerPidFd); + + // Cleanup: the payload never exits on its own; reap it via the + // identity-bound Sandbox2 handle (never a numeric ::kill()) and run + // the real cleanup path. + BOOST_TEST_REQUIRE(capturedSandbox != nullptr); + capturedSandbox->Kill(); + BOOST_TEST_REQUIRE(static_cast(monitorBody)); + monitorBody(); + BOOST_TEST_REQUIRE(capturedResult != nullptr); + BOOST_CHECK(registry->s_Children.count(childPid) == 0); +} + +// ===================================================================== +// Gate 2 (V10, LI1, LI8): allocation/resource failure. +// ===================================================================== + +BOOST_AUTO_TEST_CASE(testRegistryInsertBadAllocKillsAndReapsCleanly) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case2a")}; + + TPid capturedPid{0}; + TSpawner::TPidFdOpenFn pidFdOpen = forcedPidFdOutcome({-1, ENOSYS}, &capturedPid); + TSpawner::TRegistryInsertFn throwingInsert = + [](TSpawner::SPidRegistry&, TPid, TSpawner::SSandboxedChild) -> std::uint64_t { + throw std::bad_alloc(); + }; + + TSpawner spawner{pidFdOpen, throwingInsert, TSpawner::TMonitorLaunchFn{}, + TSpawner::TAwaitResultFn{}}; + TPid childPid{0}; + const bool spawned = + spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid); + + BOOST_TEST_REQUIRE(spawned == false); + BOOST_CHECK_EQUAL(childPid, 0); // LI3 + BOOST_TEST_REQUIRE(capturedPid > 0); + BOOST_CHECK(spawner.hasChild(capturedPid) == false); // no registry entry + + const int observerPidFd{testPidfdOpen(capturedPid)}; + BOOST_TEST_REQUIRE(observerPidFd >= 0); + BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); // guard's Kill()+AwaitResult() already ran + ::close(observerPidFd); +} + +BOOST_AUTO_TEST_CASE(testMonitorLaunchFailureKillsAndReapsCleanly) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case2b")}; + + TPid capturedPid{0}; + TSpawner::TPidFdOpenFn pidFdOpen = forcedPidFdOutcome({-1, ENOSYS}, &capturedPid); + TSpawner::TMonitorLaunchFn alwaysFail = [](std::function) { return false; }; + + // Registry insert left at the production default - it must succeed so + // this test isolates monitor-launch failure specifically (LI8's other + // half from case 2a). + TSpawner spawner{pidFdOpen, TSpawner::TRegistryInsertFn{}, alwaysFail, + TSpawner::TAwaitResultFn{}}; + TPid childPid{0}; + const bool spawned = + spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid); + + BOOST_TEST_REQUIRE(spawned == false); + BOOST_CHECK_EQUAL(childPid, 0); + BOOST_TEST_REQUIRE(capturedPid > 0); + BOOST_CHECK(spawner.hasChild(capturedPid) == false); // eraseRegistryEntry() ran + + const int observerPidFd{testPidfdOpen(capturedPid)}; + BOOST_TEST_REQUIRE(observerPidFd >= 0); + BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); + ::close(observerPidFd); +} + +// ===================================================================== +// Gate 3 (LI6): stale generation must not erase/mutate a newer registration. +// ===================================================================== + +BOOST_AUTO_TEST_CASE(testStaleMonitorGenerationCannotEraseNewerRegistration) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case3")}; + + TSpawner::SPidRegistry* registry{nullptr}; + std::shared_ptr capturedResult; + std::function monitorBody; // closes over the ORIGINAL (stale) generation. + + TSpawner::TRegistryInsertFn insertFn = + capturingRegistryInsert(®istry, nullptr, nullptr, nullptr); + TSpawner::TMonitorLaunchFn monitorLaunch = captureMonitorBodyWithoutRunning(&monitorBody); + TSpawner::TAwaitResultFn awaitResultFn = capturingAwaitResult(&capturedResult); + + TSpawner spawner{TSpawner::TPidFdOpenFn{}, insertFn, monitorLaunch, awaitResultFn}; + TPid childPid{0}; + BOOST_TEST_REQUIRE( + spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid)); + BOOST_TEST_REQUIRE(childPid > 0); + BOOST_TEST_REQUIRE(registry != nullptr); + + std::uint64_t originalGeneration{0}; + std::uint64_t newerGeneration{0}; + std::shared_ptr sandboxHandle; + { + std::lock_guard lock(registry->s_Mutex); + const auto it = registry->s_Children.find(childPid); + BOOST_TEST_REQUIRE(it != registry->s_Children.end()); + originalGeneration = it->second.s_Generation; + sandboxHandle = it->second.s_Sandbox; + // Simulate a second, newer registration reusing the same numeric + // PID racing this call's slow first monitor - exactly what + // defaultRegistryInsert would do for a fresh insert under the same + // key (bump generation, move to E_Monitoring). + newerGeneration = ++registry->s_NextGeneration; + it->second.s_Generation = newerGeneration; + it->second.s_State = TSpawner::EChildLifecycleState::E_Monitoring; + } + BOOST_TEST_REQUIRE(sandboxHandle != nullptr); + BOOST_TEST_REQUIRE(newerGeneration != originalGeneration); + + // End the real sandboxee so the stale monitor body's (real) + // AwaitResult() call returns instead of hanging. + sandboxHandle->Kill(); + + BOOST_TEST_REQUIRE(static_cast(monitorBody)); + monitorBody(); // the STALE monitor, still closed over originalGeneration. + + BOOST_TEST_REQUIRE(capturedResult != nullptr); // reached marker: AwaitResult() did run + + // Negative + cleanup assertion (LI6): the stale monitor must not have + // erased or mutated the newer entry. + std::lock_guard lock(registry->s_Mutex); + const auto it = registry->s_Children.find(childPid); + BOOST_TEST_REQUIRE(it != registry->s_Children.end()); + BOOST_CHECK_EQUAL(it->second.s_Generation, newerGeneration); + BOOST_CHECK(it->second.s_State == TSpawner::EChildLifecycleState::E_Monitoring); +} + +// ===================================================================== +// Gate 4 (V9, LI7): a stale/expired identity must never let terminateChild() +// signal whatever unrelated process now owns a reused numeric PID. +// ===================================================================== + +//! There is no seam to force the OS's PID allocator to reuse a specific +//! number deterministically, so this fabricates the reused-PID scenario +//! directly in the registry (the only way to make it deterministic) and +//! proves terminateChild() acts on the CURRENTLY-registered identity's own +//! pidfd - never a numeric kill(pid) - by making that identity a real, +//! test-owned (never spawner-owned) forked process and observing it +//! actually receive the signal via a normal blocking waitpid(), not a +//! numeric ::kill() call anywhere in this file. +BOOST_AUTO_TEST_CASE(testTerminateChildSignalsOnlyTheCurrentlyRegisteredIdentity) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case4")}; + + TSpawner::SPidRegistry* registry{nullptr}; + std::shared_ptr capturedResultA; + std::function monitorBodyA; + + TSpawner::TRegistryInsertFn insertFn = + capturingRegistryInsert(®istry, nullptr, nullptr, nullptr); + TSpawner::TMonitorLaunchFn monitorLaunch = captureMonitorBodyWithoutRunning(&monitorBodyA); + TSpawner::TAwaitResultFn awaitResultFn = capturingAwaitResult(&capturedResultA); + + TSpawner spawner{TSpawner::TPidFdOpenFn{}, insertFn, monitorLaunch, awaitResultFn}; + TPid pidA{0}; + BOOST_TEST_REQUIRE(spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), pidA)); + BOOST_TEST_REQUIRE(pidA > 0); + + // Reap A for real - end its life and run its own monitor cleanup - so + // the registry no longer has a live entry for pidA, simulating "the + // original sandboxee already exited and was reaped". + std::shared_ptr sandboxA; + { + std::lock_guard lock(registry->s_Mutex); + const auto it = registry->s_Children.find(pidA); + BOOST_TEST_REQUIRE(it != registry->s_Children.end()); + sandboxA = it->second.s_Sandbox; + } + sandboxA->Kill(); + BOOST_TEST_REQUIRE(static_cast(monitorBodyA)); + monitorBodyA(); + BOOST_CHECK(registry->s_Children.count(pidA) == 0); + + // Fabricate "an unrelated process B now owns pidA's numeric PID": a + // real, test-owned, throwaway forked process - never spawner-owned, so + // this is test-fixture setup/teardown, not the thing LI7/the "no + // numeric ::kill() on a spawner-owned child" constraint is about. + const pid_t pidB{::fork()}; + BOOST_TEST_REQUIRE(pidB >= 0); + if (pidB == 0) { + // Plain test-fixture child: default SIGTERM disposition (terminate) + // is exactly what this test wants to observe. + for (;;) { + ::pause(); + } + } + const int pidFdB{testPidfdOpen(pidB)}; + BOOST_TEST_REQUIRE(pidFdB >= 0); + + { + std::lock_guard lock(registry->s_Mutex); + TSpawner::SSandboxedChild fabricated; + fabricated.s_State = TSpawner::EChildLifecycleState::E_Monitoring; + fabricated.s_Generation = ++registry->s_NextGeneration; + fabricated.s_PidFd = pidFdB; + fabricated.s_PidFdOutcome = TSpawner::EPidFdOutcome::E_Acquired; + fabricated.s_Outcome = std::make_shared(); + registry->s_Children[pidA] = std::move(fabricated); // same numeric key A used to own. + } + + // The call under test, addressed at the numeric PID that used to + // identify A. + BOOST_TEST_REQUIRE(spawner.terminateChild(pidA)); + + // Mechanism + negative assertion: this must have signalled B via B's + // OWN pidfd (captured at B's own registration), never a numeric + // ::kill(pidA, ...) - confirmed by actually observing B die of SIGTERM + // via a normal blocking waitpid() on the test's own direct child, not + // polling. + int status{0}; + BOOST_TEST_REQUIRE(::waitpid(pidB, &status, 0) == pidB); + BOOST_CHECK(WIFSIGNALED(status) != 0); + BOOST_CHECK_EQUAL(WTERMSIG(status), SIGTERM); + + ::close(pidFdB); +} + +// ===================================================================== +// Gate 5 (V11, MG4): timeout-vs-completion race, both interleavings, plus a +// genuine concurrent stress run - all against CCasOutcomeLatch directly (the +// sole coordination primitive design.md assigns this race to). No timeout +// caller exists anywhere in the codebase yet (an accepted, documented gap - +// see task-4-report.md), so there is nothing on the spawn()/monitorBody +// integration side to additionally exercise for this gate. +// ===================================================================== + +BOOST_AUTO_TEST_CASE(testCasOutcomeLatchResolvesExactlyOnceBothOrderings) { + using TLatch = TSpawner::CCasOutcomeLatch; + using EState = TSpawner::EOutcomeState; + { + TLatch latch; + EState completed{EState::E_Completed}; + EState timedOut{EState::E_TimedOut}; + const bool completionWon{latch.tryResolve(completed)}; + const bool timeoutWon{latch.tryResolve(timedOut)}; + BOOST_CHECK(completionWon); + BOOST_CHECK(timeoutWon == false); + BOOST_CHECK(timedOut == EState::E_Completed); // loser observes the winner's value + BOOST_CHECK(latch.load() == EState::E_Completed); + } + { + TLatch latch; + EState timedOut{EState::E_TimedOut}; + EState completed{EState::E_Completed}; + const bool timeoutWon{latch.tryResolve(timedOut)}; + const bool completionWon{latch.tryResolve(completed)}; + BOOST_CHECK(timeoutWon); + BOOST_CHECK(completionWon == false); + BOOST_CHECK(completed == EState::E_TimedOut); + BOOST_CHECK(latch.load() == EState::E_TimedOut); + } +} + +BOOST_AUTO_TEST_CASE(testCasOutcomeLatchUnderRealConcurrencyResolvesExactlyOnce) { + using TLatch = TSpawner::CCasOutcomeLatch; + using EState = TSpawner::EOutcomeState; + for (int trial = 0; trial < 200; ++trial) { + TLatch latch; + std::promise startPromise; + std::shared_future start{startPromise.get_future()}; + std::atomic completedWins{0}; + std::atomic timedOutWins{0}; + + auto race = [&](EState desiredInitial, std::atomic& winCounter) { + start.wait(); // test-controlled synchronization point, never sleep(). + EState desired{desiredInitial}; + if (latch.tryResolve(desired)) { + ++winCounter; + } + }; + std::thread t1(race, EState::E_Completed, std::ref(completedWins)); + std::thread t2(race, EState::E_TimedOut, std::ref(timedOutWins)); + startPromise.set_value(); + t1.join(); + t2.join(); + + // Exactly one side ever wins, regardless of scheduling order - the + // property MG4/V11 exist to guarantee. + BOOST_CHECK_EQUAL(completedWins.load() + timedOutWins.load(), 1); + } +} + +// ===================================================================== +// Gate 6 (LI4/V10 cleanup): descriptor baseline. SFdBaselineFixture (above) +// already asserts this after every case in this suite; this case names it +// explicitly against one concrete spawn/terminate/cleanup cycle. +// ===================================================================== + +BOOST_AUTO_TEST_CASE(testDescriptorCountReturnsToBaselineAfterSpawnTerminateCleanup) { + const std::size_t before{openFdCount()}; + + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case6")}; + + std::shared_ptr capturedResult; + std::function monitorBody; + TSpawner::TPidFdOpenFn pidFdOpen = forcedPidFdOutcome({-1, ENOSYS}); // avoids the SIGTERM-survives hang. + TSpawner::TMonitorLaunchFn monitorLaunch = captureMonitorBodyWithoutRunning(&monitorBody); + TSpawner::TAwaitResultFn awaitResultFn = capturingAwaitResult(&capturedResult); + + TSpawner spawner{pidFdOpen, TSpawner::TRegistryInsertFn{}, monitorLaunch, awaitResultFn}; + TPid childPid{0}; + BOOST_TEST_REQUIRE( + spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid)); + BOOST_TEST_REQUIRE(childPid > 0); + BOOST_TEST_REQUIRE(spawner.terminateChild(childPid)); + BOOST_TEST_REQUIRE(static_cast(monitorBody)); + monitorBody(); // real cleanup path: closes the pidfd, erases the entry. + + BOOST_CHECK_EQUAL(openFdCount(), before); +} + +// ===================================================================== +// Gate 7: controller-exit orphan behavior - spawner-side half ONLY. +// +// RULING (per the task brief): the orphan-CLEANUP half (does an abandoned +// sandboxee eventually get reaped by something else in the system) is out +// of scope for this unit test and is NOT claimed as covered here - see +// task-4-report.md, which records it as an MG6 accepted-risk candidate for +// the epic/PR-E to close. +// ===================================================================== + +BOOST_AUTO_TEST_CASE(testDestructorDoesNotJoinAndReturnsUnderOneSecond) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case7")}; + + std::shared_ptr capturedSandbox; + TSpawner::TRegistryInsertFn insertFn = + capturingRegistryInsert(nullptr, nullptr, nullptr, &capturedSandbox); + + // Real monitor launch (a genuine background thread, like the production + // default) AND real AwaitResult (left as the default, empty seam): a + // genuine background thread is blocked in the real AwaitResult() on a + // genuinely live, never-self-exiting child when the spawner below is + // destroyed - this is LI9/V8's actual scenario, not a simulation of it. + // Unlike the plain default monitor-launch seam, this variant also + // signals monitorDonePromise once that thread's cleanup has fully run, + // which this test needs afterward to deterministically avoid racing + // the suite-wide SFdBaselineFixture's end-of-case descriptor count + // (the real cleanup closes the child's pidfd on that same thread, + // asynchronously with respect to this test case's own control flow). + std::promise monitorDonePromise; + std::future monitorDone{monitorDonePromise.get_future()}; + TSpawner::TMonitorLaunchFn monitorLaunch = realMonitorLaunchWithCompletionSignal(&monitorDonePromise); + + auto spawner = std::make_unique(TSpawner::TPidFdOpenFn{}, insertFn, monitorLaunch, + TSpawner::TAwaitResultFn{}); + TPid childPid{0}; + BOOST_TEST_REQUIRE( + spawner->spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid)); + BOOST_TEST_REQUIRE(childPid > 0); + BOOST_CHECK(spawner->hasChild(childPid)); // reached marker: genuinely running + + const auto start = std::chrono::steady_clock::now(); + spawner.reset(); // ~CSandboxedProcessSpawner() with a live child and a real + // monitor thread genuinely blocked in AwaitResult() on it. + const auto elapsed = std::chrono::steady_clock::now() - start; + + BOOST_CHECK(elapsed < std::chrono::seconds(1)); // V8's explicit bound + + // Test hygiene, not part of the gate 7 assertion itself: reap the + // still-running sandboxee via its identity-bound Sandbox2 handle + // (never a numeric ::kill()) so this test process doesn't leave a + // permanently-blocked monitor thread behind, then block (no polling) + // until that thread's own cleanup has fully finished, so the next + // test case's fd-baseline snapshot cannot race this one's cleanup. + BOOST_TEST_REQUIRE(capturedSandbox != nullptr); + capturedSandbox->Kill(); + monitorDone.wait(); +} + +// ===================================================================== +// Gate 8 (V8): monitor outlives spawner - destroy the spawner while a +// monitor thread is genuinely still running (blocked on a test-controlled +// gate), release the gate, assert its cleanup runs safely against the +// registry it co-owns via shared_ptr. +// ===================================================================== + +BOOST_AUTO_TEST_CASE(testMonitorCleanupRunsSafelyAfterSpawnerDestruction) { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case8")}; + + std::promise gatePromise; + std::shared_future gate{gatePromise.get_future()}; + std::promise monitorDonePromise; + std::future monitorDone{monitorDonePromise.get_future()}; + + TSpawner::TAwaitResultFn awaitResultFn = [gate](sandbox2::Sandbox2& sandbox) -> sandbox2::Result { + gate.wait(); // test-controlled synchronization point - never sleep(). + return sandbox.AwaitResult(); + }; + TSpawner::TMonitorLaunchFn monitorLaunch = realMonitorLaunchWithCompletionSignal(&monitorDonePromise); + + std::shared_ptr capturedSandbox; + TSpawner::SPidRegistry* registryRaw{nullptr}; + TSpawner::TRegistryInsertFn insertFn = + capturingRegistryInsert(®istryRaw, nullptr, nullptr, &capturedSandbox); + + TPid childPid{0}; + { + TSpawner spawner{TSpawner::TPidFdOpenFn{}, insertFn, monitorLaunch, awaitResultFn}; + BOOST_TEST_REQUIRE( + spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid)); + BOOST_TEST_REQUIRE(childPid > 0); + BOOST_CHECK(spawner.hasChild(childPid)); // reached marker + } // spawner destroyed here; the monitor thread is still genuinely blocked on `gate`. + + // registryRaw is only safe to dereference now because the monitor + // thread's own shared_ptr copy (captured in monitorBody's + // closure by spawn(), per design) keeps the same object alive - the + // spawner's own shared_ptr, which is what made this pointer valid + // originally, is gone. That co-ownership is exactly the property this + // test exists to exercise. + + // Release the gate and make the sandboxee actually exit, so the + // now-unblocked real AwaitResult() call inside the monitor thread can + // return - identity-bound cleanup via the co-owned Sandbox2 handle, + // never a numeric ::kill(). + BOOST_TEST_REQUIRE(capturedSandbox != nullptr); + gatePromise.set_value(); + capturedSandbox->Kill(); + + // Block (no polling) until the monitor thread's entire body - including + // its registry cleanup - has fully returned. + monitorDone.wait(); + + // V8/no-crash assertion: reaching this line at all, after the spawner + // is long gone, is the primary proof. The check below additionally + // confirms the monitor's registry erase actually ran. + BOOST_TEST_REQUIRE(registryRaw != nullptr); + std::lock_guard lock(registryRaw->s_Mutex); + BOOST_CHECK(registryRaw->s_Children.count(childPid) == 0); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/lib/sandbox/unittest/payloads/lifecycle_signal_payload.cc b/lib/sandbox/unittest/payloads/lifecycle_signal_payload.cc new file mode 100644 index 0000000000..b317e89d8a --- /dev/null +++ b/lib/sandbox/unittest/payloads/lifecycle_signal_payload.cc @@ -0,0 +1,72 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ + +// Deliberately dependency-free sandboxee for +// CSandboxedProcessSpawnerLifecycleTest_Linux (PR D, Task 4). Unlike +// sandbox_smoke_payload.cc (exits immediately) this payload stays alive +// indefinitely so the lifecycle test can drive CSandboxedProcessSpawner:: +// terminateChild() against a genuinely live child and distinguish its two +// termination mechanisms by observable effect: +// +// - pidfd_send_signal(SIGTERM) (the E_Acquired branch) is a *request*: this +// payload installs a SIGTERM handler that does nothing and returns, so +// the process stays alive and the test can observe "still running". +// - Sandbox2::Kill() (the E_KernelUnsupported branch) hard-codes SIGKILL, +// which cannot be caught or ignored, so the process actually exits. +// +// Only syscalls in seccomp::pytorch_inference::legacyBpfAllowedSyscalls() +// are available under the real spawn() policy - notably __NR_pause is NOT +// in that allowlist, so this cannot simply call pause() in a loop. Blocking +// on FUTEX_WAIT against a private, never-signalled word uses only +// __NR_futex (allowed) and is interrupted (EINTR) by the caught SIGTERM, +// after which the loop just re-enters the wait; the only way to actually +// terminate this process is an uncatchable signal (SIGKILL). +// +// No ml-cpp library dependencies, no policy of its own - same rationale as +// sandbox_smoke_payload.cc and ml_sandbox_probe.cc. + +#include +#include +#include +#include +#include + +namespace { + +std::atomic gFutexWord{0}; + +void ignoreSigterm(int /* signum */) { + // Deliberately empty: catching (rather than ignoring via SIG_IGN) means + // the blocking futex(2) call below observes EINTR and this handler + // itself is proof the process is still alive and processing signals + // normally - SIG_IGN would make that indistinguishable from "never + // received the signal at all". +} + +} // namespace + +int main() { + struct sigaction sa {}; + sa.sa_handler = ignoreSigterm; + ::sigemptyset(&sa.sa_mask); + sa.sa_flags = 0; + ::sigaction(SIGTERM, &sa, nullptr); + + for (;;) { + // FUTEX_WAIT (0): block while *reinterpret_cast(&gFutexWord) == + // 0, which it always is - nothing ever calls FUTEX_WAKE on this + // word. Returns on a spurious wake, a real wake (never happens + // here), or EINTR from the caught SIGTERM; any of those just loops + // back into another wait. + ::syscall(SYS_futex, reinterpret_cast(&gFutexWord), 0, 0, nullptr); + } + return 0; +} From e540295defdb060b087f19418cbce53295352e5e Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:19:45 +0200 Subject: [PATCH 10/15] [ML] Strengthen ENOSYS-fallback SIGKILL assertion and bound its wait testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd now asserts reason_code() == SIGKILL in addition to final_status() == SIGNALED, so a regression sending a different signal no longer passes silently. The monitorBody() call that drives AwaitResult() is now run on a bounded background thread (join on success, detach on timeout) since production AwaitResult() has no wall-clock limit and a Kill()-regressed-to-no-op would otherwise hang indefinitely instead of failing fast. --- ...dboxedProcessSpawnerLifecycleTest_Linux.cc | 55 +++++++++++++++++-- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc index e439b8d907..2126751b25 100644 --- a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc +++ b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc @@ -406,14 +406,27 @@ BOOST_AUTO_TEST_CASE(testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "case1-enosys")}; TSpawner::SPidRegistry* registry{nullptr}; - std::shared_ptr capturedResult; std::function monitorBody; + // Heap-owned box for the captured sandbox2::Result, not a plain stack + // local: monitorBody() below is run with a bounded wait (fixing the + // review finding that a terminateChild() regression to a no-op would + // otherwise hang this call forever, since production AwaitResult() has + // no wall-clock bound of its own - see spawn()'s + // set_walltime_limit(absl::ZeroDuration()) in + // CSandboxedProcessSpawner_Linux.cc, and there is no seam to override it + // for just this test). If the wait times out, the still-running + // background thread is detached rather than joined (so this test case, + // and the whole suite, fails fast instead of hanging) - anything that + // thread can still touch after this function returns must therefore + // live on the heap, not on this stack frame. + auto capturedResult = std::make_shared>(); + TSpawner::TPidFdOpenFn pidFdOpen = forcedPidFdOutcome({-1, ENOSYS}); TSpawner::TRegistryInsertFn insertFn = capturingRegistryInsert(®istry, nullptr, nullptr, nullptr); TSpawner::TMonitorLaunchFn monitorLaunch = captureMonitorBodyWithoutRunning(&monitorBody); - TSpawner::TAwaitResultFn awaitResultFn = capturingAwaitResult(&capturedResult); + TSpawner::TAwaitResultFn awaitResultFn = capturingAwaitResult(capturedResult.get()); TSpawner spawner{pidFdOpen, insertFn, monitorLaunch, awaitResultFn}; TPid childPid{0}; @@ -425,10 +438,42 @@ BOOST_AUTO_TEST_CASE(testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) BOOST_TEST_REQUIRE(spawner.terminateChild(childPid)); BOOST_TEST_REQUIRE(static_cast(monitorBody)); - monitorBody(); // real cleanup path: calls the (injected) AwaitResult exactly once. - BOOST_TEST_REQUIRE(capturedResult != nullptr); - BOOST_CHECK(capturedResult->final_status() == sandbox2::Result::SIGNALED); // mechanism assertion + // Run the real cleanup path (calls the injected AwaitResult() exactly + // once) on a separate thread, bounded by a std::promise/future wait - + // same synchronization primitive gate 8 already uses in this file, just + // with a timeout instead of an unconditional wait(), since here nothing + // else in the test independently guarantees the payload will ever die. + auto monitorDonePromise = std::make_shared>(); + std::future monitorDoneFuture{monitorDonePromise->get_future()}; + std::thread monitorThread([body = monitorBody, monitorDonePromise]() mutable { + body(); + monitorDonePromise->set_value(); + }); + const std::future_status waitStatus{monitorDoneFuture.wait_for(std::chrono::seconds(5))}; + if (waitStatus == std::future_status::ready) { + monitorThread.join(); + } else { + // Regression path: terminateChild()'s E_KernelUnsupported branch + // apparently didn't actually end the payload (e.g. sent the wrong + // signal, or Kill() regressed to a no-op), so the injected + // AwaitResult() is still blocked with no bound of its own. Detach + // instead of join() so this test fails on the assertion below + // within a few seconds rather than hanging indefinitely - every + // object the thread can still reach (capturedResult, the promise, + // and monitorBody's own closure, copied above) is heap-owned via + // shared_ptr/std::function-by-value, so it stays valid even though + // this function is about to return out from under it. + monitorThread.detach(); + } + // Fails fast (instead of hanging) if terminateChild() regressed to + // never actually killing the payload: a timeout here IS the failure, + // not a hang. + BOOST_TEST_REQUIRE(waitStatus == std::future_status::ready); + + BOOST_TEST_REQUIRE(*capturedResult != nullptr); + BOOST_CHECK((*capturedResult)->final_status() == sandbox2::Result::SIGNALED); // mechanism: some signal + BOOST_CHECK((*capturedResult)->reason_code() == SIGKILL); // mechanism: specifically SIGKILL, i.e. Kill() BOOST_CHECK(registry->s_Children.count(childPid) == 0); // cleanup assertion } From 47a8dd6b3ea9fc10dce4dad8e5553a1855ed36ae Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:24:56 +0200 Subject: [PATCH 11/15] [ML] Fix UAF on timeout-then-detach path in pidfd-ENOSYS lifecycle test capturedResult's outer shared_ptr was not captured by the detached monitor thread's lambda, only its raw pointer traveled via awaitResultFn's closure. On the timeout/detach regression path, the test case's stack unwind (BOOST_TEST_REQUIRE failure) freed the heap-boxed inner shared_ptr while the still-running detached thread held a dangling raw pointer into it, ready to write through it once AwaitResult() unblocked. Capture capturedResult by value in the detached thread's own lambda so its copy keeps the object alive for as long as the thread might still run, independent of the test case's lifetime. --- ...dboxedProcessSpawnerLifecycleTest_Linux.cc | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc index 2126751b25..53ac1036a6 100644 --- a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc +++ b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc @@ -446,7 +446,7 @@ BOOST_AUTO_TEST_CASE(testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) // else in the test independently guarantees the payload will ever die. auto monitorDonePromise = std::make_shared>(); std::future monitorDoneFuture{monitorDonePromise->get_future()}; - std::thread monitorThread([body = monitorBody, monitorDonePromise]() mutable { + std::thread monitorThread([body = monitorBody, monitorDonePromise, capturedResult]() mutable { body(); monitorDonePromise->set_value(); }); @@ -460,10 +460,21 @@ BOOST_AUTO_TEST_CASE(testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) // AwaitResult() is still blocked with no bound of its own. Detach // instead of join() so this test fails on the assertion below // within a few seconds rather than hanging indefinitely - every - // object the thread can still reach (capturedResult, the promise, - // and monitorBody's own closure, copied above) is heap-owned via - // shared_ptr/std::function-by-value, so it stays valid even though - // this function is about to return out from under it. + // object the thread can still reach (monitorDonePromise, and + // monitorBody's own closure, copied above) is heap-owned via + // shared_ptr/std::function-by-value. Critically, capturedResult + // (the outer shared_ptr) is ALSO captured by value into this + // lambda: capturingAwaitResult() only holds a raw pointer into the + // heap-allocated inner shared_ptr, baked into + // monitorBody/awaitResultFn's closure by value, so without a + // shared_ptr copy of capturedResult riding along in this thread's + // own capture list, BOOST_TEST_REQUIRE below failing/unwinding this + // stack frame would drop the last reference and free the object + // out from under the still-running detached thread - a + // use-after-free once the real AwaitResult() unblocks and writes + // through that raw pointer. Capturing capturedResult here keeps it + // alive for as long as the detached thread might still run, + // independent of this function's own lifetime. monitorThread.detach(); } // Fails fast (instead of hanging) if terminateChild() regressed to From b42ee960e01400a32788f25ce211aa33fe290a1e Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:41:01 +0200 Subject: [PATCH 12/15] [ML] Fix pidfd recycled-descriptor race, generation-blind rollback, dead lifecycle states Final review fix wave for PR D (Sandbox2 lifecycle rebuild), all 6 findings in one pass: - C1/I3: terminateChild() now performs pidfd_send_signal() for the E_Acquired branch while still holding s_Mutex, closing the race against monitorBody's completion handler closing/recycling the same pidfd underneath a delayed signal. rollBackState() now matches on both s_Generation and s_State, so a stale rollback cannot clobber a newer registration that reused the same numeric PID. - I1: wire up E_Monitoring (after monitor handoff is confirmed in spawn()) and E_Reaped (immediately before the registry erase in monitorBody), generation-matched under the lock. - I2: reset childPid to 0 immediately after capturing sandboxPid, and only set it to the live PID at the very end of spawn(), so an uncaught throw between guard-arm and the first try/catch can no longer leave childPid non-zero on a false/throw exit. - I4: lifecycle test assertions that a fresh pidfd_open() succeeds on an already-killed-and-reaped PID now also accept ESRCH (fully reaped) as proof of cleanup, in the 3 affected test cases. - I5: realMonitorLaunchWithCompletionSignal destroys the monitor closure (and its captured Sandbox2 handle) before signaling completion, so a waiting test cannot observe "done" while fds may still be open. - I6: add a BOOST_GLOBAL_FIXTURE that warms up Sandbox2's forkserver before any per-case fd-baseline snapshot is taken, removing the test-ordering dependency shared with gate 4's fork(). --- lib/sandbox/CSandboxedProcessSpawner_Linux.cc | 117 +++++++++++++----- ...dboxedProcessSpawnerLifecycleTest_Linux.cc | 105 ++++++++++++++-- 2 files changed, 179 insertions(+), 43 deletions(-) diff --git a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc index 72d4cf822f..92c8de2b3a 100644 --- a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc +++ b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc @@ -455,6 +455,19 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, } const core::CProcess::TPid sandboxPid{childPid}; + // I2: default the caller's out-parameter back to 0 for the entire span + // between capturing sandboxPid and confirmed success (the final `return + // true` below). Several calls in that span (e.g. + // std::make_shared() a few lines down) can throw + // std::bad_alloc *before* the try/catch blocks further down start, and + // an exception there propagates straight out of spawn() uncaught (the + // kill-and-reap guard's destructor still cleans up the sandboxee + // correctly during unwind). Without this, that throw-only exit would + // leave the caller's childPid at the live PID even though spawn() never + // returned true. Every explicit `return false` below already sets + // childPid = 0 too; this makes 0 the default regardless of whether a + // given exit is a return or an uncaught throw. + childPid = 0; // E_IdentityCaptured (LI1): arm the kill-and-reap guard now that the // sandboxee is actually running. The guard takes its own shared_ptr @@ -565,6 +578,14 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, completionWonRace = it->second.s_Outcome->tryResolve(desired); } if (completionWonRace) { + // I1: record E_Reaped immediately before erasing the + // entry, so a future accessor reading state via the + // lock during this brief window would see E_Reaped + // rather than a stale E_Monitoring. Defensive/ + // documentation-only today - nothing reads it before + // the erase below - but matches the state machine's + // declared intent. + it->second.s_State = EChildLifecycleState::E_Reaped; closePidFdIfOpen(it->second.s_PidFd); registry->s_Children.erase(it); } @@ -601,8 +622,25 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, // removing the registry entry. Disarm - the guard must not also reap. killAndReapGuard.disarm(); + // I1: record the E_Monitoring transition explicitly, generation-matched + // and under the lock, now that handoff is confirmed. Without this, + // E_Monitoring was declared in the state machine but never actually + // assigned anywhere, so the "explicit state machine, no state skipped" + // claim was not true in the code, and a future timeout caller (PR E) + // would have nothing correct to branch on. + { + std::lock_guard lock(m_PidRegistry->s_Mutex); + const auto it = m_PidRegistry->s_Children.find(sandboxPid); + if (it != m_PidRegistry->s_Children.end() && it->second.s_Generation == generation) { + it->second.s_State = EChildLifecycleState::E_Monitoring; + } + } + LOG_INFO(<< "Spawned sandboxed process " << processPath << " with PID " << childPid); + // I2: only now, with registration and monitor handoff both confirmed, is + // it safe to hand the live PID back to the caller. + childPid = sandboxPid; return true; #else // !SANDBOX2_AVAILABLE @@ -623,9 +661,9 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { // - a graceful termination *request* - for E_Acquired, or Sandbox2::Kill() // (SIGKILL via the owned monitor) for E_KernelUnsupported. No numeric // kill(pid) fallback exists anywhere in this file. - int pidFdToSignal{-1}; std::shared_ptr sandboxToKill; EChildLifecycleState previousState{EChildLifecycleState::E_Failed}; + std::uint64_t capturedGeneration{0}; { std::lock_guard lock(m_PidRegistry->s_Mutex); const auto it = m_PidRegistry->s_Children.find(pid); @@ -636,8 +674,14 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { } SSandboxedChild& child{it->second}; previousState = child.s_State; + // C1/I3: capture the generation now, under the same lock acquisition + // that decides the termination mechanism, so a failure below can + // roll back state only if it still identifies the SAME registration + // (not a newer one that reused this numeric PID after this entry + // was reaped and erased). + capturedGeneration = child.s_Generation; switch (child.s_PidFdOutcome) { - case EPidFdOutcome::E_Acquired: + case EPidFdOutcome::E_Acquired: { if (child.s_PidFd < 0) { // Logic error (should be structurally unreachable given // spawn()'s fail-closed registration in this task): a @@ -647,8 +691,29 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { << " classified E_Acquired but holds no pidfd"); return false; } - pidFdToSignal = child.s_PidFd; - break; + // C1: send the request WHILE STILL HOLDING s_Mutex. + // pidfd_send_signal is a non-blocking syscall, so this is safe, + // and it is the only way to close the race against monitorBody's + // Sandbox2-completion handler, which also takes this same lock + // before closing this exact pidfd and erasing the registry entry + // (it does not check s_State). Previously the syscall ran + // outside the lock: a snapshot-then-signal window let + // monitorBody close the pidfd and the kernel recycle that + // descriptor number for an unrelated spawn() in between, so a + // delayed pidfd_send_signal here could hit the wrong process + // (the LI7 "identity, not recycled descriptor" hazard, one layer + // below the already-fixed numeric-PID case). + if (::syscall(ML_NR_pidfd_send_signal, child.s_PidFd, SIGTERM, nullptr, 0u) != 0) { + LOG_ERROR(<< "pidfd_send_signal(SIGTERM) failed for sandboxed child PID " << pid + << ": " << ::strerror(errno)); + // No state transition happened on this path (the state is + // only advanced below, on success), so there is nothing to + // roll back. + return false; + } + child.s_State = EChildLifecycleState::E_TerminationRequested; + return true; + } case EPidFdOutcome::E_KernelUnsupported: if (!child.s_Sandbox) { // Same reasoning as above: E_KernelUnsupported without a @@ -659,6 +724,7 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { return false; } sandboxToKill = child.s_Sandbox; + child.s_State = EChildLifecycleState::E_TerminationRequested; break; case EPidFdOutcome::E_Failed: default: @@ -671,46 +737,31 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { << static_cast(child.s_PidFdOutcome) << ')'); return false; } - child.s_State = EChildLifecycleState::E_TerminationRequested; } + // Only the E_KernelUnsupported/Sandbox2::Kill() path reaches here - the + // E_Acquired/pidfd path above already returned from inside the locked + // block (C1). sandboxToKill is identity-bound via the owned shared_ptr, + // so - unlike the pidfd branch - it remains safe to call Kill() outside + // s_Mutex, unchanged from before this fix wave. + // Rolls the registry entry's s_State back to what it was before this - // call optimistically set it to E_TerminationRequested, but only if - // nothing else has moved the state on in the meantime (e.g. a - // concurrent reap). Called on the failure paths below, after the actual - // pidfd_send_signal()/Kill() call - re-acquires the lock briefly; the - // call itself still happens outside the lock, unchanged. - const auto rollBackState = [this, pid, previousState]() { + // call optimistically set it to E_TerminationRequested, but only if the + // entry still matches BOTH the captured generation AND the expected + // in-flight state (I3) - guards against a stale rollback clobbering a + // different (newer) registration that reused this numeric PID after the + // original entry was reaped and erased, and that newer registration + // happens to also currently be E_TerminationRequested. + const auto rollBackState = [this, pid, previousState, capturedGeneration]() { std::lock_guard lock(m_PidRegistry->s_Mutex); const auto it = m_PidRegistry->s_Children.find(pid); if (it != m_PidRegistry->s_Children.end() && + it->second.s_Generation == capturedGeneration && it->second.s_State == EChildLifecycleState::E_TerminationRequested) { it->second.s_State = previousState; } }; - if (pidFdToSignal >= 0) { - if (::syscall(ML_NR_pidfd_send_signal, pidFdToSignal, SIGTERM, nullptr, 0u) != 0) { - LOG_ERROR(<< "pidfd_send_signal(SIGTERM) failed for sandboxed child PID " << pid - << ": " << ::strerror(errno)); - rollBackState(); - return false; - } - return true; - } - - // sandboxToKill is only ever set on the E_KernelUnsupported branch - // above; pidFdToSignal >= 0 is only ever set on the E_Acquired branch. - // Exactly one of the two is populated by the switch, so reaching here - // with neither would itself be a logic error - defensively refuse - // rather than silently no-op. - if (!sandboxToKill) { - LOG_ERROR(<< "Logic error: terminateChild() for PID " << pid - << " resolved neither a pidfd nor a Sandbox2 handle to kill"); - rollBackState(); - return false; - } - try { // Locked design decision (design.md): MonitorBase::Kill() takes no // signal parameter and hard-codes SIGKILL - this is the ENOSYS diff --git a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc index 53ac1036a6..9335e3a775 100644 --- a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc +++ b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc @@ -305,7 +305,18 @@ TSpawner::TMonitorLaunchFn captureMonitorBodyWithoutRunning(std::function* donePromise) { return [donePromise](std::function body) -> bool { std::thread([body = std::move(body), donePromise]() mutable { - body(); + // I5: destroy the closure - and therefore its captured + // shared_ptr - BEFORE signaling completion. + // `body` is a member of this thread lambda's own closure, so + // without this it is not destroyed until the thread function + // returns, which happens AFTER set_value() below; a test + // blocked on donePromise's future could then observe "done" + // while the sandbox's fds may still be open, racing gates 7/8's + // fd-baseline check. + { + auto b = std::move(body); + b(); + } donePromise->set_value(); }).detach(); return true; @@ -328,8 +339,64 @@ TSpawner::TAwaitResultFn capturingAwaitResult(std::shared_ptr* }; } +// --------------------------------------------------------------------- +// I6: forkserver / fork() warm-up, run once before ANY per-case fixture. +// --------------------------------------------------------------------- + +//! Sandbox2's global forkserver is created lazily on the first RunAsync() +//! anywhere in this process, and holds its own comms descriptors for the +//! rest of the process's lifetime. SFdBaselineFixture (above) snapshots the +//! fd count before each case's first spawn(); if this test binary/suite +//! ever runs with this suite as the FIRST thing to spawn anything in the +//! whole process (e.g. via `--run_test=` filtering, or a future link-order +//! change), the first case's fd-baseline check would see the forkserver's +//! descriptors appear mid-case and spuriously fail. This is the same root +//! cause as the "gate 4 fork() implicit test-ordering dependency" concern +//! (gate 4 also forks - see testTerminateChildSignalsOnlyTheCurrentlyRegisteredIdentity +//! - and pays the same one-time lazy-init cost the first time anything in +//! this binary spawns or forks) - fixed once, here, for both. +//! +//! A BOOST_GLOBAL_FIXTURE runs once for the whole test module, before any +//! test case (and therefore before any per-case SFdBaselineFixture +//! construction) regardless of `--run_test=` filtering or link order, so +//! placing the warm-up here - rather than relying on some earlier test +//! case in this or another suite having already run - makes the forkserver +//! guaranteed already-started by the time any case's baseline is captured. +struct SForkserverWarmupFixture { + SForkserverWarmupFixture() { + CScopedTmpDirEnv tmpEnv; + const std::string childRoot{makeChildIpcRoot(tmpEnv.dir(), "forkserver-warmup")}; + + std::shared_ptr capturedResult; + std::function monitorBody; + // ENOSYS forces the Sandbox2::Kill() termination path below (rather + // than requiring a real pidfd_send_signal/SIGTERM round-trip), + // keeping this warm-up simple and unconditional regardless of what + // the real kernel supports. + TSpawner::TPidFdOpenFn pidFdOpen = forcedPidFdOutcome({-1, ENOSYS}); + TSpawner::TMonitorLaunchFn monitorLaunch = captureMonitorBodyWithoutRunning(&monitorBody); + TSpawner::TAwaitResultFn awaitResultFn = capturingAwaitResult(&capturedResult); + + TSpawner spawner{pidFdOpen, TSpawner::TRegistryInsertFn{}, monitorLaunch, awaitResultFn}; + TPid childPid{0}; + // Best-effort: if this somehow fails, every real test case's own + // spawn() will surface the underlying problem on its own merits - + // this warm-up only exists to make the FIRST case's fd baseline + // deterministic, not to assert anything itself. + if (spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid) && + childPid > 0) { + spawner.terminateChild(childPid); + if (monitorBody) { + monitorBody(); // real cleanup path: closes the pidfd, erases the entry. + } + } + } +}; + } // namespace +BOOST_GLOBAL_FIXTURE(SForkserverWarmupFixture); + BOOST_FIXTURE_TEST_SUITE(CSandboxedProcessSpawnerLifecycleTest_Linux, SFdBaselineFixture) // ===================================================================== @@ -385,10 +452,18 @@ BOOST_AUTO_TEST_CASE(testSpawnFailsClosedOnEveryNonKernelUnsupportedPidfdFailure // unwind (before spawn() returned), so the real sandboxee should // already be gone - confirm via a test-owned observer pidfd, // never a signal. + // I4: a fresh pidfd_open() on a PID the kill-and-reap guard has + // already Kill()ed and AwaitResult()ed can legitimately fail with + // ESRCH (fully reaped already - the common case) rather than + // succeed, so accept both outcomes as proof of cleanup instead of + // requiring a live pidfd. const int observerPidFd{testPidfdOpen(capturedPid)}; - BOOST_TEST_REQUIRE(observerPidFd >= 0); - BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); - ::close(observerPidFd); + if (observerPidFd < 0) { + BOOST_CHECK_EQUAL(errno, ESRCH); // already fully reaped - this IS proof of cleanup + } else { + BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); + ::close(observerPidFd); + } } } @@ -564,10 +639,15 @@ BOOST_AUTO_TEST_CASE(testRegistryInsertBadAllocKillsAndReapsCleanly) { BOOST_TEST_REQUIRE(capturedPid > 0); BOOST_CHECK(spawner.hasChild(capturedPid) == false); // no registry entry + // I4: accept either ESRCH (already fully reaped) or a live-but-exited + // pidfd as proof the guard's Kill()+AwaitResult() already ran. const int observerPidFd{testPidfdOpen(capturedPid)}; - BOOST_TEST_REQUIRE(observerPidFd >= 0); - BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); // guard's Kill()+AwaitResult() already ran - ::close(observerPidFd); + if (observerPidFd < 0) { + BOOST_CHECK_EQUAL(errno, ESRCH); // already fully reaped - this IS proof of cleanup + } else { + BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); + ::close(observerPidFd); + } } BOOST_AUTO_TEST_CASE(testMonitorLaunchFailureKillsAndReapsCleanly) { @@ -592,10 +672,15 @@ BOOST_AUTO_TEST_CASE(testMonitorLaunchFailureKillsAndReapsCleanly) { BOOST_TEST_REQUIRE(capturedPid > 0); BOOST_CHECK(spawner.hasChild(capturedPid) == false); // eraseRegistryEntry() ran + // I4: accept either ESRCH (already fully reaped) or a live-but-exited + // pidfd as proof eraseRegistryEntry()/the guard's cleanup already ran. const int observerPidFd{testPidfdOpen(capturedPid)}; - BOOST_TEST_REQUIRE(observerPidFd >= 0); - BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); - ::close(observerPidFd); + if (observerPidFd < 0) { + BOOST_CHECK_EQUAL(errno, ESRCH); // already fully reaped - this IS proof of cleanup + } else { + BOOST_CHECK(pidfdReadableWithin(observerPidFd, 3000)); + ::close(observerPidFd); + } } // ===================================================================== From 31c72b4df51412df4f7f2070a4751fd23526bb57 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:48:37 +0200 Subject: [PATCH 13/15] [ML] Adjudicate final-review residuals: fix log PID, guard warm-up hang, guard E_Monitoring write Final whole-branch review of the fix wave (b42ee960e) surfaced three new, small issues in that very wave: - LOG_INFO printed childPid before the I2 fix restores it, always logging "PID 0" on success. - The I6 forkserver warm-up fixture called AwaitResult() unconditionally even when terminateChild() failed, risking an unbounded hang of the BOOST_GLOBAL_FIXTURE (and therefore the whole test binary) if Sandbox2::Kill() ever throws. - The new E_Monitoring write was unconditional on generation match alone, which could silently revert an E_TerminationRequested marker set by a concurrent registry-scanning terminator (PR E's future timeout caller). Applied directly per the SDD final-review adjudication rule (no second fix-wave dispatch): all three are small, load-bearing, and closing them now is cheaper than a PR E regression hunt. --- lib/sandbox/CSandboxedProcessSpawner_Linux.cc | 11 +++++++++-- .../CSandboxedProcessSpawnerLifecycleTest_Linux.cc | 10 ++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc index 92c8de2b3a..ce6ccca68a 100644 --- a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc +++ b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc @@ -631,12 +631,19 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, { std::lock_guard lock(m_PidRegistry->s_Mutex); const auto it = m_PidRegistry->s_Children.find(sandboxPid); - if (it != m_PidRegistry->s_Children.end() && it->second.s_Generation == generation) { + // Only advance from E_Registered: a registry-scanning terminator + // (e.g. a future PR E timeout caller) can race this window and + // already have set E_TerminationRequested on the same generation; + // an unconditional overwrite here would silently revert that marker. + if (it != m_PidRegistry->s_Children.end() && it->second.s_Generation == generation && + it->second.s_State == EChildLifecycleState::E_Registered) { it->second.s_State = EChildLifecycleState::E_Monitoring; } } - LOG_INFO(<< "Spawned sandboxed process " << processPath << " with PID " << childPid); + // Final-review fix: log the live PID, not the not-yet-restored out + // parameter (childPid is still 0 here per the I2 fix below). + LOG_INFO(<< "Spawned sandboxed process " << processPath << " with PID " << sandboxPid); // I2: only now, with registration and monitor handoff both confirmed, is // it safe to hand the live PID back to the caller. diff --git a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc index 9335e3a775..9e1b2a45ab 100644 --- a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc +++ b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc @@ -385,8 +385,14 @@ struct SForkserverWarmupFixture { // deterministic, not to assert anything itself. if (spawner.spawn(ML_SANDBOX2_LIFECYCLE_PAYLOAD, childIpcArgs(childRoot), childPid) && childPid > 0) { - spawner.terminateChild(childPid); - if (monitorBody) { + // Only await completion if termination was actually requested + // successfully: if Sandbox2::Kill() threw and terminateChild() + // returned false, the sandboxee may still be running, and an + // unconditional monitorBody() call would block this + // BOOST_GLOBAL_FIXTURE - and therefore the entire test binary, + // across every suite - inside AwaitResult() with no bound and + // no diagnostic (production's wall-time limit is unbounded). + if (spawner.terminateChild(childPid) && monitorBody) { monitorBody(); // real cleanup path: closes the pidfd, erases the entry. } } From 13cc5268def62c1339c856f8734e765b42148af8 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 21:14:20 +0200 Subject: [PATCH 14/15] [ML] Fix self-review round 2 findings: EXTERNAL_KILL assertion, gate 8 UAF, gate 3 fd leak, promise UAF, log classification - Gate 1 (testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd): Sandbox2::Kill() produces EXTERNAL_KILL/reason_code()==0, not SIGNALED/SIGKILL - fix the assertion to match the pinned sandboxed-api v20241008 status classification order. - realMonitorLaunchWithCompletionSignal: stop destroying the monitor closure (and its co-owned shared_ptr) before signalling completion; hand the caller its own shared_ptr> reference instead. Fixes a deterministic UAF in gate 8, which dereferenced a raw SPidRegistry* after the only remaining owning shared_ptr had already been released. Also switch its promise parameter to shared_ptr, matching the existing ENOSYS-case precedent, so a throw between spawn() and the wait can no longer free a stack-local promise out from under the still-running detached thread. - Gate 3 (testStaleMonitorGenerationCannotEraseNewerRegistration): close the real pidfd the case's fabricated stale-generation scenario leaves open, so SFdBaselineFixture's descriptor count matches the baseline. - logSandboxeeTermination: give EXTERNAL_KILL, VIOLATION, TIMEOUT, SETUP_ERROR and INTERNAL_ERROR their own log cases instead of funnelling them into one generic LOG_ERROR - EXTERNAL_KILL is this file's own successful ENOSYS-fallback path, and VIOLATION is the most operationally important signal a sandbox can report. - terminateChild(): document that a repeated call reaching Sandbox2::Kill() twice is safe (idempotent at the pinned tag) - considered during review, confirmed harmless, recorded so it isn't re-derived next time. --- lib/sandbox/CSandboxedProcessSpawner_Linux.cc | 49 +++++++ ...dboxedProcessSpawnerLifecycleTest_Linux.cc | 128 ++++++++++++++---- 2 files changed, 148 insertions(+), 29 deletions(-) diff --git a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc index ce6ccca68a..6e013709d6 100644 --- a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc +++ b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc @@ -273,6 +273,17 @@ bool defaultMonitorLaunch(std::function monitorBody) { //! Log how a sandboxed pytorch_inference terminated. Runs on the monitor //! thread that owns the sandbox instance, so it deliberately takes no //! spawner state - the caller does the registry bookkeeping under the lock. +//! +//! Review finding 5 (self-review round 2): every StatusEnum value gets its +//! own case rather than funnelling everything but SIGNALED into one opaque +//! LOG_ERROR. Two of those previously-generic cases matter operationally: +//! EXTERNAL_KILL is this file's OWN ENOSYS-fallback success path +//! (terminateChild()'s E_KernelUnsupported branch calls Sandbox2::Kill(), +//! which the monitor observes as EXTERNAL_KILL, not SIGNALED - see gate 1's +//! testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) and must not +//! be logged as an abnormal termination; VIOLATION is the most +//! operationally important signal a sandbox can report and must never be +//! indistinguishable from an internal error. void logSandboxeeTermination(core::CProcess::TPid sandboxPid, const sandbox2::Result& result) { switch (result.final_status()) { case sandbox2::Result::OK: @@ -287,7 +298,38 @@ void logSandboxeeTermination(core::CProcess::TPid sandboxPid, const sandbox2::Re LOG_INFO(<< "Sandboxed pytorch_inference (PID " << sandboxPid << ") was terminated by signal " << result.reason_code()); break; + case sandbox2::Result::EXTERNAL_KILL: + // Expected, successful termination - this is the ENOSYS-fallback + // path (Sandbox2::Kill() via terminateChild()'s E_KernelUnsupported + // branch), not a failure, so INFO rather than ERROR. + LOG_INFO(<< "Sandboxed pytorch_inference (PID " << sandboxPid + << ") was force-killed via Sandbox2::Kill()"); + break; + case sandbox2::Result::VIOLATION: + // reason_code() carries the violating syscall number for this + // status. Logged at ERROR with that detail so a seccomp policy + // violation is never mistaken for an opaque internal error. + LOG_ERROR(<< "Sandboxed pytorch_inference (PID " << sandboxPid + << ") violated the sandbox policy (syscall " << result.reason_code() << ')'); + break; + case sandbox2::Result::TIMEOUT: + LOG_ERROR(<< "Sandboxed pytorch_inference (PID " << sandboxPid + << ") exceeded its wall-time/CPU limit and was terminated"); + break; + case sandbox2::Result::SETUP_ERROR: + LOG_ERROR(<< "Sandboxed pytorch_inference (PID " << sandboxPid + << ") failed to set up the sandbox"); + break; + case sandbox2::Result::INTERNAL_ERROR: + LOG_ERROR(<< "Sandboxed pytorch_inference (PID " << sandboxPid + << ") hit an internal Sandbox2 error"); + break; default: + // UNSET (and any future StatusEnum value this file does not yet + // know about) - AwaitResult() has already returned by the time this + // runs, so UNSET should be structurally unreachable, but keep a + // narrow default rather than silently dropping an unrecognized + // status. LOG_ERROR(<< "Sandboxed pytorch_inference (PID " << sandboxPid << ") terminated abnormally, final_status=" << result.final_status()); break; @@ -674,6 +716,13 @@ bool CSandboxedProcessSpawner::terminateChild(core::CProcess::TPid pid) { { std::lock_guard lock(m_PidRegistry->s_Mutex); const auto it = m_PidRegistry->s_Children.find(pid); + // Considered-and-dropped (self-review round 2): this guard does not + // exclude E_TerminationRequested, so a repeated terminateChild() + // call on an already-in-flight (or already E_KernelUnsupported- + // Kill()ed) child can reach Sandbox2::Kill() a second time. Confirmed + // harmless against the pinned sandboxed-api v20241008 tag: + // Sandbox2::Kill() is idempotent (sets a flag and issues a null-safe + // notify; no double-free/double-signal), so this is not a hazard. if (it == m_PidRegistry->s_Children.end() || it->second.s_State == EChildLifecycleState::E_Reaped || it->second.s_State == EChildLifecycleState::E_Failed) { diff --git a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc index 9e1b2a45ab..dde9e3c1fb 100644 --- a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc +++ b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc @@ -302,21 +302,42 @@ TSpawner::TMonitorLaunchFn captureMonitorBodyWithoutRunning(std::function* donePromise) { - return [donePromise](std::function body) -> bool { - std::thread([body = std::move(body), donePromise]() mutable { - // I5: destroy the closure - and therefore its captured - // shared_ptr - BEFORE signaling completion. - // `body` is a member of this thread lambda's own closure, so - // without this it is not destroyed until the thread function - // returns, which happens AFTER set_value() below; a test - // blocked on donePromise's future could then observe "done" - // while the sandbox's fds may still be open, racing gates 7/8's - // fd-baseline check. - { - auto b = std::move(body); - b(); - } +//! +//! donePromise is heap-owned (shared_ptr), matching the precedent already +//! established for the ENOSYS case above (capturingAwaitResult's caller): +//! gates 7 and 8 both call spawn() before blocking on the returned future, +//! but if a BOOST_TEST_REQUIRE between spawn() and the wait throws, the +//! stack-local std::promise a raw pointer would have pointed at is +//! destroyed while this detached thread is still running and will later +//! call donePromise->set_value() on freed stack memory - self-review round +//! 2, finding 4. +//! +//! bodyKeepAliveOut is an optional extra output: when non-null, the seam +//! hands the caller its OWN shared_ptr> reference to the +//! monitor closure (self-review round 2, finding 2). Gate 8 needs this: it +//! destroys the spawner and then dereferences a raw SPidRegistry* obtained +//! from this same closure's registry-insert seam. That raw pointer is only +//! valid for as long as SOME shared_ptr copy - normally the +//! monitor closure's own - is still alive. Previously the detached thread +//! destroyed its only copy of the closure immediately after running it and +//! before signalling completion, so by the time gate 8's test thread woke +//! up and dereferenced the raw pointer, the registry had already been +//! freed (a deterministic UAF, not merely racy). Handing the test its own +//! extra reference here means the object survives regardless of when the +//! detached thread releases its own copy. This does not change the +//! fd-baseline story: gates 7/8 already keep the Sandbox2 handle alive via +//! their own `capturedSandbox` copy for the same span, so this reference +//! extends nothing that wasn't already being kept alive. +TSpawner::TMonitorLaunchFn +realMonitorLaunchWithCompletionSignal(std::shared_ptr> donePromise, + std::shared_ptr>* bodyKeepAliveOut = nullptr) { + return [donePromise, bodyKeepAliveOut](std::function body) -> bool { + auto bodyPtr = std::make_shared>(std::move(body)); + if (bodyKeepAliveOut != nullptr) { + *bodyKeepAliveOut = bodyPtr; + } + std::thread([bodyPtr, donePromise]() mutable { + (*bodyPtr)(); donePromise->set_value(); }).detach(); return true; @@ -564,8 +585,17 @@ BOOST_AUTO_TEST_CASE(testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) BOOST_TEST_REQUIRE(waitStatus == std::future_status::ready); BOOST_TEST_REQUIRE(*capturedResult != nullptr); - BOOST_CHECK((*capturedResult)->final_status() == sandbox2::Result::SIGNALED); // mechanism: some signal - BOOST_CHECK((*capturedResult)->reason_code() == SIGKILL); // mechanism: specifically SIGKILL, i.e. Kill() + // Self-review round 2, finding 1: Sandbox2::Kill() does not produce a + // WIFSIGNALED-style SIGNALED/SIGKILL result. It sets the monitor's + // external-kill flag, and the monitor's status classification (pinned + // sandboxed-api v20241008, monitor_ptrace.cc) checks that flag AHEAD of + // the WIFSIGNALED path, so a Kill()ed sandboxee is reported as + // EXTERNAL_KILL with reason_code() == 0, never SIGNALED/SIGKILL. + // EXTERNAL_KILL is actually the STRONGER discriminator here: it is only + // reachable via Sandbox2::Kill(), whereas SIGNALED could also be + // produced by an external SIGKILL unrelated to this mechanism. + BOOST_CHECK((*capturedResult)->final_status() == sandbox2::Result::EXTERNAL_KILL); // mechanism: Kill() + BOOST_CHECK((*capturedResult)->reason_code() == 0); BOOST_CHECK(registry->s_Children.count(childPid) == 0); // cleanup assertion } @@ -749,6 +779,18 @@ BOOST_AUTO_TEST_CASE(testStaleMonitorGenerationCannotEraseNewerRegistration) { BOOST_TEST_REQUIRE(it != registry->s_Children.end()); BOOST_CHECK_EQUAL(it->second.s_Generation, newerGeneration); BOOST_CHECK(it->second.s_State == TSpawner::EChildLifecycleState::E_Monitoring); + + // Self-review round 2, finding 3: this case uses the REAL pidfd path + // (empty TPidFdOpenFn{}), so the entry above still holds a genuine open + // pidfd. The stale monitor body correctly skipped closing it (generation + // mismatch - that skip is exactly what LI6 asserts above), but that also + // means nothing else in this case ever closes it: production's + // defaultRegistryInsert only closes a stale entry's pidfd when a NEWER + // spawn() replaces it, which never happens in this fabricated scenario. + // Close it explicitly so SFdBaselineFixture's end-of-case descriptor + // count matches the suite-wide baseline instead of leaking one fd on + // every run. + ::close(it->second.s_PidFd); } // ===================================================================== @@ -965,9 +1007,12 @@ BOOST_AUTO_TEST_CASE(testDestructorDoesNotJoinAndReturnsUnderOneSecond) { // the suite-wide SFdBaselineFixture's end-of-case descriptor count // (the real cleanup closes the child's pidfd on that same thread, // asynchronously with respect to this test case's own control flow). - std::promise monitorDonePromise; - std::future monitorDone{monitorDonePromise.get_future()}; - TSpawner::TMonitorLaunchFn monitorLaunch = realMonitorLaunchWithCompletionSignal(&monitorDonePromise); + // Heap-owned (shared_ptr), not a stack local, so a BOOST_TEST_REQUIRE + // throwing before monitorDone.wait() below cannot free this out from + // under the still-running detached thread (finding 4). + auto monitorDonePromise = std::make_shared>(); + std::future monitorDone{monitorDonePromise->get_future()}; + TSpawner::TMonitorLaunchFn monitorLaunch = realMonitorLaunchWithCompletionSignal(monitorDonePromise); auto spawner = std::make_unique(TSpawner::TPidFdOpenFn{}, insertFn, monitorLaunch, TSpawner::TAwaitResultFn{}); @@ -1008,14 +1053,31 @@ BOOST_AUTO_TEST_CASE(testMonitorCleanupRunsSafelyAfterSpawnerDestruction) { std::promise gatePromise; std::shared_future gate{gatePromise.get_future()}; - std::promise monitorDonePromise; - std::future monitorDone{monitorDonePromise.get_future()}; + // Heap-owned (shared_ptr), not a stack local, matching gate 7 (finding + // 4): a throw between spawn() and monitorDone.wait() below must not free + // this out from under the still-running detached thread. + auto monitorDonePromise = std::make_shared>(); + std::future monitorDone{monitorDonePromise->get_future()}; TSpawner::TAwaitResultFn awaitResultFn = [gate](sandbox2::Sandbox2& sandbox) -> sandbox2::Result { gate.wait(); // test-controlled synchronization point - never sleep(). return sandbox.AwaitResult(); }; - TSpawner::TMonitorLaunchFn monitorLaunch = realMonitorLaunchWithCompletionSignal(&monitorDonePromise); + // Self-review round 2, finding 2: also request the seam's own extra + // shared_ptr> reference to the monitor closure + // (monitorBodyKeepAlive), held by this test until after the registryRaw + // dereference below. Previously the ONLY surviving + // shared_ptr once the spawner block below exits was the + // detached thread's own copy inside that closure, and the thread + // destroyed its copy immediately after running the body and BEFORE + // signalling monitorDone - so by the time this test woke up from + // monitorDone.wait() and dereferenced registryRaw, the registry had + // already been freed (a deterministic use-after-free, not merely + // racy). Holding monitorBodyKeepAlive here keeps the same object alive + // regardless of when the detached thread releases its own copy. + std::shared_ptr> monitorBodyKeepAlive; + TSpawner::TMonitorLaunchFn monitorLaunch = + realMonitorLaunchWithCompletionSignal(monitorDonePromise, &monitorBodyKeepAlive); std::shared_ptr capturedSandbox; TSpawner::SPidRegistry* registryRaw{nullptr}; @@ -1031,12 +1093,14 @@ BOOST_AUTO_TEST_CASE(testMonitorCleanupRunsSafelyAfterSpawnerDestruction) { BOOST_CHECK(spawner.hasChild(childPid)); // reached marker } // spawner destroyed here; the monitor thread is still genuinely blocked on `gate`. - // registryRaw is only safe to dereference now because the monitor - // thread's own shared_ptr copy (captured in monitorBody's - // closure by spawn(), per design) keeps the same object alive - the - // spawner's own shared_ptr, which is what made this pointer valid - // originally, is gone. That co-ownership is exactly the property this - // test exists to exercise. + // registryRaw is safe to dereference below because monitorBodyKeepAlive + // (captured just above) holds its own shared_ptr reference + // via the monitor closure - independent of whatever the detached monitor + // thread's own copy of that same closure does or does not still hold by + // this point. The spawner's own shared_ptr, which is what made this + // pointer valid originally, is gone; this test-owned reference is what + // now keeps the object alive, exercising the same underlying + // co-ownership property the monitor thread relies on in production. // Release the gate and make the sandboxee actually exit, so the // now-unblocked real AwaitResult() call inside the monitor thread can @@ -1056,6 +1120,12 @@ BOOST_AUTO_TEST_CASE(testMonitorCleanupRunsSafelyAfterSpawnerDestruction) { BOOST_TEST_REQUIRE(registryRaw != nullptr); std::lock_guard lock(registryRaw->s_Mutex); BOOST_CHECK(registryRaw->s_Children.count(childPid) == 0); + // monitorBodyKeepAlive is not explicitly reset: it goes out of scope + // here, after every dereference of registryRaw above, which is all that + // matters for finding 2. Its (and capturedSandbox's) destruction here + // still runs on this thread, strictly before SFdBaselineFixture's + // end-of-case descriptor check, so this does not reintroduce the fd- + // baseline race the original early-destroy trick (I5) guarded against. } BOOST_AUTO_TEST_SUITE_END() From 92aafd87f0e8b2f08fb0cc0181126f0ec49f7032 Mon Sep 17 00:00:00 2001 From: Valeriy Khakhutskyy <1292899+valeriy42@users.noreply.github.com> Date: Wed, 9 Sep 2026 21:26:13 +0200 Subject: [PATCH 15/15] fix(sandbox2-lifecycle-test): release monitor closure refs before fd checks Round-3 self-review M1: testDescriptorCountReturnsToBaselineAfterSpawnTerminateCleanup asserted the fd baseline while its local monitorBody std::function still held the closure's captured shared_ptr, keeping the comms socketpair fd open. Release it (monitorBody = nullptr) before the check. S1: realMonitorLaunchWithCompletionSignal's detached thread called set_value() while still holding its own bodyPtr reference, reintroducing the I5 ordering hazard (a waiter could observe "done" before the thread released its Sandbox2 handle). Reset bodyPtr before signalling. Checked gates 7/8 for the same pattern: neither has an inline fd assertion in-scope with a handle-holding local (their only fd check is SFdBaselineFixture's post-return destructor), so no change needed there. --- ...SandboxedProcessSpawnerLifecycleTest_Linux.cc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc index dde9e3c1fb..a56da58c29 100644 --- a/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc +++ b/lib/sandbox/unittest/CSandboxedProcessSpawnerLifecycleTest_Linux.cc @@ -338,6 +338,11 @@ realMonitorLaunchWithCompletionSignal(std::shared_ptr> donePr } std::thread([bodyPtr, donePromise]() mutable { (*bodyPtr)(); + bodyPtr.reset(); // release this thread's reference BEFORE signalling, so a waiter + // observing "done" is guaranteed this thread no longer holds the + // closure (and therefore the Sandbox2 handle it captured) - restores + // the ordering guarantee an earlier round's I5 fix established + // (self-review round 3, finding S1). donePromise->set_value(); }).detach(); return true; @@ -975,6 +980,17 @@ BOOST_AUTO_TEST_CASE(testDescriptorCountReturnsToBaselineAfterSpawnTerminateClea BOOST_TEST_REQUIRE(static_cast(monitorBody)); monitorBody(); // real cleanup path: closes the pidfd, erases the entry. + // Self-review round 3, finding M1: monitorBody's closure (built by + // spawn()'s defaultMonitorLaunch path) captures `sandbox` - + // shared_ptr - BY VALUE and never releases it during + // execution; invoking the closure does not destroy the closure itself. + // This `monitorBody` local therefore still keeps the Sandbox2 instance + // (and its supervisor-side comms socketpair fd, only closed by + // ~Comms()/~Sandbox2()) alive until it goes out of scope. Release it + // explicitly here, BEFORE the fd-baseline check, so ~Sandbox2() (and the + // comms fd close) has already run when openFdCount() is taken. + monitorBody = nullptr; + BOOST_CHECK_EQUAL(openFdCount(), before); }