[ML] Typed filesystem/network launch policy for Sandbox2 pytorch_inference by valeriy42 · Pull Request #3185 · elastic/ml-cpp

valeriy42 · 2026-09-09T14:21:20Z

Stacks on #3182.

Replaces raw argument-directory inference in CPytorchInferenceSandboxPolicy with a typed launch spec: every input/output/restore/logPipe path is validated against a pinned child-root contract ($TMPDIR/ml-child-ipc/<child-id>) before any policy is built. Rejects relative, root-level, dot-dot, out-of-root, wrong-depth, duplicate, mutable-symlink/alias, and cross-option child-id-mismatch paths - never widens a mount to recover a rejected argument.

Also minimizes the filesystem policy: enumerates and justifies all seven historically bulk-mounted fixed directories, replaces whole /etc with five individually justified files, never binds host /proc//sys (relies on Sandbox2's own namespaced procfs/sysfs), uses a private bounded tmpfs at /tmp instead of the host's, and consumes the syscall allowlist already shared with the legacy BPF filter instead of hand-duplicating it.

Adds a purpose-built allowlisted mechanism probe (ml_sandbox_probe) proving allowed IPC access, denied host reads, denied external egress, loopback reachability, and mount enumeration, plus a portable validator unit-test suite and a Linux-only mechanism integration test.

Verified this session: the validator's core logic compiles clean with -Wall -Wextra -Werror and passes a standalone driver covering every rejection/acceptance path against real mkdtemp/mkdir/symlink fixtures. The SANDBOX2_AVAILABLE/Linux path compiles clean against stub sandbox2/seccomp headers (no vendored Sandbox2 headers available on this host). Not yet verified: an actual Sandbox2 run of the mechanism probe and the real Linux CMake/build integration - needs a Linux CI or devbox pass, in progress. Also fixes a Windows build break this change would otherwise have introduced (POSIX-only realpath/PATH_MAX used unconditionally in a file ml-cpp builds on every platform).

edsavage

LGTM

Just a few minor suggestions.

edsavage · 2026-09-16T03:48:13Z

+//! 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<SFixedMountDecision>& fixedMountDecisions();


This function would probably be better suited to be defined in the .cc file alongside the only caller of it (buildPytorchInferenceFilesystemPolicy())

Done in ab68235: EFixedMountAction, SFixedMountDecision, fixedMountDecisions(), and allowlistedEtcFiles() moved into the .cc anonymous namespace; the header now exposes only the validator types and buildPytorchInferenceFilesystemPolicy.

edsavage · 2026-09-16T03:49:34Z

+
+//! Individual /etc files pytorch_inference/libtorch are demonstrated to
+//! need, replacing a whole-/etc bind. Extend only with a named consumer.
+const std::vector<std::string>& allowlistedEtcFiles();


Same as fixedMountDecisions, this would be better suited to live in the .cc..

Also, if that were the case EFixedMountAction and SFixedMountDecision could also be moved there too.

Same as the sibling thread — mount decision tables are implementation details in CPytorchInferenceSandboxPolicy.cc now (ab68235).

edsavage · 2026-09-16T03:53:21Z

+    BOOST_REQUIRE_EQUAL(outcomeFor(resultsContent, "pid_namespace"), "namespaced");
+    BOOST_REQUIRE_EQUAL(outcomeFor(resultsContent, "loopback_reachable"), "ok");
+
+    ::unlink((childRoot + "/probe.txt").c_str());


I'm a bit concerned about these clean up steps getting missed if we hit assertion above. Maybe a RAII mechanism would be safer?

Done in ab68235: CMechanismProbeFixture RAII guard in the mechanism IT destructor cleans up probe.txt, results.txt, child root, ml-child-ipc, and the mkdtemp dir even when BOOST_REQUIRE_* aborts.

edsavage · 2026-09-16T03:57:04Z

+    // 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 -
+    // an open item, not resolved here.


Should we track this in a separate issue?

Opened #3200 to track verifying the distro-specific TLS trust-bundle path; the in-code NOTE in allowlistedEtcFiles() links it (ab68235). Not resolved in this PR.

Copilot

🟡 Changes recommended

Unresolved policy validation, path parsing, syscall, and mechanism-test issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a typed Sandbox2 launch policy for PyTorch inference, with minimized mounts and validation tests.

