fix: validate TEST_SRCDIR in SandboxStash to prevent path traversal outside sandboxExecRoot#29631
Conversation
d45079e to
fdba6e6
Compare
…ath traversal Add validateEnvPathComponent() to reject absolute paths and '..' traversal sequences in TEST_SRCDIR and TEST_WORKSPACE before they reach Path.getRelative() and renameTo() in the Bazel server JVM. Fixes bazelbuild#29476
|
Rebased on latest master and cleaned up the commit. The diff is now a single, minimal change:
The new No other code is touched. Happy to add tests if requested. |
|
Reopened — the PR was auto-closed during the rebase (branch momentarily had 0 commits ahead of master). The fix commit is now properly on top of latest master. Diff: +36/-1\ — view changes |
|
Hi @meteorcloudy @iancha1992 — This PR has an approved review and is ready to merge. It's a minimal security fix (+36/-1) that validates TEST_SRCDIR to prevent path traversal outside the sandbox. Could you help get this merged? Thanks! |
Fixes #29476
Summary
SandboxStash.getCurrentRunfilesDir() constructs a host filesystem path from action-environment values TEST_WORKSPACE and TEST_SRCDIR without validation:
java private static String getCurrentRunfilesDir(Map<String, String> environment) { return environment.get(TEST_WORKSPACE) + / + environment.get(TEST_SRCDIR); }The result is passed to Path.getRelative() and used as the destination of a
enameTo() call at SandboxStash.java:151. With sufficient ../ segments in TEST_SRCDIR, the destination resolves outside sandboxExecRoot.
Critically, the
enameTo() is performed by the Bazel server JVM, not the sandboxed child process. The sandbox isolates the child, but here the server itself performs the file operation using attacker-controlled env values.
This only triggers with --reuse_sandbox_directories which is default: true.
Root Cause
This is the same class of issue that motivated stripping TMPDIR in PosixLocalEnvProvider (#3296). The fix was partial -- TMPDIR was sanitized but TEST_SRCDIR (and TEST_WORKSPACE) were missed.
Fix
Added �alidateEnvPathComponent() that rejects:
Applied to both TEST_SRCDIR and TEST_WORKSPACE in getCurrentRunfilesDir() before the values reach getRelative().
Uses PathFragment.isAbsolute() and PathFragment.containsUplevelReferences() which are existing Bazel path utilities, ensuring consistency with the rest of the codebase.
Testing
The validation throws IllegalArgumentException for malicious values, which is caught by the existing IOException handler in akeStashedSandboxInternal() and stashSandboxInternal(), causing sandbox reuse to be safely disabled rather than allowing the traversal.
Reproduction from #29476:
python sh_test( name = exploit, srcs = [exploit.sh], env = {TEST_SRCDIR: ../../../../../../_stash_escape}, )Before fix: _stash_escape directory created outside sandbox by Bazel server JVM.
After fix: IllegalArgumentException thrown, sandbox reuse turned off, no escape.
Related
/cc @iancha1992