fix(docker): extract archives through os.Root, not a lexical path check (CodeQL 4–7) - #344
Merged
Conversation
Both tar.gz extractors validated containment by comparing strings: the joined path had to start with targetDir, and a symlink's target was resolved by joining it to the link's lexical parent. Entry by entry that looks airtight, and six single-step escape archives are indeed refused. A chain of two links is not. `sub/up -> ..` genuinely is the extraction root, so it is allowed — correctly. `sub/up/w -> ..` is then really `root/w`, pointing at the root's PARENT, while lexically it looks like `root/sub` and is allowed too. The following entry `sub/up/w/pwned` contains no "..", passes every check, and lands outside. Measured: a file written to the root's parent, and with one more link, into any sibling directory — in both extractors. Containment is now the kernel's job: os.OpenRoot plus Root.OpenFile / MkdirAll / Symlink, so an escaping path fails with "path escapes from parent" rather than being compared as text. The link-target check stays, with its parent resolved via EvalSymlinks, because os.Root stops mxcli following an outward link but not mxbuild or the JVM that read the tree afterwards. The two extractors were near-identical copies and are now one implementation with a strip1 flag, which is also why the alert appeared four times. Two side effects of the rewrite: - `strings.Contains(name, "..")` dropped legitimate files silently — "foo..bar" never appeared in the extracted tree, with no error. filepath.IsLocal tests path elements and is the right check. - the containment prefix test had no separator, so /cache/runtime-evil counted as inside /cache/runtime. Unreachable via filepath.Join, still the wrong test. Neither real archive contains a symlink at all — the 11.13.0 CDN tarballs are 3,646 files + 279 dirs (runtime) and 33,188 + 5,036 (mxbuild), no other entry type — so a refusal cannot break a download. Closes CodeQL alerts 4, 5, 6 and 7 (go/unsafe-unzip-symlink). Co-Authored-By: Claude Opus 5 <noreply@anthropic.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.
Closes CodeQL alerts 4, 5, 6 and 7 —
go/unsafe-unzip-symlink(high), all four incmd/mxcli/docker/download.go. Four alerts because there were two near-identical extractors (extractTarGz,extractTarGzStrip1) × two tainted sources (header.Name,header.Linkname).These are real
My first read was "probably a false positive" — the code already checks the joined path and resolves each symlink target. Six single-step escape archives confirmed that: absolute link target,
../../link target, absolute entry name,..in the name, link-to-root-then-write, link-to-dir-then-write. All correctly refused, nothing written outside.The seventh worked. Two chained symlinks escape, because the guard resolves a link against its lexical parent while the OS resolves it against its real one:
Measured against
main, in both extractors (strip1true and false):Extraction reports success. The archives come from
cdn.mendix.comover HTTPS, so this needs a compromised CDN or a MITM — but the extractor is the thing that is supposed to hold if that happens.The fix
Containment becomes the kernel's job:
os.OpenRoot+Root.OpenFile/Root.MkdirAll/Root.Symlink(Go 1.24/1.25; the repo is on 1.26). An escaping path now fails withopenat …: path escapes from parentinstead of being compared as text.The link-target check stays, with its parent resolved through
EvalSymlinksrather than joined lexically —os.Rootstops mxcli following an outward link, but not mxbuild or the JVM that read the extracted tree afterwards, so a hostile archive should not get to leave one in the cache either.The two extractors are now one implementation behind a
strip1flag.Would refusing break a real download? Measured, not assumed — the 11.13.0 CDN tarballs listed with
curl | tar -tzv:mendix-11.13.0.tar.gzarm64-mxbuild-11.13.0.tar.gzTwo bugs the rewrite removes on the way
strings.Contains(name, "..")matched a substring, so a legitimatefoo..barwas dropped from the extracted tree silently — no error, no warning.filepath.IsLocaltests path elements, and also covers absolute paths and Windows reserved names.HasPrefix(target, targetDir)with no separator, which puts/cache/runtime-evilinside/cache/runtime. Unreachable throughfilepath.Join, still the wrong test;pathWithinnow requires the separator and is asserted directly.Tests
cmd/mxcli/docker/extract_test.go— the escape archives, the six single-step controls, unit tests for the three new helpers, and an ordinary-archive control so that refusing everything cannot pass the suite. Escapes are detected by walking the whole sandbox for anything outside the extraction root, not by looking for expected filenames.Verified in both directions. Against the old extractor:
Symptom row appended to
.claude/skills/fix-issue.md.Stacked on nothing — branches off
main, independent of #343.🤖 Generated with Claude Code