fix(util): reject path traversal in all archive extractors#683
fix(util): reject path traversal in all archive extractors#683AruneshDwivedi wants to merge 2 commits into
Conversation
Only the zstd tar path validated that extracted entries stayed inside the destination; the gzip, xz, bzip2, zip and 7z extractors joined the entry name onto dest without checking, so an archive containing ../ entries could write outside the target directory. Add a shared containment check applied to every extractor. Signed-off-by: Arunesh Dwivedi <arunesh.devops@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens internal/shared/util archive extraction to prevent Zip Slip-style path traversal by ensuring extracted entry paths remain within the requested destination directory.
Changes:
- Adds a shared
ensureWithinDest(dest, target)containment check and applies it to gzip/xz/bzip2 tar, zip, and 7z extractors. - Introduces an XZ tar regression test that verifies
../path traversal entries are rejected and no file is written outside the destination.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/shared/util/decompressor.go | Adds ensureWithinDest and applies it across non-zstd extractors to block ../ path traversal during extraction. |
| internal/shared/util/decompressor_test.go | Adds an XZ tar path traversal regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rel, err := filepath.Rel(dest, target) | ||
| if err != nil { | ||
| return err | ||
| } |
| if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { | ||
| return fmt.Errorf("archive entry %q is outside destination", target) | ||
| } |
… relative to dest Signed-off-by: AruneshDwivedi <arunesh.dwivedi@users.noreply.github.com>
|
Addressed the first Copilot point: ensureWithinDest now returns the consistent "outside destination" error instead of leaking the raw filepath.Rel error when target cannot be made relative to dest (for example absolute target vs relative dest, or different volumes on Windows). Added a regression test (TestEnsureWithinDestWrapsRelError). The second point about pre-existing symlinks inside dest escaping via lexical checks is a deeper change (requires resolving symlinks in dest across all call sites) and is out of scope for this guard; happy to take it as a separate hardening PR if you want it. |
The zstd tar extractor validates that each entry stays inside the destination via safeZstdTarTarget, but the gzip, xz, bzip2, zip and 7z extractors join the entry name onto dest and write it without that check, so an archive with ../ entries can write files outside the target directory. This adds a shared containment check to every extractor and a regression test for the xz tar path.