fix(archive): validate GNU tar metadata - #838
Conversation
Read GNU long-name and long-link records during the raw tar scan so it validates the same paths used by the extractor. Reject escaping targets and writes through hidden symlink names while keeping safe GNU metadata compatible.
|
@rustytrees Thanks for the contribution! One small request: could you please instruct your agent to follow the PR template provided in the repository for PRs? It helps keep the PRs consistent and easier to review. Thanks! |
|
I published a runnable PoC for the GNU metadata mismatch fixed here: git clone https://github.com/rustytrees/malt-security-pocs.git
cd malt-security-pocs
git checkout bd0e2a4d088cc63c8a7ab1613e851d23808b5a4d
./scripts/run-poc.sh --malt /path/to/malt --expect vulnerable h01The PoC combines GNU Exact |
There was a problem hiding this comment.
@rustytrees Thanks - I verified the premise and the fix. Zig 0.16's iterator really does apply GNU L/K (std/tar.zig, the .gnu_long_name / .gnu_long_link arms), and on main a GNU-renamed symlink hides an escape from the pre-scan: the payload gets written outside the extraction root. With this branch that exact case is refused. The reading of the pax-vs-GNU override semantics is correct too.
One gap before this closes the issue.
The override lifetime still diverges from std.tar
The pre-scan resets override_name / override_link after every non-prefix entry. std.tar does not: its else arm, the one that handles unsupported header types, logs to diagnostics and continues the loop without resetting its File accumulator. So a pending L/K record carries to the next entry there, but not here. Slip one unsupported entry between the metadata record and the real entry and the two disagree about which entry the long name belongs to.
Against this branch:
L "door"
entry("decoy", '3', "") # character device: unsupported by std.tar, ignored by the pre-scan
entry("placeholder", '2', "../outside") # std.tar names this "door"; the pre-scan records "placeholder"
file("door/escaped", "owned") # pre-scan sees no "door" symlink, so it allows this
=> /outside/escaped is written
A hard link ('1') works as the intervening entry too, which matters because extractTarGz deliberately tolerates those. The control - same archive without the GNU record - is correctly blocked, so it is the carry-over that does it, not something else in the shape.
The fix is small and matches what this PR already set out to do: reset the overrides only on the kinds std.tar actually returns (directory, normal, symlink) and leave them pending across unsupported kinds, mirroring the accumulator exactly.
One thing worth a comment in the code
While reading this I noticed pipeToFileSystem runs to completion before the diagnostics loop is checked, so an archive that ultimately returns ExtractionFailed has already written its payload to disk. That makes the pre-scan the only real defence rather than a fast-fail optimisation, which is worth stating in the doc comment on preScanTarGz so nobody later assumes the diagnostics check is a backstop.
This is a strict improvement as it stands and does not regress anything, so I am happy either way: extend it here, or merge as-is and open a follow-up. Tell me which you prefer.
Note this needs a rebase on top of #837 - the two both rewrite preScanTarGz and conflict.
Description
The tar pre-scan now reads GNU long-name and long-link records before validating the following entry. It checks the same effective name and link target that the extractor uses.
Regression tests cover an escaping long-link target, a hidden symlink name followed by an outside write, and safe GNU metadata.
Related Issue
Closes #854.
Notes for Reviewers
Verification completed:
zig build test-one: 2,427 passedzig build test: 5,062 passed, 3 skippedgit verify-commit