archive: replace static path check with symlink-aware safeResolve

[ctalledo]

Summary

A malicious tar archive can escape the extraction root by planting a
symlink chain that passes static string-based path checks but resolves
outside the root at runtime. Unpack and UnpackLayer validated entry
paths using filepath.Join + filepath.Rel, which does not follow
symlinks on disk. A two-hop chain (an in-root symlink to .. followed
by a second symlink whose target appears safe statically but escapes
after the first hop is resolved) bypasses both the entry-name check and
the existing symlink-target check in createTarFile.

On Linux this is mitigated by chrootarchive wrapping extraction in
chroot(2). On platforms without chroot support (Windows) and for
callers that bypass chrootarchive (e.g. BuildKit ADD --unpack),
the extraction root was not enforced.

This PR introduces safeResolve (ported from
containerd/continuity/fs.RootPath), which walks each path component
with os.Lstat and resolves symlinks within the extraction root,
bounding any targets that would escape back inside it.

Applied to:

• Unpack: main path computation and deferred directory chtimes loop
• UnpackLayer: same
• createTarFile TypeLink: hardlink target resolution

Test plan

• go test ./... passes
• TestUntarSymlinkBreakout fails on the pre-fix code (confirmed
  locally): a two-hop symlink chain (inner/go_up -> "..",
  inner/go_up/escape -> "../victim") bypasses both the entry-name
  check and the existing symlink-target check in createTarFile, but
  is blocked by safeResolve

[codecov-commenter]

Codecov Report

❌ Patch coverage is 80.68182% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.23%. Comparing base (f12e6e1) to head (c7f83f1).
⚠️ Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
safepath.go	79.03%	8 Missing and 5 partials ⚠️
archive.go	82.35%	1 Missing and 2 partials ⚠️
diff.go	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
+ Coverage   65.09%   65.23%   +0.14%     
==========================================
  Files          42       43       +1     
  Lines        2833     2091     -742     
==========================================
- Hits         1844     1364     -480     
+ Misses        803      545     -258     
+ Partials      186      182       -4     

☔ 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.

[ctalledo]

Good catch — these had real Windows correctness problems, especially the filepath.IsAbs one: a tar-style absolute target like /etc/passwd is not absolute under Windows semantics, so it would have been silently joined relative to newdir. Pushed a refactor that operates on slash-separated paths internally (using the path package) and only converts to OS-native at the boundaries. PTAL.

[vvoland]

What was the reason for changing from filepath.Clean?

[ctalledo]

filepath.Clean doesn't neutralize traversal and is platform-wrong for tar names: it keeps a leading .. (filepath.Clean("../../etc") -> "../../etc"), and it uses the OS separator while tar names are always forward-slash (so it mangles them on Windows). Anchoring at / makes path.Join("/", name) clamp .. at the root (can't climb above it), then TrimLeft makes it root-relative again; using path rather than filepath keeps it slash-based on every platform. The filepath.IsLocal check immediately after is the actual sanitizer.

[vvoland]

My concern is that it would now rewrite ../../etc/passwd into etc/passwd, whereas the previous logic would simply reject it completely as "%q is outside of %q"

In this case, would path.Clean be better?

[ctalledo]

Good catch -- this restores the prior behaviour of rejecting traversal entries; the path.Join("/", name) clamp had started silently rewriting ../../etc/passwd to etc/passwd. Now path.Clean(strings.TrimLeft(name, "/")): still strips a leading / (lenient, as before), but keeps .. so IsLocal rejects escapes. Both Unpack and UnpackLayer (c7f83f1).

[vvoland]

Outside of the difference in full/relative path args, is this implementation better than symlink.FollowSymlinkInScope ?

[ctalledo]

Good point -- FollowSymlinkInScope is equivalent here and would drop the continuity port. But per @tonistiigi's review on #25 (safeResolve/absPath TOCTOU), the plan is to move these onto dir-fd *at ops (started in #26, finished in a follow-up) rather than resolve to a path string -- and FollowSymlinkInScope has the same TOCTOU caveat. I'll revisit the remaining callers then: migrate to fds or switch to FollowSymlinkInScope.

[vvoland]

This should also get the same hardening as hardlink?

[ctalledo]

For symlinks it's the link's location that needs containment, not the target -- the target is stored verbatim and may legitimately point outside root (e.g. /usr/lib), with containment applying when it's followed. #25 reworks this case to root.Symlink(hdr.Linkname, path), which bounds the location via os.Root.

[vvoland]

@tonistiigi PTAL

[thaJeztah]

We should use path, not filepath when processing tar headers, because they're expected to always follow unix-semantics (forward slashes)

Suggested change

	// inode itself, not its target).
	targetDir, err := safeResolve(extractDir, filepath.Dir(hdr.Linkname))
	// inode itself, not its target).
	targetDir, err := safeResolve(extractDir, path.Dir(hdr.Linkname))

[thaJeztah]

This one would be a bit ugly, but here we're converting UNIX to platform-specific 🤔

I guess for Base it doesn't really make a difference, but it we want to be explicit that we're dealing with forward-slashes here;

targetPath := filepath.Join(targetDir, path.Base(hdr.Linkname))

Or more explicit that we're converting (FromSlash will be a no-op though);

base := path.Base(hdr.Linkname)
targetPath := filepath.Join(targetDir, filepath.FromSlash(hdr.Linkname))

[thaJeztah]

This looks incorrect: hdr.Name is a tar header path, so it uses Unix (slash) semantics, so we can't use os.PathSeparator; and should use strings.HasSuffix(hdr.Name, "/").

BUT perhaps better, to use hdr.Typeflag != tar.TypeDir to make sure we're dealing with a file (not a directory) here instead of using the trailing /.

If we do, we should still trim the trailing slash before calling path.Dir, otherwise path.Dir("foo/bar/") returns "foo/bar" instead of "foo".

if hdr.Typeflag != tar.TypeDir {
	parent := path.Dir(strings.TrimSuffix(hdr.Name, "/"))
	// ...
}

[thaJeztah]

Slightly wondering if we should leave this out, and to require callers to always use Unix (tar header format) semantics for p instead of silently correcting if it's not.

Looking at the code (also before this PR) it looks like we have many places where we handle tar-headers using local platform semantics (filepath instead of path). I think that could silently swallow subtle things; e.g. (on Windows);

hdr.Name = `foo\bar`

When using filepath;

filepath.Base(hdr.Name) // "bar"
filepath.Dir(hdr.Name)  // "foo"

But with path;

path.Base(hdr.Name) // "foo\bar"
path.Dir(hdr.Name)  // "."

[thaJeztah]

ugh; missed two lines in my rebase because I dropped an intermediate commit

[ctalledo]

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.