archive: use os.Root for bounded tar extraction (ART-226) by ctalledo · Pull Request #25 · moby/go-archive

ctalledo · 2026-05-21T21:33:19Z

Depends on #24.

Replaces path-string-based extraction with os.Root (Go 1.24), which
bounds all file operations within the extraction root at the kernel
level using openat(2) semantics, preventing tar path-traversal
attacks.

Note: on Windows, sequential.OpenFile (which passes
FILE_FLAG_SEQUENTIAL_SCAN) is replaced by root.OpenFile. Correctness
is unaffected, but there may be a minor performance regression for large
extractions on Windows.

codecov-commenter · 2026-05-27T23:14:05Z

Codecov Report

❌ Patch coverage is 61.21212% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.85%. Comparing base (2cd730e) to head (f5ce6f5).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
archive.go	51.56%	16 Missing and 15 partials ⚠️
safepath.go	70.96%	11 Missing and 7 partials ⚠️
diff.go	61.76%	8 Missing and 5 partials ⚠️
archive_unix.go	50.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
- Coverage   66.35%   64.85%   -1.50%     
==========================================
  Files          42       43       +1     
  Lines        2027     2117      +90     
==========================================
+ Hits         1345     1373      +28     
- Misses        497      558      +61     
- Partials      185      186       +1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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>

tonistiigi · 2026-06-06T01:16:52Z

+	// (mknod, xattrs, symlinks with absolute targets, lchtimes).
+	// safeResolve walks each path component and bounds any symlinks within
+	// the root to prevent TOCTOU symlink attacks.
+	absPath, err := safeResolve(root.Name(), path)


Even if this function works, absPath is not safe as soon as "safeResolve" returns if the destination directory is mutable. Anything using absPath is essentially a breakout from os.Root.

mknod, xattrs, lctimes

This can be done by using NOFOLLOW from the parent dir fd resolved from os.Root. Eg. https://github.com/tonistiigi/fsutil/blob/tonistiigi/root-writer-receiver/root_parent_unix.go#L29

root.Symlink should work fine.

Thanks @tonistiigi; it's a valid concern, but hinges on a threat model where the destination directory is mutable (e.g., another process with write access to the dest tree racing the extraction). Let me address that in a follow-on PR, because otherwise the fix would make this PR too large.

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>

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>

thaJeztah · 2026-07-15T16:21:58Z

+		// replace it. The only exception is when it is a directory *and* the
+		// entry from the archive is also a directory, in which case we merge
+		// (i.e. just apply the metadata from the archive).
+		if fi, err := root.Lstat(hdr.Name); err == nil {


Wondering if we'd still have the same issue w.r.t. path vs filepath semantics here; it shouldn't escape the root, but may still produce inconsistent results? (at least IIUC, os.Root handles paths using the host's semantics (which could be either Linux or Windows), but the Tar headers are expected to be following POSIX / Unix semantics).

ctalledo · 2026-07-15T18:25:28Z

Superseded by #45, which combines #24, #25 and #26 into a single PR rebased onto latest main (incorporating the review feedback from here). Closing in favor of that.

tar header names always use POSIX (forward-slash) semantics, but parts of the extraction path handled them with host-specific primitives: filepath.Base / filepath.Clean, and os.Root (which resolves paths using the host's rules). On Windows this could interpret a name inconsistently -- a literal backslash in a Linux filename is a path separator there, so os.Root would split "a\b" into two components instead of one name. This never escapes the root, but produces incorrect results. - Use the path (not filepath) package for the remaining tar-name operations in Unpack / UnpackLayer and the pack-side whiteout check. - os.Root cannot be forced to POSIX-parse, so skip entries whose names contain characters Windows cannot represent in a single path component (":" and "\"), consistently in both Unpack and UnpackLayer (previously only UnpackLayer skipped ":"). Raised by thaJeztah on moby#25. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>

Follow-up to the previous POSIX-semantics commit, closing the remaining spots where tar-header paths were not handled consistently: - The Windows skip now also covers hardlink targets. The guard checked only hdr.Name, but a hardlink's hdr.Linkname is resolved by os.Root.Link and would be misparsed on Windows if it contained "\". Factored into unrepresentableOnWindows(hdr), used by both Unpack and UnpackLayer. Symlink targets are stored verbatim, so are unaffected. - UnpackLayer whiteout handling now uses path.Base / path.Dir / path.Join on the tar name (hdr.Name) instead of filepath.*. - safeResolve no longer silently ToSlash-converts its input; it now documents and requires a slash-separated (POSIX) path. All callers already pass tar-header names. Raised by thaJeztah on moby#25. Signed-off-by: Cesar Talledo <cesar.talledo@docker.com>

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>

ctalledo mentioned this pull request May 21, 2026

archive: cache parent directory fd during tar extraction #26

Closed

1 task

ctalledo force-pushed the art-226-osroot branch 4 times, most recently from b9728a4 to 3690fd0 Compare May 27, 2026 21:57

github-advanced-security AI found potential problems May 27, 2026

View reviewed changes

Comment thread archive.go Fixed

ctalledo force-pushed the art-226-osroot branch from 3690fd0 to de28396 Compare May 27, 2026 23:08

ctalledo force-pushed the art-226-osroot branch 7 times, most recently from 8982634 to e5588b6 Compare May 28, 2026 16:13

ctalledo force-pushed the art-226-osroot branch 2 times, most recently from 6175c5c to d29a4ec Compare May 28, 2026 18:42

tonistiigi reviewed Jun 6, 2026

View reviewed changes

ctalledo requested a review from tonistiigi June 8, 2026 18:56

ctalledo mentioned this pull request Jun 10, 2026

[DO NOT MERGE] vendor: go-archive path-traversal stack (CI signal) moby/moby#52811

Draft

ctalledo added 3 commits June 10, 2026 09:40

ctalledo force-pushed the art-226-osroot branch from b79dabf to f5ce6f5 Compare June 10, 2026 17:03

rzlink mentioned this pull request Jul 7, 2026

Confine symlinks to root during Windows unpack #33

Closed

thaJeztah reviewed Jul 15, 2026

View reviewed changes

This was referenced Jul 15, 2026

archive: harden tar extraction against path traversal #45

Merged

archive: replace static path check with symlink-aware safeResolve #24

Closed

ctalledo closed this Jul 15, 2026

Navigation Menu

Uh oh!

archive: use os.Root for bounded tar extraction (ART-226) - #25

archive: use os.Root for bounded tar extraction (ART-226)#25
ctalledo wants to merge 4 commits into
moby:mainfrom
ctalledo:art-226-osroot

ctalledo commented May 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

codecov-commenter commented May 27, 2026 •

edited

Loading

Uh oh!

tonistiigi Jun 6, 2026

Uh oh!

ctalledo Jun 8, 2026

Uh oh!

Uh oh!

thaJeztah Jul 15, 2026

Uh oh!

ctalledo commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

ctalledo commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tonistiigi Jun 6, 2026

Choose a reason for hiding this comment

Uh oh!

ctalledo Jun 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

ctalledo commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ctalledo commented May 21, 2026 •

edited

Loading

codecov-commenter commented May 27, 2026 •

edited

Loading