Changes:

Validates per-child IPC paths.
Minimizes filesystem and syscall policy configuration.
Adds validator and Linux mechanism-probe tests.

File summaries

File	Description
`include/sandbox/CPytorchInferenceSandboxPolicy.h`	Defines launch-spec and policy APIs.
`lib/sandbox/CPytorchInferenceSandboxPolicy.cc`	Implements path validation and policy construction.
`lib/sandbox/CMakeLists.txt`	Builds the policy implementation.
`lib/sandbox/unittest/CMakeLists.txt`	Registers test targets.
`lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc`	Tests path validation behavior.
`lib/sandbox/unittest/CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc`	Tests Sandbox2 enforcement.
`lib/sandbox/unittest/payloads/ml_sandbox_probe.cc`	Provides the sandbox mechanism probe.

Review details

Suppressed comments (7)

lib/sandbox/CPytorchInferenceSandboxPolicy.cc:170

splitPathComponents drops a trailing slash, and . remains a component, so --input=<childRoot>/ and --input=<childRoot>/. are accepted even though they name the child directory rather than a single pipe leaf. This violates the documented exact-depth contract and can synthesize a different s_PipePaths entry than the argument. Reject a trailing slash and leaf == ".".

        const std::string leaf{components.back()};
        const std::size_t lastSlash = value.rfind('/');
        const std::string literalParent{value.substr(0, lastSlash)};

lib/sandbox/CPytorchInferenceSandboxPolicy.cc:40

logProperties is also a filesystem path: pytorch_inference/Main.cc passes it to CLogger::reconfigure, which reads the file, but this recognizer treats it as an ignored scalar. A sandboxed launch can therefore receive an unvalidated config-file path (or fail startup because the normal config is not mounted). Explicitly reject/strip it for this route or include it in the typed path contract and mapping.

