[ML] Typed filesystem/network launch policy for Sandbox2 pytorch_inference

[valeriy42]

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]

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

[edsavage]

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.

[edsavage]

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

[edsavage]

Should we track this in a separate issue?

[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.