archive: cache parent directory fd during tar extraction - #26
Closed
ctalledo wants to merge 5 commits into
Closed
Conversation
ctalledo
force-pushed
the
art-227-dirfd-cache
branch
4 times, most recently
from
May 27, 2026 21:57
33dca5a to
0cbb0a7
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ctalledo
force-pushed
the
art-227-dirfd-cache
branch
6 times, most recently
from
May 28, 2026 00:36
21b1745 to
1310801
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
- Coverage 65.09% 64.89% -0.20%
==========================================
Files 42 45 +3
Lines 2833 2236 -597
==========================================
- Hits 1844 1451 -393
+ Misses 803 585 -218
- Partials 186 200 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ctalledo
force-pushed
the
art-227-dirfd-cache
branch
2 times, most recently
from
May 28, 2026 16:13
8a8927d to
eb5c2d7
Compare
The Unpack and UnpackLayer functions validated tar entry paths using a pure string check (filepath.Join + filepath.Rel). This does not follow symlinks, so a malicious archive could plant a symlink pointing outside the extraction root and then write files through it, bypassing the check. On Linux this is mitigated by chrootarchive wrapping extraction in a chroot(2) call. On platforms without chroot support (Windows) and for callers that bypass chrootarchive (e.g. BuildKit ADD --unpack), the extraction root is not enforced. Introduce safeResolve (ported from containerd/continuity/fs.RootPath), which walks each path component with os.Lstat and resolves symlinks within the extraction root, bounding absolute targets and relative targets that would escape root back inside it. Apply safeResolve to: - Unpack: main path computation and the deferred directory chtimes loop - UnpackLayer: same - createTarFile TypeLink: hardlink target resolution Add a regression test for the symlink-chain bypass: a within-dest symlink (go_up -> "..") used to redirect a second symlink outside dest (escape -> "../victim"), which the static check missed. Fixes ART-225. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
ctalledo
force-pushed
the
art-227-dirfd-cache
branch
2 times, most recently
from
May 28, 2026 18:43
4aca623 to
0570238
Compare
ctalledo
force-pushed
the
art-227-dirfd-cache
branch
from
June 8, 2026 19:00
0570238 to
9c0f906
Compare
The name normalisation in Unpack and UnpackLayer used
strings.TrimLeft(path.Join("/", hdr.Name), "/"), which silently
rewrote a traversal entry such as "../../etc/passwd" to an in-root
path ("etc/passwd") and accepted it, instead of rejecting it. That
diverged from the prior behaviour (which rejected such entries) and
left the following filepath.IsLocal check as dead code, since the
anchor-at-"/" clamp always produced a local path first.
Use path.Clean(strings.TrimLeft(hdr.Name, "/")) instead: strip a
leading "/" so absolute entries stay root-relative (lenient, as
before), but preserve a leading ".." so filepath.IsLocal rejects
entries that escape the root. This restores reject-on-traversal
semantics while staying forward-slash based for cross-platform tar
names. Raised by Pawel Gronowski on moby#24.
Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
Replace the path-string-based extraction with os.Root, which bounds all file operations within the extraction root at the kernel level using openat(2) semantics. This is the primary defence against tar path-traversal attacks (ART-226). Key changes: - Unpack and UnpackLayer open an os.Root for dest at entry. - createTarFile takes *os.Root and a root-relative name instead of an absolute path and extractDir. - TypeDir, TypeReg, TypeLink, Lchown, Chmod, and Chtimes all go through root.* methods. - For operations os.Root does not yet support (mknod, xattrs, lchtimes on symlinks), absPath is derived via safeResolve so paths remain bounded within the root. - overlayWhiteoutConverter.ConvertRead makes direct unix.Setxattr and unix.Mknod syscalls and requires an absolute path; pass safeResolve'd absPath rather than the root-relative hdr.Name. - safepath.go is retained: safeResolve is still used for absPath derivation. - testBreakout updated to skip symlinks: correct under the os.Root model, where symlink nodes may have out-of-root targets but traversal through them via root.* is rejected by the kernel. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
The TypeSymlink case used os.Symlink(hdr.Linkname, absPath), where absPath was a resolved absolute path, on the assumption that os.Root.Symlink rejects the absolute symlink targets (e.g. /usr/lib) that are common in container images. That assumption is wrong: os.Root.Symlink does not validate oldname (the link target); it only bounds newname (the link location) within root. Absolute targets are stored verbatim. So root.Symlink(hdr.Linkname, path) creates the link node within root via openat(2) semantics while preserving the target exactly, with no resolved absolute path involved. This also resolves the CodeQL "arbitrary file write extracting an archive containing symbolic links" finding, which the os.Symlink-on-a-resolved-path form triggered, rather than dismissing it. Pointed out by Tonis Tiigi on moby#25. absPath is still used for mknod and xattrs. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
Each root.*(path) call re-walks every path component via doInRoot, costing 2D syscalls per call at path depth D. A typical file entry triggers ~5 root.* calls, so at depth 4 that is ~40 syscalls in path traversal alone. Add a dirCache that keeps one parent-directory fd open between entries. Consecutive entries in the same directory reuse the cached fd for all *at(2) operations, reducing path-traversal cost to ~5 syscalls per entry regardless of depth. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
ctalledo
force-pushed
the
art-227-dirfd-cache
branch
from
June 10, 2026 17:03
9c0f906 to
3359566
Compare
This was referenced Jul 15, 2026
ctalledo
added a commit
to ctalledo/go-archive
that referenced
this pull request
Jul 15, 2026
Combines the tar path-traversal hardening (previously split across moby/go-archive moby#24, moby#25 and moby#26) into one change on current main. Addresses ART-224 and the cluster of externally reported tar-extraction breakouts (Windows BuildKit ADD/build, and docker cp on all platforms). - Reject traversal entries instead of clamping them: normalize hdr.Name with path.Clean(strings.TrimLeft(name, "/")) and reject non-local names via filepath.IsLocal, in both Unpack and UnpackLayer. - Bound extraction with os.Root (openat-based); create symlinks with root.Symlink (target stored verbatim, so absolute targets are kept) and hardlinks with root.Link plus a filepath.IsLocal defence-in-depth check. - Cache the most recent parent directory fd (dirCache) so consecutive entries in the same directory use *at(2) syscalls, amortizing os.Root's per-call path re-evaluation. - Resolve symlink components with fsRootPath, a straight fork of containerd/continuity fs.RootPath (path.go + path_test.go), un-exported and trimmed to the functions used, to ease upstream sync. - tar header names are POSIX; convert to native paths with filepath.FromSlash at each os.Root / filesystem boundary, and skip entries whose name or hardlink target Windows cannot represent (":", "\"). Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
thaJeztah
pushed a commit
to ctalledo/go-archive
that referenced
this pull request
Jul 16, 2026
Combines the tar path-traversal hardening (previously split across moby/go-archive moby#24, moby#25 and moby#26) into one change on current main. Addresses ART-224 and the cluster of externally reported tar-extraction breakouts (Windows BuildKit ADD/build, and docker cp on all platforms). - Reject traversal entries instead of clamping them: normalize hdr.Name with path.Clean(strings.TrimLeft(name, "/")) and reject non-local names via filepath.IsLocal, in both Unpack and UnpackLayer. - Bound extraction with os.Root (openat-based); create symlinks with root.Symlink (target stored verbatim, so absolute targets are kept) and hardlinks with root.Link plus a filepath.IsLocal defence-in-depth check. - Cache the most recent parent directory fd (dirCache) so consecutive entries in the same directory use *at(2) syscalls, amortizing os.Root's per-call path re-evaluation. - Resolve symlink components with fsRootPath, a straight fork of containerd/continuity fs.RootPath (path.go + path_test.go), un-exported and trimmed to the functions used, to ease upstream sync. - tar header names are POSIX; convert to native paths with filepath.FromSlash at each os.Root / filesystem boundary, and skip entries whose name or hardlink target Windows cannot represent (":", "\"). Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
thaJeztah
pushed a commit
to ctalledo/go-archive
that referenced
this pull request
Jul 16, 2026
Combines the tar path-traversal hardening (previously split across moby/go-archive moby#24, moby#25 and moby#26) into one change on current main. Addresses ART-224 and the cluster of externally reported tar-extraction breakouts (Windows BuildKit ADD/build, and docker cp on all platforms). - Reject traversal entries instead of clamping them: normalize hdr.Name with path.Clean(strings.TrimLeft(name, "/")) and reject non-local names via filepath.IsLocal, in both Unpack and UnpackLayer. - Bound extraction with os.Root (openat-based); create symlinks with root.Symlink (target stored verbatim, so absolute targets are kept) and hardlinks with root.Link plus a filepath.IsLocal defence-in-depth check. - Cache the most recent parent directory fd (dirCache) so consecutive entries in the same directory use *at(2) syscalls, amortizing os.Root's per-call path re-evaluation. - Resolve symlink components with fsRootPath, a straight fork of containerd/continuity fs.RootPath (path.go + path_test.go), un-exported and trimmed to the functions used, to ease upstream sync. - tar header names are POSIX; convert to native paths with filepath.FromSlash at each os.Root / filesystem boundary, and skip entries whose name or hardlink target Windows cannot represent (":", "\"). Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>
thaJeztah
pushed a commit
to ctalledo/go-archive
that referenced
this pull request
Jul 16, 2026
Combines the tar path-traversal hardening (previously split across moby/go-archive moby#24, moby#25 and moby#26) into one change on current main. Addresses ART-224 and the cluster of externally reported tar-extraction breakouts (Windows BuildKit ADD/build, and docker cp on all platforms). - Reject traversal entries instead of clamping them: normalize hdr.Name with path.Clean(strings.TrimLeft(name, "/")) and reject non-local names via filepath.IsLocal, in both Unpack and UnpackLayer. - Bound extraction with os.Root (openat-based); create symlinks with root.Symlink (target stored verbatim, so absolute targets are kept) and hardlinks with root.Link plus a filepath.IsLocal defence-in-depth check. - Cache the most recent parent directory fd (dirCache) so consecutive entries in the same directory use *at(2) syscalls, amortizing os.Root's per-call path re-evaluation. - Resolve symlink components with fsRootPath, a straight fork of containerd/continuity fs.RootPath (path.go + path_test.go), un-exported and trimmed to the functions used, to ease upstream sync. - tar header names are POSIX; convert to native paths with filepath.FromSlash at each os.Root / filesystem boundary, and skip entries whose name or hardlink target Windows cannot represent (":", "\"). Signed-off-by: Cesar Talledo <cesar.talledo@docker.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
3 participants
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.
Summary
Depends on #25.
Each
root.*(path)call re-walks every path component viadoInRoot, costing 2D syscalls per call at path depth D. A typical file entry triggers ~5root.*calls, so at depth 4 that is ~40 syscalls in path traversal alone.This PR adds a
dirCachethat keeps one parent-directory fd open between entries. Consecutive entries in the same directory reuse the cached fd for all*at(2)operations, reducing path-traversal cost to ~5 syscalls per entry regardless of depth.Test plan
go test ./...passes