bool isPathOptionName(const std::string& name) {
    return name == "input" || name == "output" || name == "restore" || name == "logPipe";

lib/sandbox/unittest/CPytorchInferenceSandboxPolicyMechanismTest_Linux.cc:142

This test-only grant changes the policy under test: the production builder consumes the shared allowlist, which contains connect but not socket, so a real child cannot create the sockets used by this probe. The test can therefore pass while the production policy still kills the first socket call. Add the required socket syscall(s) to the production/shared policy or remove the socket-based positive checks; do not grant them only here.

    policyBuilder.AllowSyscall(__NR_socket);

lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc:145

This test does not actually exercise E_OutsideTrustedBase: the selected parent /var/tmp/not-under-tmpdir normally does not exist, so the validator returns E_CanonicalizationFailed and the test accepts that result. Create an existing directory outside the fixture's canonical base and require E_OutsideTrustedBase so the out-of-root check is covered rather than only the missing-parent case.

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"})};

lib/sandbox/unittest/payloads/ml_sandbox_probe.cc:142

The PID value only demonstrates a private PID namespace; it does not demonstrate that /proc is not a host bind. A process can have PID 1 in a private namespace while a host /proc is mounted over /proc, exposing host process entries. Add a check that distinguishes the namespace's procfs from the host procfs, rather than relying on getpid() alone.

    // 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()));

lib/sandbox/unittest/payloads/ml_sandbox_probe.cc:166

A failure with ENETUNREACH or EHOSTUNREACH for one TEST-NET address is not proof that all external egress is disabled; a namespace with a route plus a destination-specific reject can produce the same errors while other destinations remain reachable. Use a controlled endpoint or another check that exercises the namespace's actual route isolation.

        const bool denied = rc != 0 && (connectErrno == ENETUNREACH ||
                                        connectErrno == EHOSTUNREACH);
        report("external_egress", denied ? "denied" : "allowed", std::strerror(connectErrno));

lib/sandbox/unittest/payloads/ml_sandbox_probe.cc:101

An open failure with ENOENT is reported as denied, so the integration test passes on an image that simply has no /etc/shadow; it does not prove that a present host file was blocked. Treat missing as a distinct outcome (or precondition that the sentinel exists) and require an actual access denial.

    int shadowFd = ::open("/etc/shadow", O_RDONLY);
    if (shadowFd < 0) {
        report("host_read_etc_shadow", "denied", std::strerror(errno));

Files reviewed: 7/7 changed files
Comments generated: 4
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

valeriy42 · 2026-09-18T11:48:59Z

+    // path. spec must already be s_Ok (validateChildIpcLaunchSpec), so
+    // s_ChildIpcRoot is exactly $TMPDIR/ml-child-ipc/<child-id> - never
+    // ml-child-ipc itself, never a sibling child's directory.
+    policyBuilder.AddDirectoryAt(spec.s_ChildIpcRoot, "/run/elastic/ml-ipc", /*is_ro=*/false);


Implemented in ab68235: buildPytorchInferenceFilesystemPolicy now takes const SChildIpcValidationResult&; on s_Ok == false or a non-ml-child-ipc s_ChildIpcRoot it SetErrors and skips AddDirectoryAt.

valeriy42 · 2026-09-18T11:49:01Z

+        if (eqPos == std::string::npos) {
+            // NOTE (reviewed, not fixed): CCmdLineParser.cc's
+            // boost::program_options parser also accepts spellings other
+            // than the exact concatenated "--<name>=<value>" form this loop
+            // requires - a space-separated "--input /path", or (via boost's
+            // default allow_guessing style) an unambiguous abbreviation
+            // like "--inp=/path". None of those are a mount-widening bypass:
+            // an unrecognized option is never added to s_PipePaths, so its
+            // directory is simply never mounted and the spawn either fails
+            // closed (pipe unreachable) or gets rejected elsewhere. The sole
+            // production caller, ProcessPipes.addArgs() in
+            // elasticsearch/x-pack/plugin/ml, always emits the exact
+            // concatenated "--input=" + value form, so this is a defensive
+            // fail-closed gap rather than an active exploit path. Left
+            // unfixed rather than special-cased.
+            continue;


Declining this PR: the fail-closed NOTE at CPytorchInferenceSandboxPolicy.cc documents that only --name=value tokens are parsed; production ProcessPipes emits that form and unrecognized spellings are never mounted.

valeriy42 · 2026-09-18T11:49:02Z

+    // plus "." and "..", never a full directory bind. A regression back to
+    // AddDirectory("/etc", true) would spike this into the dozens/hundreds,
+    // so an upper bound catches it without hard-coding the exact count.
+    BOOST_REQUIRE_EQUAL(outcomeFor(resultsContent, "etc_enumeration"), "counted");
+    BOOST_TEST_REQUIRE(std::stoi(detailFor(resultsContent, "etc_enumeration")) <= 10);


Declining this PR: the <= 10 upper bound still catches a regression to whole-/etc bind (dozens/hundreds of entries); tightening to an exact allowlist count can follow once the probe contract is frozen.

valeriy42 · 2026-09-18T11:49:04Z

+    struct statfs tmpStatfs {};
+    const bool isTmpfs = ::statfs("/tmp", &tmpStatfs) == 0 && tmpStatfs.f_type == TMPFS_MAGIC;


Declining this PR: TMPFS_MAGIC assertion is not added here; private tmpfs behavior is already covered by the private_tmpfs_write mechanism outcome in the probe IT.

elasticsearchmachine · 2026-09-18T07:09:45Z

Pinging @elastic/ml-core (Team:ML)

…eanup Skip __NR_futex in the legacy syscall dump so AllowFutexOp remains the Sandbox2 grant. Builder now takes SChildIpcValidationResult and refuses to mount when s_Ok is false or s_ChildIpcRoot is not ml-child-ipc shaped. Move fixed-mount tables into the .cc; add RAII cleanup in the mechanism IT. Track distro TLS bundle path in #3200.

…rence Replaces raw argument-directory inference with a typed launch spec that validates every input/output/restore/logPipe path against a pinned child-root contract before any policy is built: each must canonicalize to exactly $TMPDIR/ml-child-ipc/<child-id>/<leaf>, for one consistent <child-id>. Rejects relative, root-level, dot-dot, out-of-root, wrong-depth, duplicate, and mutable-symlink/alias paths - never widens a mount to recover a rejected argument. Minimizes the filesystem policy: enumerates and justifies all seven historically bulk-mounted fixed directories (/lib /lib64 /usr/lib /usr/lib64 /etc /proc /sys), each mounted only if its source actually exists on this host; replaces whole /etc with five individually justified files; never binds host /proc or /sys (relies on Sandbox2's own namespaced procfs/sysfs); uses a private bounded tmpfs at /tmp instead of the host's; consumes the syscall allowlist already shared with the legacy BPF filter instead of hand-duplicating it. Adds a purpose-built allowlisted mechanism probe proving allowed IPC access, denied host reads, denied external egress (narrowed to the actual no-route errno class), loopback reachability, and mount enumeration, plus a portable validator unit-test suite that runs on every POSIX ml-cpp CI platform without needing Sandbox2 itself, and a Linux-only mechanism integration test. Also fixes a Windows build break this change would otherwise have introduced: the new production file used POSIX-only realpath()/PATH_MAX unconditionally, but ml-cpp builds this library on every platform including Windows. canonicalize() now has a _WIN32 branch using _fullpath()/_MAX_PATH (inert until any Windows caller exists); the POSIX-only unit test is excluded from the Windows build instead. Verified this session: the validator's core logic compiles clean with -Wall -Wextra -Werror and passes a standalone driver covering every rejection/acceptance path (valid multi-pipe case, empty value, relative, root-level, dot-dot, too-shallow, too-deep, duplicate, symlink-alias, child-id-mismatch, scalar-options-ignored) against real mkdtemp/mkdir/symlink fixtures. The SANDBOX2_AVAILABLE/Linux PolicyBuilder path compiles clean with -Werror against stub sandbox2/ seccomp headers (no vendored Sandbox2 headers available on this host). Not yet verified: an actual Sandbox2 run of the mechanism probe and the real Linux CMake/build integration - needs a Linux CI or devbox pass. The /etc/ssl trust-bundle path is deliberately left out of the allowlisted /etc files pending confirmation of the actual path on the CI build image (Debian-style vs RHEL-style).

…eanup Skip __NR_futex in the legacy syscall dump so AllowFutexOp remains the Sandbox2 grant. Builder now takes SChildIpcValidationResult and refuses to mount when s_Ok is false or s_ChildIpcRoot is not ml-child-ipc shaped. Move fixed-mount tables into the .cc; add RAII cleanup in the mechanism IT. Track distro TLS bundle path in #3200.

…ename.

Retarget legacyBpfAllowedSyscalls() to ml::seccomp after PR B rename. Return absl::StatusOr<PolicyBuilder> instead of calling private SetError.

valeriy42 added this pull request to stack #3183 September 9, 2026 14:21

valeriy42 force-pushed the feature/sandbox2-pr-c-fs-net-policy branch from f85396c to ca20bda Compare September 9, 2026 19:23

valeriy42 marked this pull request as ready for review September 10, 2026 08:24

edsavage approved these changes Sep 16, 2026

View reviewed changes

edsavage requested a lite review from Copilot September 16, 2026 04:05

Copilot started reviewing on behalf of edsavage September 16, 2026 04:06 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

valeriy42 force-pushed the feature/sandbox2-pr-c-fs-net-policy branch from ca20bda to 33d32fc Compare September 18, 2026 07:04

valeriy42 added :ml v9.6.0 >non-issue labels Sep 18, 2026

valeriy42 force-pushed the feature/sandbox2-pr-c-fs-net-policy branch from 33d32fc to 126277b Compare September 18, 2026 08:24

valeriy42 mentioned this pull request Sep 18, 2026

Sandbox2 pytorch_inference: verify TLS trust-bundle path per supported distro #3200

Open

valeriy42 added 3 commits September 18, 2026 15:44

Retarget sandbox policy includes after CMlLegacyBpfSyscallAllowlist r…

8b70229

…ename.

valeriy42 force-pushed the feature/sandbox2-pr-c-fs-net-policy branch from ab68235 to 8b70229 Compare September 18, 2026 14:37

Fix Linux compile: allowlist namespace and StatusOr policy builder.

7268dd9

Retarget legacyBpfAllowedSyscalls() to ml::seccomp after PR B rename. Return absl::StatusOr<PolicyBuilder> instead of calling private SetError.

		struct statfs tmpStatfs {};
		const bool isTmpfs = ::statfs("/tmp", &tmpStatfs) == 0 && tmpStatfs.f_type == TMPFS_MAGIC;

Conversation

valeriy42 commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edsavage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (7)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

valeriy42 commented Sep 9, 2026 •

edited

Loading