Reject path traversal in mount destinations during jail staging - #292
Open
M0nd0R wants to merge 2 commits into
Open
Reject path traversal in mount destinations during jail staging#292M0nd0R wants to merge 2 commits into
M0nd0R wants to merge 2 commits into
Conversation
Mount destinations were joined as newroot+"/"+dst and walked with mkdirat/openat without rejecting "."/".." components or following symlinks safely. An attacker who can influence mount dst (CLI, config, or prefix_dst_env) could create directories, symlinks, or mounts outside the intended staging root on the host before pivot_root. Add isSafeContainmentPath(), validate destinations in both legacy and new mount APIs, use O_NOFOLLOW while walking parents, and include a standalone regression test.
PR google#292 rejected '.'/'..' and added O_NOFOLLOW on the legacy createDirRecursively walk, but mnt_newapi createDirAt still used multi-component mkdirat(), which follows intermediate symlinks. After a -s symlink mount pointing outside the staging root, later mounts with destinations under that symlink (e.g. link/pwned) could create directories or attach mounts on the host before pivot_root. Walk destinations one component at a time with O_NOFOLLOW, resolve parent fd + basename for leaf symlinkat/move_mount/open_tree/openat, and use AT_SYMLINK_NOFOLLOW in existsAs*At helpers.
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
1 participant
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
Mount destinations are joined as
newroot + "/" + dstand walked withmkdirat/openatwithout rejecting./..components (and withoutO_NOFOLLOW). An attacker who can influence mountdstvia CLI, config, orprefix_dst_envcan create directories, symlinks, or mounts outside the intended staging root on the host beforepivot_root.This violates the intended invariant that destinations stay under the jail staging tree.
Changes
util::isSafeContainmentPath()to reject.,.., and embedded NULcreateDirRecursively()to refuse unsafe paths and useO_NOFOLLOWmnt_legacy.cc) and new (mnt_newapi.cc) mount APIstests/path_containment_test.ccTest plan
g++ -std=c++20 -O1 -o path_containment_test tests/path_containment_test.cc && ./path_containment_test→OK path_containment_test passed-R,-B,-Twith safe paths) still work-T '/tmp/../../../../tmp/evil'PoC (pre-patch behavior)
createDirRecursively("/staging/root/../../escaped/pwned/x")created directories outside the staging prefix by following..viaopenat.