Conversation
Dispatches an already-decided route (E_Sandbox2/E_Legacy) to CSandboxedProcessSpawner or core::CDetachedProcessSpawner. Fixes three defects in the frozen enhancement/sandbox2 prior art: never re-parses --disableSandbox from args (route is an explicit caller-supplied parameter), never retries a failed Sandbox2 launch through the legacy spawner (V2), and explicitly fails closed - rather than silently falling through - when a sandboxed process path is requested on a build without Sandbox2 support.
Extends CCommandProcessor to decide a spawn route (E_Sandbox2 default, E_Legacy only for the operator kill-switch token) and hand it to the now-landed CProcessSpawnerRouter, instead of talking to CDetachedProcessSpawner directly. handleStart() scans tokens for --disableSandbox before any spawn decision: zero occurrences keeps the Sandbox2 route; exactly one occurrence on the configured sandboxed path strips the token and routes to legacy; any other case (wrong path, or 2+ occurrences) rejects the command before spawning. CCommandProcessor's constructor now takes an explicit sandboxedProcessPaths list (no default that reuses permittedProcessPaths).
… (PR E Task 3) Flip TERMINATE_ON_DEGRADED_SECCOMP_FAILURE from false to true in Main.cc. This is now safe because CProcessSpawnerRouter (Task 2) guarantees that degraded-mode launches are never accidental fallbacks from failed Sandbox2 attempts, only ever explicit --disableSandbox route decisions. Updated comment to state the concrete invariant this flip depends on. Existing fault-injection test (testDecideDegradedModeActionFaultInjection) already covers all failure modes and validates expected behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Emit a single-line JSON sandbox2_launch log line from CProcessSpawnerRouter::spawn() for every Sandbox2-eligible spawn (deployment_id, model_id, route, sandbox2_established, mode), fired on every dispatch outcome including a failed spawn. Adds docs/sandbox2_production_failure_modes.md documenting the schema.
Adds ml_sandbox_userns_probe.cc: a dependency-free payload exercising the 7
staged kernel primitives Sandbox2's forkserver depends on (pipe+fork,
unshare(CLONE_NEWUSER), uid/gid map writes incl. setgroups,
unshare(CLONE_NEWNS|CLONE_NEWPID), fork into the new PID namespace, then
mount("/", MS_REC|MS_PRIVATE) and mount("proc", ...) - the proc mount runs
strictly after the stage-5 fork, preserving the 50bacc2 ordering fix.
CSandboxUserNamespaceProbeTest_Linux.cc runs it as a plain host subprocess
(no Sandbox2 policy involved - this characterizes the ambient CI
environment, not a sandbox policy) and reacts to ML_SANDBOX2_REQUIRE:
unset=diagnostic-only, enforced=must-pass, fail_closed=must-fail-somewhere.
run_tests.sh wires a two-pass mode loop (enforced, fail_closed) into the
aarch64/Docker branch and a one-pass (fail_closed only) loop into the
x86_64/macOS branch, per the accepted H3 risk: no userns-capable x86_64
Buildkite runner exists (EPERM on mount("proc", ...)), so enforced mode is
never exercised there.
runProbe() collapsed every non-zero exit into a single bool, so an execl() failure (missing/unexecutable payload binary) exited 127 and satisfied fail_closed's !probeSucceeded check the same as a genuine staged probe failure - masking a broken build/CMake wiring as confirmed absence of userns capability. Classify the child's exit status into Success / StagedFailure / ExecFailure (126/127 reserved for exec failure, distinct from the payload's own EXIT_FAILURE=1 staged-failure code) and fail the test outright on ExecFailure before consulting ML_SANDBOX2_REQUIRE, in any mode.
Ports the Sandbox2 attack-defense harness from the frozen enhancement/sandbox2 branch, fixing 6 defects found during review: no reached marker (models could crash before execution and look identical to a block; also fixes the root cause by adding --skipModelValidation so CModelGraphValidator doesn't reject the attack models before forward() runs), no unsandboxed positive control, a leak-model test case that duplicated the exploit case's assertions, a shared un-drained controller/pytorch output FIFO reader that could leak stale responses across commands/cases, no per-case kill/reap cleanup assertion, and a flat IPC layout that didn't match the real $TMPDIR/ml-child-ipc/<child-id> contract. Also ports evil_model_generator.py and dev-tools/run_sandbox2_attack_defense.sh, and extends docs/sandbox2_production_failure_modes.md to name this harness as V14's evidence source per design.md's closure requirement.
- ControllerProcess.__init__: wrap the constructor body in try/except that calls self.cleanup() before re-raising. Previously, if __init__ raised after subprocess.Popen succeeded, main()'s `controller` variable was never assigned, so its `finally: if controller is not None: controller.cleanup()` never ran - leaking the spawned controller binary and its reader/stdin-keeper threads. - PipeReaderThread: open the FIFO O_RDONLY | O_NONBLOCK instead of a blocking O_RDONLY open, and poll readability with select() in the read loop (checking `running` between polls) instead of blocking in os.read(). Previously, stop() was a no-op while the thread was parked in the blocking open() (self.fd stayed None until a writer connected), so any run_pytorch_case() early-return path where pytorch_inference never opened its output/log FIFO left the reader thread permanently stuck, and the subsequent os.remove(pipe_path) unlinked the FIFO out from under it.
…ide) Add 3rd_party/controller-protocol.version (single line, controller-protocol-version=1) asserting the controller's --disableSandbox token semantics (controller-only, never forwarded to the child) and the per-child IPC route contract ($TMPDIR/ml-child-ipc/<child-id> -> /run/elastic/ml-ipc). Wire it into buildZip so it lands at the top level of buildDependenciesZip's -deps zip, unaffected by dependenciesSpec's exclude list, for a future Elasticsearch-side assertion (out of scope here). Task 7 of PR E, Sandbox2 rebuild epic.
…ivation Final whole-branch review fixes 1, 2, 5, 6 and 7. - SHIPS DORMANT (fix 1): the no-token route decision for a configured sandboxed process path is now gated on the internal ML_SANDBOX2_DEFAULT_ENFORCED option (exactly "1" is truthy, off by default), so a plain pytorch_inference launch keeps the legacy route until Elasticsearch owns the operator setting that turns mandatory Sandbox2 on. The router's legacy-route log line no longer claims a --disableSandbox token it cannot see; CCommandProcessor logs the route provenance where it is known. - bin/controller/Main.cc's unconditional sandboxed-path list needs no change (fix 2): with the default off, that list only nominates which path accepts the token and which emits the H4 signal. Comment added. - validateChildIpcLaunchSpec() is now called once per spawn(), before dispatch, and its childId threaded into the H4 signal (fix 5), so deployment_id is populated on the degraded and fail_closed modes instead of blanking on exactly the modes the signal exists to debug. CSandboxedProcessSpawner_Linux.cc's own V16 gate is untouched. jsonEscape() now escapes control characters as well as quote and backslash. - One shared isSandboxedProcessPath() predicate, owned by the router and queried by CCommandProcessor (fix 6), replacing two independent std::find copies that could silently desync. - Negative assertion that a non-sandboxed process path emits no sandbox2_launch line at all (fix 7).
Final whole-branch review fix 3. Nothing in the tree read the ML_SANDBOXED value CSandboxedProcessSpawner sets on a Sandbox2-launched child, so a sandboxee still attempted its own in-process seccomp install from inside an already-sandboxed environment - which, with MG8's hard termination now active, could kill every enforced-route launch, or succeed and emit the legacy-route attestation marker on a launch the H4 signal reports as route sandbox2. pytorch_inference now runs the install / degraded-mode decision / attestation-marker sequence only when ML_SANDBOXED is not exactly "1" (design.md routing contract point 5), via a pure applyInProcessSeccompFilter() helper so the skip is unit-testable off Linux: the installer is never invoked, no action is derived and no marker is produced, for every outcome an attempt could have returned.
…er arch
Final whole-branch review fixes 4 and 8, plus the fix-wave report.
- test_sandbox2_attack_defense.py discovered the child PID by filtering
/proc on PPid == controller pid, but the Sandbox2 sandboxee is a child
of the forkserver, not of the controller, so every sandboxed case
failed at PID discovery and only the unsandboxed control could pass.
PID now comes from the "Spawned ... with PID" line both spawners
already log on the controller's log pipe, scoped per case by the
existing log-offset mechanism - one mechanism for every case.
- run_tests.sh ran both ML_SANDBOX2_REQUIRE=enforced and =fail_closed on
the same aarch64 host/kernel, asserting mutually exclusive outcomes,
so exactly one pass always failed. aarch64 now runs enforced only,
matching H3 ("aarch64 enforced (pinned); x86_64 fail-closed"); the
x86_64 branch keeps fail_closed and drops its single-element loop.
…terminate dormant Two fail-open/regression fixes uncovered by re-reviewing the ship-dormant change: * CDetachedProcessSpawner now builds the child's environment from environ with any exact-name ML_SANDBOXED entry removed, instead of passing environ through unfiltered. pytorch_inference skips its mandatory in-process seccomp filter when it sees ML_SANDBOXED=1, so an inherited or injected marker in the controller's environment would have silently left a legacy-route child with no security boundary at all. CSystemCallFilter.h's comment claimed this stripping already happened; it now points at the implementation that performs it. * TERMINATE_ON_DEGRADED_SECCOMP_FAILURE goes back to false. Its justification was that a degraded launch is only reachable via an explicit --disableSandbox token, which stopped being true once the no-token default became the legacy route: every ordinary launch would exit EXIT_FAILURE wherever in-process seccomp installation fails. The comment now records that activation belongs with the change that stops legacy being the default. The decideDegradedModeAction() fault-injection tests pass the bool explicitly and are unchanged.
* Adds an additive legacy_reason field to the sandbox2_launch signal, emitted only when route == "legacy" and omitted entirely (never "", never null) on route == "sandbox2" - i.e. absent for both enforced and fail_closed. mode == "degraded" alone cannot tell a deliberate --disableSandbox kill switch from the dormant default that holds for the whole rollout window, during which every ordinary launch is degraded. CCommandProcessor passes the provenance it already knows from deciding the route; the router still never derives it from args. No existing field or its semantics change. * The V14 attack-defense harness starts the controller with ML_SANDBOX2_DEFAULT_ENFORCED=1 and asserts, from each launch's own H4 signal and before any target-file assertion, that the case took the route it means to test. Its "sandboxed" cases send a plain start with no token, so with the dormant default they were routing to the legacy path and the negative assertion was being checked against a child that was never sandboxed.
…h comment CreateProcess() on Windows was called with lpEnvironment=0, so the legacy route's child inherited the parent's environment completely unfiltered - including ML_SANDBOXED if present - while pytorch_inference's sandbox2LaunchedChild() check (bin/pytorch_inference/Main.cc) that decides whether to install the in-process seccomp filter runs unconditionally on every platform. The POSIX ML_SANDBOXED-stripping added earlier in this PR was guarded #ifndef Windows and never had a Windows counterpart, so a Windows build could fail open. Port the stripping to CDetachedProcessSpawner_Windows.cc: build a filtered ANSI environment block from GetEnvironmentStringsA() (matching the file's existing ANSI CreateProcess usage) and pass it via lpEnvironment instead of 0. Add a Windows-gated unit test mirroring the existing POSIX testMlSandboxedStrippedFromChildEnvironment. Also drop a dead .superpowers/... scratch-path reference from a comment in test/evil_model_generator.py - that path is a session-scoped planning file in a different repo, not present for anyone cloning ml-cpp standalone.
isStrippedChildEnvEntry() in CDetachedProcessSpawner_Windows.cc used ::strncmp (case-sensitive) to match the ML_SANDBOXED marker. Windows environment variable names are case-insensitive OS-wide, and the child-side reader (CSystemCallFilter::sandbox2LaunchedChild() via std::getenv) matches case-insensitively too, so a differently-cased entry such as ml_sandboxed=1 would survive the strip and still be found by the child - reproducing the fail-open bypass the prior fix closed. Switch to ::_strnicmp (case-insensitive strncmp), matching this file's existing narrow-ANSI API usage. POSIX's strncmp is left unchanged since POSIX env var names are case-sensitive. Adds mixed-case coverage to the Windows-only testMlSandboxedStrippedFromChildEnvironmentBlock test.
…x 3) GetEnvironmentStringsA()/CreateProcessA() round-tripped the parent's native UTF-16 environment through the ANSI code page, silently mangling any value not representable there (e.g. TEMP/USERPROFILE under a non-ASCII Windows username) to '?' for every Windows child - a regression introduced as a side effect of the ML_SANDBOXED stripping. Switch to GetEnvironmentStringsW()/CreateProcessW()/CREATE_UNICODE_ENVIRONMENT end to end so the environment block is never converted at all; the in-process isStrippedChildEnvEntry()/buildChildEnvironmentBlock() pair now operate on wchar_t/std::wstring, matching the case-insensitive _wcsnicmp compare. cmdLine/processPath go through CStringUtils::narrowToWide() purely for CreateProcessW's other two string parameters (a lateral move, not a regression, since CreateProcessA already interpreted them through the ANSI code page). Windows-only test updated accordingly.
A Linux build WITH Sandbox2 support and one WITHOUT it emitted identical H4 signals (route:"legacy", legacy_reason:"dormant_default", mode:"degraded") for every plain launch under the shipped dormant default. Add an additive "sandbox2_compiled_in" boolean, sourced once from sandbox::CMlSandboxAvailability::isCompiledIn() (a build-time constant, not per-launch state) and emitted on every signal line regardless of route - unlike legacy_reason. Lets PR F's rollout logic distinguish "supported but dormant" from "not capable at all". Documented in docs/sandbox2_production_failure_modes.md's field table and both example lines; covered by a new test. Also fixes testH4SignalEscapesControlCharactersInDeploymentId, which searched for the JSON line's end via the literal "mode":"degraded"} - no longer valid now that an additive field follows mode.
…ix 5) noDependenciesSpec's include-list didn't name controller-protocol.version, so the H2 capability token was present in buildDependenciesZip's output but absent from buildNoDependenciesZip's - yet the controller binary the token makes claims about ships only in the nodeps zip. A build combining a locally-built nodeps with a downloaded deps snapshot could assert the token from a different ml-cpp revision than the actual controller. dependenciesSpec already ships it implicitly (no matching exclude); add it explicitly to noDependenciesSpec's whitelist so it's present in BOTH zips, per the original plan's "excluded from neither" acceptance bar.
… Fixes 8, 9) captureLogged() in CCommandProcessorTest.cc and CProcessSpawnerRouterTest.cc did reconfigure(stream); fn(); CLogger::instance().reset() - if fn() threw (e.g. a failed BOOST_REQUIRE* inside it), reset() never ran, leaving the global logger redirected into a stream nobody reads for the rest of the test binary process. Add a local CScopedLoggerReset RAII guard to each file so reset() always runs. testMlSandboxedStrippedFromChildEnvironment in CDetachedProcessSpawnerTest.cc set ML_SANDBOXED/ML_SANDBOXED_KEEP_ME and only unset them at the end of the test function - an earlier BOOST_REQUIRE failure would skip the unset calls and leak the marker into later tests in the same process. Add a CScopedEnvVar RAII guard (same idiom as CScopedSandbox2DefaultEnforced/CScopedChildIpcRoot elsewhere in this PR) and use it instead of manual setEnv/unSetEnv.
… (review Fix 10) ML_SANDBOX2_REQUIRE=fail_closed asserted BOOST_TEST_REQUIRE(!probeSucceeded) - i.e. it required userns capability to be UNAVAILABLE. MG6's accepted risk names its own revisit trigger as "when a userns-capable x86_64 CI runner becomes available" - the day that happens, this assertion would flip to failing and look exactly like a regression rather than an environment improvement. Distinguish three outcomes instead: harness/exec broken (unchanged, still a hard failure via the existing E_ExecFailure branch), userns genuinely absent (expected, log and pass), and userns now available (log a clear, actionable message pointing at MG6's revisit trigger, but do not fail the build).
…w nice-to-haves) - CSystemCallFilter.h / CSeccompFilterBuilderTest.cc: reword "now that hard termination is active" to the conditional "once TERMINATE_ON_DEGRADED_SECCOMP_FAILURE is activated" - stale from an earlier fix round that flipped the constant to true, since reverted to false. Matches Main.cc's already-correct phrasing. - lib/sandbox/CMakeLists.txt: update the stale "No controller or pytorch_inference routing depends on it yet" comment - PR E's CProcessSpawnerRouter (linked via bin/controller/CMakeLists.txt's MlSandbox) and pytorch_inference's seccomp path are exactly that wiring, landed in this PR. - ml_sandbox_userns_probe.cc: guard #define _GNU_SOURCE with #ifndef _GNU_SOURCE - g++ already predefines it on glibc targets, causing a macro-redefinition warning.
valeriy42
added this pull request to stack #3183
September 10, 2026 07:24
CI's "Validate formatting with clang-format" check requires exactly clang-format 5.0.1 (docker.elastic.co/ml-dev/ml-check-style:2); several fix-round subagents on this branch could only verify with a newer local clang-format and disclosed the gap. Whitespace/line-wrap only, no logic change - confirmed via `git diff -w` and by re-running ml_test_controller and ml_test_seccomp before and after (same 6 pre-existing, unrelated local-environment failures in both).
Comments and docs referenced private planning-doc artifacts (design.md, evidence.md, MG/SG/V/H finding IDs, PR A-F letter labels, Task N numbering) from the elastic-workspace harness that orchestrates ml-cpp development. These are meaningless to anyone with only ml-cpp checked out. Rewrites each citation as self-contained prose describing the actual constraint, invariant, or fact, or points at the real thing being described (e.g. the sandbox2_launch signal by name) instead of the workspace doc section that discusses it. No logic changes; the V14 heading in docs/sandbox2_production_failure_modes.md is retitled to describe what it actually is.
OUT collides with the SAL annotation macro of the same name defined by Windows headers (windef.h's IN/OUT/NEAR/FAR parameter-direction hints), so MSVC preprocessed every `OUT` in these tests to nothing before parsing - e.g. `std::remove(OUT.c_str())` became `std::remove(.c_str())`, producing a cascade of syntax errors starting at CCommandProcessorTest.cc(291). Renamed to TARGET_FILE, distinct from the file's existing OUTPUT_FILE constant.
…unt tests Several PR E tests hardcoded POSIX-only "-c"/"cp" shell invocations, unlike the file's own pre-existing PROCESS_ARGS1/2 which already branch per platform. On Windows this ran cmd.exe with literal POSIX syntax, so 8 tests never produced their expected file. Added copyArgs(), a platform-aware helper mirroring the existing pattern, and switched all 8 call sites to it. Two tests (testStartStripsDisableSandboxTokenForConfiguredSandboxedPath, testStartLeavesArgsUntouchedWhenTokenAbsent) rely on counting a POSIX shell's positional parameters ($#) to prove exact token stripping. cmd.exe's /C form concatenates every arg into one command-line string for CreateProcess rather than exposing them as separate replaceable parameters, so this technique has no Windows equivalent; gated both behind #ifndef Windows.
…ndbox spawner ml_test_controller crashed deterministically on Linux (SEGV inside libpthread at the same address every run) at the teardown of any test that constructed a CCommandProcessor or CProcessSpawnerRouter, including tests that predate this PR. Root cause is an ODR violation confined to that one binary. include/sandbox/CSandboxedProcessSpawner.h declares an extra member (m_AwaitResultFn) only under SANDBOX2_AVAILABLE, so sizeof(CSandboxedProcessSpawner) differs by 32 bytes between translation units compiled with and without that macro. ml_add_executable() creates the Mlcontroller OBJECT library without any link libraries, so the controller sources compiled into it never saw MlSandbox's PUBLIC SANDBOX2_AVAILABLE, while bin/controller/unittest's own translation units - which do link MlSandbox - did. With CProcessSpawnerRouter holding the sandboxed spawner by value, that difference propagated into sizeof(CProcessSpawnerRouter) and sizeof(CCommandProcessor), so the classes' inline constructors and destructors disagreed about member offsets and corrupted memory as a router was destroyed - surfacing in the pthread calls ~CDetachedProcessSpawner() makes to stop its tracker thread. The production controller executable recompiles the same sources with MlSandbox linked, so it was unaffected, as were all non-Linux platforms, where the macro is never defined. Two changes: * CProcessSpawnerRouter now holds the sandboxed spawner behind a std::unique_ptr, created on first use inside spawn()'s Sandbox2 branch. A router that only ever dispatches the legacy route - every router while the Sandbox2 default is dormant - no longer constructs or destructs any Sandbox2 machinery at all, and the class's layout no longer depends on SANDBOX2_AVAILABLE. terminateChild()/hasChild() treat a null spawner as "no sandboxed children exist" rather than constructing one to ask. * bin/controller/CMakeLists.txt links the Mlcontroller OBJECT library against MlSandbox, so its sources compile with the same Sandbox2 configuration as both the production executable and the unit tests. Routing behaviour is unchanged: no dispatch decision, fail-closed branch or sandbox2_launch signal field is touched, and the Sandbox2 route still never falls back to the legacy spawner. Adds two regression tests: a static_assert that the router does not store the sandboxed spawner by value (the property that made its layout macro-sensitive), and a legacy-only router lifecycle test covering repeated construction, dispatch, live-child queries and destruction.
valeriy42
added a commit
to valeriy42/elasticsearch
that referenced
this pull request
Sep 10, 2026
The check hard-fails bundlePlugin/explodedBundlePlugin today because the resolved ml-cpp SNAPSHOT does not yet publish controller-protocol.version (companion change: elastic/ml-cpp#3188). That blocks every build on this branch, including work unrelated to sandboxing. sandbox_enabled defaults to false during this dark-launch window, so nothing downstream depends on the mismatch yet. Log a warning instead of throwing for now; revert to GradleException once ml-cpp#3188 lands and mlCppVersion() resolves to an artifact containing the file. Detection logic is unchanged.
valeriy42
added a commit
to valeriy42/elasticsearch
that referenced
this pull request
Sep 10, 2026
…ocked The javadoc claimed the sandbox_enabled=false (default) path "uses exactly the pipe layout the bundled ml-cpp controller already supports today" and was therefore independent of the paired ml-cpp artifact. That's false: PyTorchBuilder emits --disableSandbox unconditionally on Linux at sandbox_enabled=false (PyTorchBuilder#buildCommand), and today's bundled controller has no parsing for that token - it forwards it verbatim to pytorch_inference, which aborts on the unrecognized CLI option. So the default path is blocked on the paired artifact too, for a different reason (unrecognized flag) than the explicit-enable path (missing isolated IPC directory support). Rewrite the class-level javadoc and the per-method javadoc on testDefaultSandboxDisabledStartsAndInfersSuccessfully and testChildIpcPathsIsolatedPerDeployment (both run at the default setting) to state plainly that every test method in this class is currently blocked on elastic/ml-cpp#3188, and add @AwaitsFix to those two methods for consistency with the other two - verifyControllerProtocolVersion now hard-fails the build before any of these tests could run regardless of annotation, so all four should carry the same accurate, skip-cleanly-not-error-loudly annotation.
valeriy42
marked this pull request as ready for review
September 10, 2026 14:19
|
Pinging @elastic/ml-core (Team:ML) |
…OX2_DEFAULT_ENFORCED Elasticsearch can now request Sandbox2 explicitly on a start command instead of relying on an internal controller env var that core-server bootstrap had no path to set. --requireSandbox forces the Sandbox2 route (same duplicate/mutual-exclusion/sandboxed-path validation and strip-before-forward as --disableSandbox), and is rejected together with --disableSandbox on the same command. The no-token case now always takes the legacy route unconditionally - permanent behaviour for non-ES callers, not a rollout seam - since Elasticsearch is expected to always send one of the two tokens per launch. Renamed ELegacyReason::E_DormantDefault to E_NoTokenDefault and the H4 signal's legacy_reason value from "dormant_default" to "no_token_default" to match. Attack-defense harness now sends --requireSandbox explicitly for its sandboxed cases instead of setting the env var on the controller's own environment. Bumped controller-protocol-version to 2 for the wire-format addition.
Two line-wrapping violations from f8c05c8, caught by CI's check-style step (clang-format 5.0.1). Docker-based local verification: the platform-mismatch warning on arm64 previously caused the check to silently no-op; forcing --platform linux/amd64 makes it actually run and confirms all 9 touched files are clean.
CSandboxedProcessSpawner_Linux.cc's spawn() rejects every sandboxed pytorch_inference launch with E_CanonicalizationFailed, because validateChildIpcLaunchSpec() calls realpath() on $TMPDIR/ml-child-ipc/<child-id> before that directory has ever been created. realpath() requires its target to exist, so this failed for every child-id, every time - it was not a mis-ordering of an existing creation step, the creation step itself did not exist anywhere in production code. Comments in CPytorchInferenceSandboxPolicy.h/.cc and CProcessSpawnerRouter.cc already asserted "the native controller creates the per-child ml-child-ipc/<child-id> directory (mode 0700) before policy construction", but grepping the whole tree for mkdir/create_directories under lib/sandbox and bin/controller only turns up test fixtures (CPytorchInferenceSandboxPolicyTest.cc, CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc, CSandboxedProcessSpawnerLifecycleTest_Linux.cc, CProcessSpawnerRouterTest.cc) creating the directory by hand before calling into this code - no production code ever did. The same validateChildIpcLaunchSpec() function has a second production call site with the identical problem: CProcessSpawnerRouter::spawn() calls it (via deriveDeploymentId()) to derive the deployment id for the sandbox2_launch log signal, before dispatching to either the legacy or Sandbox2 backend - so the signal's deployment_id field was silently empty on every launch too. Fix: add ensureChildIpcDirectory() to CPytorchInferenceSandboxPolicy, which derives the single child-id implied by the launch's --input=/--output=/--restore=/--logPipe= arguments (a literal, pre-canonicalization structural match against $TMPDIR/ml-child-ipc/<one component> - it is not a security check; validateChildIpcLaunchSpec()'s canonical-base/symlink/depth checks still run afterwards against whatever this creates or finds) and mkdir()s it with mode 0700. Call it at both call sites, before validateChildIpcLaunchSpec(). It is idempotent: an already-existing directory (a retry/restart reusing the same child-id) is success, not an error. A creation failure for any other reason (permissions, ENOSPC, ...) is reported as a distinct outcome and logged at the spawner call site, then still flows into the existing E_CanonicalizationFailed rejection path, so a failed mkdir() fails the spawn cleanly rather than crashing or silently proceeding without the directory it needs. Extends CPytorchInferenceSandboxPolicyTest.cc with three cases against the real (missing-directory) scenario: validation fails before the directory exists, ensureChildIpcDirectory() then makes it and validation succeeds with the mode verified as 0700, a second call for the same child-id is a no-op success (retry/restart), and an unwritable trusted base makes ensureChildIpcDirectory() report E_CreationFailed with validation still failing closed afterwards.
The clean rebuild's Sandbox2 policy builder grants syscalls to pytorch_inference by looping over legacyBpfAllowedSyscalls() alone - the same list used to build the legacy in-process BPF filter. That is not sufficient: Sandbox2's namespace/threading setup makes pytorch_inference exercise syscalls (scheduling, epoll, pipes, directory/file management for forecast temp storage) that the simpler legacy filter never needed a grant for, since it never sets up namespaces or Sandbox2's own thread/monitor machinery. #2873's enhancement/sandbox2 branch already solved this with a dedicated sandbox2ExplicitSyscalls() list, granted in addition to what Sandbox2's PolicyBuilder helpers (AllowRead/AllowWrite/AllowOpen/etc.) cover implicitly. That list was never carried into this rebuild's CPytorchInferenceSyscallAllowlist.h, so any syscall in it (confirmed via a real run: sched_getaffinity) got denied and killed the sandboxed process. Ports sandbox2ExplicitSyscalls() and sandbox2HelperCoveredSyscalls() from the original branch, wires the explicit list into buildPytorchInferencePolicy() alongside the existing legacy-list loop, and adds sandbox2AllowsAllLegacySyscalls() plus a regression test so a future change to legacyBpfAllowedSyscalls() without a corresponding Sandbox2 grant fails loudly instead of silently regressing.
…box2 buildPytorchInferenceFilesystemPolicy() mounted spec.s_ChildIpcRoot into the sandbox at a fixed remapped path (/run/elastic/ml-ipc) instead of its own host path. Elasticsearch constructs pytorch_inference's --input=/--output=/--restore=/--logPipe= argv using the real host path under $TMPDIR/ml-child-ipc/<child-id>, so once Sandbox2 enforcement actually engaged (after the previous two fixes), pytorch_inference could not find its own pipes: it looked for them at the host path baked into its argv, which does not exist inside its own mount namespace once mounted under a different name. Mounts the child IPC root at the same absolute path inside and outside the sandbox instead, matching the convention every other mount in this policy already uses (AddDirectory, not AddDirectoryAt). Updates the doc comments in CPytorchInferenceSandboxPolicy.h and build.gradle's controller-protocol contract comment that described the old remap, and updates CPytorchInferenceSandboxPolicyMechanismTest_Linux's probe invocation to pass the host-visible childRoot instead of the now-removed hardcoded /run/elastic/ml-ipc literal. Verified on the devbox: CPytorchInferenceSandboxPolicyMechanismTest_Linux passes (ipc_readwrite: allowed), and PyTorchSandboxIT#testConcurrentDeploymentsDoNotCollideUnderIsolatedChildIpcDir passes end-to-end against a rebuilt ml-cpp artifact (0 failures, 0 errors).
Sandbox2 mounts a fresh, PID-namespaced procfs at /proc on the outer root before it builds and pivots into the sandbox chroot, then detaches the old root - so the pivoted rootfs has no /proc unless the policy adds one. The E_MountNamespacedProcfs case was a no-op on the false assumption that Sandbox2 provides /proc inside the rootfs automatically. With /proc absent, readlink(/proc/self/exe) and open(/proc/self/maps) both fail with ENOENT. Intel oneMKL's runtime dispatcher reads /proc/self/exe to self-locate and dlopen its CPU-specific libmkl_*.so.3 kernels; the failed read makes it abort with "Intel oneMKL FATAL ERROR: Cannot load <mkl-loader>", killing every sandboxed pytorch_inference under enforced Sandbox2. Bind /proc into the rootfs. Because Sandbox2 mounts the fresh procfs after CLONE_NEWPID and before PrepareChroot, the bind captures that already namespaced procfs (never the host's): verified inside the sandbox, /proc shows only the sandboxee's own PIDs (2) versus 194 on the host. /sys is left unmounted (E_Skip) - no fresh namespaced /sys exists to bind outside a network namespace, and pytorch_inference/libtorch run without it. Verified on the qaf local-deployment harness with xpack.ml.trained_models.sandbox_enabled=true (mode:enforced): the trained-model boot check and 7/8 test_scenario_buildly inference scenarios pass with zero MKL crashes across all enforced launches.
The Sandbox2 filesystem policy had no test exercising /proc, so the missing /proc mount that broke Intel oneMKL's runtime dispatcher went undetected. Add a proc_self_exe mechanism to ml_sandbox_probe (readlink /proc/self/exe inside the real sandbox) and assert it in the policy mechanism test. Without the /proc mount this readlink fails with ENOENT and the check reports "unreadable", turning the "Cannot load <mkl-loader>" crash into a build-time test failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacks on #3187. Adds typed controller-side routing behind two symmetric controller tokens.
CCommandProcessorparses at most one of--disableSandbox(operator kill switch, forces legacy) or--requireSandbox(operator opt-in, forces Sandbox2) for the exact configuredpytorch_inferencepath. Duplicate occurrences of either token, a mismatched process path, or both tokens present together are all rejected before spawn. The selected token is stripped before the child ever sees it.CProcessSpawnerRouterdispatches the decided route toCSandboxedProcessSpawnerorCDetachedProcessSpawnerwith no automatic fallback: a required Sandbox2 launch that fails returns a failed start response, it never retries through the legacy spawner.startcommand with neither token always takes the legacy route - permanent behaviour for any caller that sends no routing token (support/debug scripts, direct controller invocation, the test harness), not a rollout seam. Elasticsearch is expected to always send exactly one of the two tokens per launch, chosen from its own operator setting's live value.sandbox2_launch:deployment_id,model_id,route,sandbox2_established,mode,legacy_reason,sandbox2_compiled_in) so a consumer can distinguish an operator kill switch from the no-token default, and "Sandbox2 supported but no routing token sent" from "built without Sandbox2 support."ML_SANDBOXEDis stripped from every legacy-route child's environment (POSIX and Windows) so an inherited or injected value can never fail-open the mandatory in-process seccomp filter.EPERMonmount("proc", ...)and there is currently no userns-capable x86_64 CI runner.test_sandbox2_attack_defense.py: reached markers, unsandboxed positive controls, per-case cleanup/reap assertions, the realml-child-ipc/<child-id>IPC layout, an assertion that the controller's ownsandbox2_launchsignal reports the expected route before any security-boundary check runs, and explicit--requireSandbox/--disableSandboxtokens per case instead of a global env-var default.3rd_party/controller-protocol.version, now version 2) for a future cross-repo compatibility check.Filesystem-policy fixes found during enforced-mode qualification
End-to-end qualification on the qaf harness with
xpack.ml.trained_models.sandbox_enabled=true(realmode:enforcedroute, not the legacy fallback) surfaced several launch/filesystem-policy gaps that previously only manifested once a realpytorch_inferenceran under the enforced sandbox. Fixed here:$TMPDIR/ml-child-ipc/<child-id>(mode 0700) beforevalidateChildIpcLaunchSpec()'s liverealpath()calls, on both spawn paths. Elasticsearch only ever constructs the IPC path strings; nothing created the directory they name./run/elastic/ml-ipcremap), becausepytorch_inferencereceives its--input=/--output=/--restore=/--logPipe=argv as host paths under that root - a remap left them unresolvable in the sandbox mount namespace.legacyBpfAllowedSyscalls()alone is insufficient./procis mounted (PID-namespaced) inside the sandbox rootfs. Sandbox2 mounts a fresh PID-namespaced procfs on the outer root, thenpivot_roots into the chroot and detaches the old root, so the rootfs had no/proc. Without itreadlink(/proc/self/exe)fails withENOENT, breaking Intel oneMKL's runtime dispatcher (it reads/proc/self/exeto self-locate anddlopenits CPU-specificlibmkl_*.so.3kernels) - every enforcedpytorch_inferenceaborted withIntel oneMKL FATAL ERROR: Cannot load <mkl-loader>. The mount binds the already-namespaced procfs (never the host's): inside the sandbox/procshows only the sandboxee's own PIDs./sysis left unmounted.ml_sandbox_probenow checks/proc/self/exeis readable inside the real sandbox, asserted byCPytorchInferenceSandboxPolicyMechanismTest_Linux- turning the MKL crash into a build-time failure.Verified: ml-cpp sandbox unit tests 33/33; qaf boot check and
test_scenario_buildly(8/8) pass undermode:enforcedwith zero MKL crashes and no legacy fallbacks.