fix(security): extract archives through an os.Root handle - #196
Merged
Conversation
The three tarball extractors (cookbook install, policyfile cookbook fetch, and the ruby.wasm loader) each checked containment with filepath.Rel before writing an entry. That check is lexical: it reasons about the entry name as a string, and a name like "nginx/cache/evil.rb" is provably inside the destination by every string comparison we can make. The write, though, happens against the filesystem. If the destination already holds a "cache" symlink pointing elsewhere, the create follows it and the entry lands outside the destination that was just validated. The same gap covers the TOCTOU case, where the symlink appears between the check and the write. Open the destination as an os.Root and route every MkdirAll and OpenFile through that handle. Containment is then enforced per path component when the file is opened, so an escape fails at the syscall regardless of what the name looked like. The lexical check stays, since it still produces the better error message, naming the offending archive entry. No caller can reach this today: all three destinations are freshly created directories (os.MkdirTemp, or RemoveAll followed by MkdirAll), so there is no pre-existing symlink to follow, and all three extractors already skip symlink and hardlink entries so an archive cannot plant one mid-stream. This removes the unwritten "callers must pass a fresh directory" invariant that was holding that safety up, and clears the five go-path-traversal code scanning alerts, which flag the filepath.Rel guard because CodeQL does not model it as a sanitizer. Each extractor gets a test that plants a symlink in the destination and asserts the entry does not escape through it. All three fail before this change. Signed-off-by: Tim Smith <tim@mondoo.com>
The entry name is already relative by the time it is validated, so joining
it onto the destination and relativizing it straight back off was a round
trip that computed the same string it started with.
Check the relative path directly instead. Same rejection semantics, with
one difference that only tightens things: an absolute entry name is now
refused outright rather than being silently rewritten to sit under the
destination, which is what os.Root does with it anyway.
This also drops the filepath.Join call that the xgrep scanner matched on.
The suggested replacement in that alert, strings.HasPrefix against a
canonical base, is the weaker check: it accepts a sibling directory sharing
a prefix ("/tmp/foobar/evil" passes a "/tmp/foo" prefix test), so it is not
adopted here.
Signed-off-by: Tim Smith <tim@mondoo.com>
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.
Addresses the open
go-path-traversalcode scanning alerts (#10, #11, #12, #16, #21).What the scanner found
Five alerts across the three tarball extractors:
cli/cookbook/extract.go(cookbook install)cli/policyfile/extract.go(policyfile cookbook fetch)cli/policyfile/rubyeval/loader.go(ruby.wasm loader)Each site already guarded containment with
filepath.Rel, which CodeQL does not model as a sanitizer. So the immediate question was whether these are real.What is actually wrong
The guards are correct for what they check, and the existing
../tests still pass. But the check is lexical: it reasons about the entry name as a string. An entry likenginx/cache/evil.rbis provably inside the destination by every string comparison available.The write happens against the filesystem. If the destination already holds a
cachesymlink pointing elsewhere, the create follows it and the entry lands outside the directory that was just validated. The same gap covers the TOCTOU case, where a symlink appears between the check and the write.Is it exploitable today?
No. All three destinations are freshly created directories (
os.MkdirTemp, orRemoveAllfollowed byMkdirAll), so there is no pre-existing symlink to follow, and all three extractors skip symlink and hardlink entries, so a hostile archive cannot plant one mid-stream.That safety rests entirely on an unwritten invariant: every caller must pass a fresh directory. Nothing in the extractors enforces it, and nothing warns a future caller who reuses a directory. This change removes the invariant rather than documenting it.
The change
Open the destination as an
os.Rootand route everyMkdirAllandOpenFilethrough that handle. Containment is enforced per path component at open time, so an escape fails at the syscall no matter what the name looked like.The lexical check stays: it still produces the better error message, naming the offending archive entry, and it rejects bad input before any I/O.
Tests
Each extractor gets a test that plants a symlink in the destination and asserts the entry does not escape through it. All three fail before this change:
go build ./...,go vet ./..., andgofmtare clean.cli/cookbook,cli/policyfile, andcli/policyfile/rubyevalall pass, including the full rubyeval suite that exercises real ruby.wasm extraction through the changed path.Not covered here
Two other path traversal alerts on the security tab are not addressed, because neither is a defect:
ruby-path-traversal, critical) incli/policyfile/rubyeval/generate_goldens.rb. A developer-only golden generator that is never compiled into the binary. The flaggedFile.read(policyfile_path)reads paths produced byDir.globover a fixedtestdatadirectory, with no external input. Recommend dismissing as a false positive rather than contorting a dev script to satisfy the scanner.cli/policyfile/fetch.go(a git source'spathescaping the clone) and is unrelated to these.