Unpack, UnpackLayer: treat tar entry names as POSIX paths - #63
Draft
thaJeztah wants to merge 4 commits into
Draft
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63 +/- ##
==========================================
- Coverage 65.75% 65.09% -0.67%
==========================================
Files 42 42
Lines 2038 2051 +13
==========================================
- Hits 1340 1335 -5
- Misses 528 541 +13
- Partials 170 175 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // #nosec G305 -- The joined path is checked for path traversal. | ||
| dstPath := filepath.Join(dest, hdr.Name) | ||
| dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name)) |
thaJeztah
marked this pull request as ready for review
July 21, 2026 23:19
| // Strip a leading "/" so absolute entries stay root-relative, and | ||
| // normalize the POSIX tar path. Skip entries referring to the extraction | ||
| // root and reject paths that escape it. | ||
| name := path.Clean(strings.TrimLeft(hdr.Name, "/")) |
Member
Author
|
CodeQL issues will be fixed in follow-up(s) |
There was a problem hiding this comment.
Pull request overview
This PR updates archive extraction to treat tar header entry names as POSIX paths (using path.Clean), then converts to native paths only at the filesystem boundary, improving cross-platform correctness (notably Windows) and strengthening path traversal protection by rejecting non-local entries via filepath.IsLocal.
Changes:
- Normalize tar header names with
path.Clean(POSIX semantics) in bothUnpackandUnpackLayer. - Reject non-local (potentially escaping) entry names using
filepath.IsLocalafter normalization. - Convert POSIX paths to native paths only when constructing filesystem paths (
filepath.FromSlash), and usepath.Base/path.Cleanfor tar-metadata operations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| archive.go | Updates Unpack to normalize tar entry names as POSIX paths, apply IsLocal validation, and defer native-path conversion until joining paths for extraction. |
| diff.go | Updates UnpackLayer to normalize and validate POSIX tar entry names, and to use POSIX path helpers when operating on tar metadata (e.g., AUFS hardlink handling). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 task
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
diff.go:121
- Path traversal guard misses the case where filepath.Rel returns exactly ".." (e.g., a tar entry name of ".." or "../"). In that case the current prefix-check does not trigger, and extraction can escape dest by one directory.
// #nosec G305 -- The joined path is guarded against path traversal.
dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name))
rel, err := filepath.Rel(dest, dstPath)
if err != nil {
return 0, err
}
// Note as these operations are platform specific, so must the slash be.
if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
return 0, breakoutError(fmt.Errorf("%q is outside of %q", hdr.Name, dest))
}
archive.go:863
- Path traversal guard misses the case where filepath.Rel returns exactly ".." (e.g., a tar entry name of ".." or "../"). In that case the current prefix-check does not trigger, and extraction can escape dest by one directory.
// #nosec G305 -- The joined path is checked for path traversal.
dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name))
rel, err := filepath.Rel(dest, dstPath)
if err != nil {
return err
}
if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
return breakoutError(fmt.Errorf("%q is outside of %q", hdr.Name, dest))
}
Comment on lines
93
to
95
| if strings.HasPrefix(hdr.Name, WhiteoutLinkDir) && hdr.Typeflag == tar.TypeReg { | ||
| basename := filepath.Base(hdr.Name) | ||
| basename := path.Base(hdr.Name) | ||
| aufsHardlinks[basename] = hdr |
Comment on lines
+835
to
+847
| // Strip a leading "/" so absolute entries stay root-relative, and | ||
| // normalize the POSIX tar path. Skip entries referring to the extraction | ||
| // root and reject paths that escape it. | ||
| name := path.Clean(strings.TrimLeft(hdr.Name, "/")) | ||
| if name == "." { | ||
| continue | ||
| } | ||
| for _, exclude := range options.ExcludePatterns { | ||
| if strings.HasPrefix(hdr.Name, exclude) { | ||
| if strings.HasPrefix(name, exclude) { | ||
| continue loop | ||
| } | ||
| } | ||
| hdr.Name = name |
Normalize tar entry names with path.Clean instead of filepath.Clean.
Tar header paths always use POSIX ('/') separators. Preserve their POSIX
form while operating on archive metadata, deferring conversion to native
paths until the filesystem boundary.
This fixes a bug where ExcludePatterns could fail to match valid archive
entries on Windows because filepath.Clean converted tar paths to native
path separators before exclusion matching.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Normalize tar entry names with path.Clean instead of filepath.Clean.
Tar header paths always use POSIX ('/') separators. Preserve their POSIX
form while operating on archive metadata, deferring conversion to native
paths until the filesystem boundary.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
diff.go:116
- Path traversal guard misses the case where filepath.Rel returns exactly ".." (e.g., tar entry name ".." or "../"). That would allow writing to dest's parent because the current check only matches ".." followed by a path separator.
dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name))
rel, err := filepath.Rel(dest, dstPath)
if err != nil {
return 0, err
}
archive.go:842
- The new POSIX normalization + exclusion matching behavior isn’t covered by a unit test. Consider adding a test tar stream containing a nested header name like "dir/file" and verifying ExcludePatterns (e.g., "dir") still excludes it on all platforms (this was the reported Windows regression).
name := path.Clean(strings.TrimLeft(hdr.Name, "/"))
if name == "." {
continue
}
for _, exclude := range options.ExcludePatterns {
Comment on lines
855
to
859
| // #nosec G305 -- The joined path is checked for path traversal. | ||
| dstPath := filepath.Join(dest, hdr.Name) | ||
| dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name)) | ||
| rel, err := filepath.Rel(dest, dstPath) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
archive.go:859
- The path-traversal guard below misses the case where filepath.Rel(dest, dstPath) returns exactly ".." (e.g., a tar entry named ".."), which can escape the extraction root without triggering the current HasPrefix check. Consider rejecting rel == ".." in addition to ".."+PathSeparator.
// #nosec G305 -- The joined path is checked for path traversal.
dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name))
rel, err := filepath.Rel(dest, dstPath)
if err != nil {
return err
Comment on lines
111
to
115
| // #nosec G305 -- The joined path is guarded against path traversal. | ||
| dstPath := filepath.Join(dest, hdr.Name) | ||
| dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name)) | ||
| rel, err := filepath.Rel(dest, dstPath) | ||
| if err != nil { | ||
| return 0, err |
Use filepath.Localize when converting AUFS hardlink basenames from tar header paths to native filesystem paths. This validates that the basename is a single native path component before it is passed to filepath.Join(), avoiding reinterpretation of Windows path separators. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
| defer os.RemoveAll(aufsTempdir) | ||
| } | ||
| if err := createTarFile(filepath.Join(aufsTempdir, basename), dest, hdr, tr, options); err != nil { | ||
| if err := createTarFile(filepath.Join(aufsTempdir, localBasename), dest, hdr, tr, options); err != nil { |
Only treat hardlinks targeting entries inside /.wh..wh.plnk as AUFS metadata by requiring a path-component boundary after the directory name. This avoids matching similarly prefixed paths while preserving existing handling of AUFS metadata entries and opaque whiteouts. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
archive.go:851
- createImpliedDirectories is called before verifying that the entry stays within dest. For a crafted name like "../newdir/file", this can create directories outside dest (via filepath.Join(dest, parent)) even though the later Rel() check will reject the entry. Also, the Rel() guard should reject rel == "..". Consider validating the destination path first, then creating implied directories only after the breakout check passes.
hdr.Name = name
// Ensure that the parent directory exists.
err = createImpliedDirectories(dest, hdr, options)
if err != nil {
Comment on lines
+52
to
+59
| // Strip any leading "/" so absolute entries stay root-relative, and | ||
| // normalize the POSIX tar path. Skip entries referring to the extraction | ||
| // root and reject paths that escape it. | ||
| name := path.Clean(strings.TrimLeft(hdr.Name, "/")) | ||
| if name == "." { | ||
| continue | ||
| } | ||
| hdr.Name = name |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
diff.go:59
- The new POSIX normalization doesn’t actually reject paths that escape the extraction root (e.g., ".." or "../..."). Because createImpliedDirectories runs before the filepath.Rel breakout check, a crafted entry could still write outside dest (and the later rel-prefix check misses the exact ".." case). Add an explicit guard on the cleaned POSIX name before assigning hdr.Name.
name := path.Clean(strings.TrimLeft(hdr.Name, "/"))
if name == "." {
continue
}
hdr.Name = name
archive.go:841
- The new POSIX normalization doesn’t actually reject paths that escape the extraction root (e.g., ".." or "../..."). Because createImpliedDirectories runs before the filepath.Rel breakout check, a crafted entry could still create/write outside dest. Add an explicit guard on the cleaned POSIX name before the exclude-pattern loop.
name := path.Clean(strings.TrimLeft(hdr.Name, "/"))
if name == "." {
continue
}
archive.go:845
- This change is intended to fix ExcludePatterns mismatches on Windows by preserving POSIX separators during archive-metadata operations, but there isn’t a regression test covering a nested path exclusion like "dir/sub" on Windows (where filepath.Clean previously rewrote "/" to "\"). Adding a Windows-focused test would prevent reintroducing the bug.
for _, exclude := range options.ExcludePatterns {
if strings.HasPrefix(name, exclude) {
continue loop
}
diff.go:118
- The breakout/path-traversal check below misses the case where rel == ".." (no trailing separator). A tar entry named ".." would resolve to dest’s parent and bypass the current prefix-only guard, so it should be rejected explicitly.
dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name))
rel, err := filepath.Rel(dest, dstPath)
if err != nil {
archive.go:858
- The breakout/path-traversal check below misses the case where rel == ".." (no trailing separator). A tar entry named ".." would resolve to dest’s parent and bypass the current prefix-only guard, so it should be rejected explicitly.
dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name))
rel, err := filepath.Rel(dest, dstPath)
if err != nil {
thaJeztah
marked this pull request as draft
July 22, 2026 09:30
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.
Unpack: treat tar entry names as POSIX paths
Normalize tar entry names with path.Clean instead of filepath.Clean.
Tar header paths always use POSIX ('/') separators. Preserve their POSIX
form while operating on archive metadata, deferring conversion to native
paths until the filesystem boundary.
This fixes a bug where ExcludePatterns could fail to match valid archive
entries on Windows because filepath.Clean converted tar paths to native
path separators before exclusion matching.
UnpackLayer: treat tar entry names as POSIX paths
Normalize tar entry names with path.Clean instead of filepath.Clean.
Tar header paths always use POSIX ('/') separators. Preserve their POSIX
form while operating on archive metadata, deferring conversion to native
paths until the filesystem boundary.
UnpackLayer: localize AUFS hardlink basenames
Use filepath.Localize when converting AUFS hardlink basenames from tar
header paths to native filesystem paths. This validates that the
basename is a single native path component before it is passed to
filepath.Join(), avoiding reinterpretation of Windows path separators.
UnpackLayer: tighten AUFS whiteout path matching
Only treat hardlinks targeting entries inside /.wh..wh.plnk as AUFS
metadata by requiring a path-component boundary after the directory
name. This avoids matching similarly prefixed paths while preserving
existing handling of AUFS metadata entries and opaque whiteouts.