Repository navigation
mnt: fail closed on untrusted workdir candidates - #381
Open
krishna28238-arch wants to merge 1 commit into
Open
krishna28238-arch wants to merge 1 commit into
krishna28238-arch wants to merge 1 commit into
Conversation
findWorkDir() accepted any pre-existing entry at a candidate path as long as access(R_OK) succeeded: a symlink or a foreign-owned directory planted in a shared parent such as /tmp was silently selected as the staging root, and both mount backends then attached the jail-root tmpfs to it through path-based operations that follow the planted entry. Verify every candidate with a new util::openVerifiedDir() helper: only a real directory (O_NOFOLLOW) owned by the current euid is accepted, and the verified fd is returned to the callers. The new-API backend attaches the detached root tmpfs with move_mount() against that fd, and the legacy backend mounts its staging trees through /proc/self/fd handles, so the attach targets the verified inode instead of re-resolving the path.
This branch has not been deployed
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
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.
While auditing how nsjail picks its staging directories, I found a seam the recent hardening work in this area doesn't cover:
findWorkDir()can select and attach the jail staging root onto an entry a local attacker planted first. This PR makes that selection fail closed.The problem
findWorkDir()builds candidates like/tmp/nsjail.<uid>.<purpose>under world-writable parents (/tmp,/dev/shm, writable dirs found under/).tryCreateDir()accepts a pre-existing entry whenevermkdir()reportsEEXIST— regardless of who owns it or whether it is a symlink — and every caller then attaches the staging tmpfs by path:mount(2)in the legacy backend,move_mount(2)plus two lateropenat()re-opens in the newapi backend. A path-based attach takes whatever entry currently sits at that location.So on a shared multi-user box, a local user who creates
/tmp/nsjail.0.root(a directory they own, or a symlink into their home) before root runs nsjail gets the staging root — and the populated jail root tree — mounted onto their own directory, with the candidate names fully predictable from uid and purpose. This is the same class as #323 (log file opened through a symlink), one layer above what the other PRs in this arc hardened: #292 rejects traversal in the destination strings inside the staging tree, #325 (open) anchors legacy mount destinations to a verified parent, and #334 made flag application fail closed — but the staging root itself was still chosen by an unverified path. #287 explored a descriptor-confined resolution for destinations and was closed; this applies the descriptor idea to the staging root selection itself.What this PR does
util::openVerifiedDir(): creates or reuses a directory and returns anO_DIRECTORY|O_CLOEXEC|O_NOFOLLOWfd only afterfstatconfirms the inode is owned by the current euid. Symlinks, regular files and foreign-owned directories are refused.findWorkDir()validates every candidate through it and skips a refused candidate (fail closed, in the spirit of mnt: fail closed when explicit security mount flags cannot be applied #334); the verified fd travels back to the caller so nothing re-resolves the path afterwards.move_mount()attaches the jail root onto the verified fd itself (MOVE_MOUNT_T_EMPTY_PATH), and the twoopenat()re-opens of the staging root gainedO_NOFOLLOW./proc/self/fd/<fd>, pinning the verified inode instead of re-walking/tmp.Before / after
/tmp/nsjail.<uid>.rootO_NOFOLLOW)Testing
makebuilds clean (with-Werror) on top of master (d8415bc)./tmp/nsjail.<uid>.rootand/tmp/nsjail.<uid>.tmp(old code accepts the paths and then dies at mount), nsjail now skips the poisoned candidates, falls back to the/dev/shmcandidate, builds the mount tree and runs the payload inside the jail normally.One note for review: my end-to-end run exercised the legacy backend (the test container has no
move_mount, soisAvailable()selects legacy). The newapi fd-target path is compile-checked and its fd logic is covered by the unit test; a native run there would be a useful second opinion.The diff is +101/−16 over six files, most of it the helper and the two attach sites. Happy to reshape or split anything.