From 5c7e55be051576a7bfc42299cec3e7d55fc6341f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 21 Jul 2026 19:31:02 +0200 Subject: [PATCH 1/8] archive: skip files that cannot be represented on Windows Co-authored-by: Cesar Talledo Signed-off-by: Sebastiaan van Stijn --- archive.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/archive.go b/archive.go index e59cfbb..bc396be 100644 --- a/archive.go +++ b/archive.go @@ -862,6 +862,12 @@ loop: } } + // Skip entries whose name (or hardlink target) Windows cannot represent. + if err := unrepresentableOnWindows(hdr); err != nil { + log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err) + continue loop + } + // Ensure that the parent directory exists. err = createImpliedDirectories(dest, hdr, options) if err != nil { @@ -941,6 +947,27 @@ loop: return nil } +// unrepresentableOnWindows returns an error describing why a tar entry cannot +// be faithfully created on Windows, or nil if it can (always on non-Windows). +// On Windows ":" is illegal in a filename and "\" is a path separator, so a tar +// name or hardlink target containing them (they use POSIX semantics) would be +// misinterpreted by os.Root (e.g. "a\b" resolved as two components). Symlink +// targets are stored verbatim (not resolved at creation), so they are exempt. +func unrepresentableOnWindows(hdr *tar.Header) error { + if runtime.GOOS != "windows" { + return nil + } + if strings.ContainsAny(hdr.Name, `:\`) { + return fmt.Errorf("entry name %q contains a character Windows cannot represent in a path", hdr.Name) + } + // A hardlink target is resolved within the root by os.Root.Link; a symlink + // target is stored verbatim, so only hardlinks need the target checked. + if hdr.Typeflag == tar.TypeLink && strings.ContainsAny(hdr.Linkname, `:\`) { + return fmt.Errorf("hardlink target %q contains a character Windows cannot represent in a path", hdr.Linkname) + } + return nil +} + // createImpliedDirectories will create all parent directories of the current path with default permissions, if they do // not already exist. This is possible as the tar format supports 'implicit' directories, where their existence is // defined by the paths of files in the tar, but there are no header entries for the directories themselves, and thus From df55fdf3cd62b85c6f22c7b1002c3acb3481f78c Mon Sep 17 00:00:00 2001 From: Cesar Talledo Date: Wed, 15 Jul 2026 16:15:36 -0700 Subject: [PATCH 2/8] archive: harden tar extraction against path traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (":", "\"). archive: make lchtimes use os.Root for path resolution Resolve the parent directory through os.Root and perform utimensat(2) relative to the opened directory instead of using an absolute host path. This preserves os.Root's path containment guarantees while still updating the symlink itself using AT_SYMLINK_NOFOLLOW. createImpliedDirectories: Keep implied dirs at ImpliedDirectoryMode under umask createImpliedDirectories previously used user.MkdirAllAndChown, whose setPermissions runs os.Chmod after creation, so implied parent directories always ended up with ImpliedDirectoryMode regardless of the process umask. The os.Root rewrite creates them with root.Mkdir only, which applies the mode subject to umask: under umask 0o027 an implied directory became 0o750 instead of 0o755. Re-apply the mode with root.Chmod after each successful Mkdir so implied directories keep ImpliedDirectoryMode independent of umask, matching the prior behavior and the function's documented contract. Co-authored-by: Cesar Talledo Co-authored-by: Paweł Gronowski Co-authored-by: Sebastiaan van Stijn Signed-off-by: Cesar Talledo Signed-off-by: Paweł Gronowski Signed-off-by: Sebastiaan van Stijn --- archive.go | 271 +++++++++++++++++++++-------- archive_test.go | 65 ++++++- archive_unix.go | 11 +- archive_windows.go | 3 +- chrootarchive/archive_unix_test.go | 7 +- diff.go | 130 ++++++++------ rootpath.go | 112 ++++++++++++ sequential_other.go | 6 + sequential_windows_go126.go | 9 + sequential_windows_pre126.go | 6 + time_nonwindows.go | 34 +++- time_windows.go | 2 +- utils_test.go | 13 +- 13 files changed, 517 insertions(+), 152 deletions(-) create mode 100644 rootpath.go create mode 100644 sequential_other.go create mode 100644 sequential_windows_go126.go create mode 100644 sequential_windows_pre126.go diff --git a/archive.go b/archive.go index bc396be..a45fb79 100644 --- a/archive.go +++ b/archive.go @@ -8,9 +8,11 @@ import ( "fmt" "io" "os" + "path" "path/filepath" "runtime" "strings" + "sync" "syscall" "time" @@ -95,6 +97,23 @@ func NewDefaultArchiver() *Archiver { return &Archiver{Untar: Untar} } +// isPathEscapes reports whether err is os.Root's path-containment error. +// +// os.Root currently returns an unexported errPathEscapes sentinel, so callers +// cannot detect it with errors.Is. Keep the string comparison isolated here +// until Go exports the error; see https://go.dev/issue/74640. +func isPathEscapes(err error) bool { + // https://github.com/golang/go/blob/go1.26.5/src/os/file.go#L421 + const errPathEscapes = "path escapes from parent" + for err != nil { + if errors.Unwrap(err) == nil { + return err.Error() == errPathEscapes + } + err = errors.Unwrap(err) + } + return false +} + // breakoutErr marks errors caused by archive breakout attempts. // Unit tests use it to distinguish expected breakout failures from other // errors. @@ -357,7 +376,7 @@ func (ta *tarAppender) addTarFile(srcPath, archivePath string) error { // handle re-mapping container ID mappings back to host ID mappings before // writing tar headers/files. We skip whiteout files because they were written // by the kernel and already have proper ownership relative to the host - if !isOverlayWhiteout && !strings.HasPrefix(filepath.Base(hdr.Name), WhiteoutPrefix) && !ta.IdentityMapping.Empty() { + if !isOverlayWhiteout && !strings.HasPrefix(path.Base(hdr.Name), WhiteoutPrefix) && !ta.IdentityMapping.Empty() { uid, gid, err := getFileUIDGID(fi.Sys()) if err != nil { return err @@ -418,7 +437,10 @@ func (ta *tarAppender) addTarFile(srcPath, archivePath string) error { return nil } -func createTarFile(dstPath, extractDir string, hdr *tar.Header, reader io.Reader, opts *TarOptions) error { +// createTarFile extracts a single tar entry into the given root. dstPath is the +// root-relative path of the entry being extracted, in native (host-separator) +// form so it can be passed directly to os.Root methods and fsRootPath. +func createTarFile(root *os.Root, dstPath string, hdr *tar.Header, reader io.Reader, opts *TarOptions) error { var ( Lchown = true inUserns, bestEffortXattrs bool @@ -438,20 +460,33 @@ func createTarFile(dstPath, extractDir string, hdr *tar.Header, reader io.Reader // so use hdrInfo.Mode() (they differ for e.g. setuid bits) hdrInfo := hdr.FileInfo() + // absPath is computed lazily. It is only required for mknod and xattrs. + // Symlinks intentionally use root.Symlink directly to preserve absolute + // targets; os.Root.Symlink rejects absolute targets like /usr/lib. + absPath := sync.OnceValues(func() (string, error) { + return fsRootPath(root.Name(), dstPath) + }) + switch hdr.Typeflag { case tar.TypeDir: - // Create directory unless it exists as a directory already. - // In that case we just want to merge the two - if fi, err := os.Lstat(dstPath); err != nil || !fi.IsDir() { - if err := os.Mkdir(dstPath, hdrInfo.Mode()); err != nil { + // Create directory unless it already exists as one; merge in that case. + // os.Root.Mkdir only accepts the nine least-significant permission + // bits; special bits (setuid, setgid, sticky) are applied afterward + // by handleLChmod via root.Chmod. + if fi, err := root.Lstat(dstPath); err != nil || !fi.IsDir() { + if err := root.Mkdir(dstPath, hdrInfo.Mode()&0o777); err != nil { return err } } case tar.TypeReg: - // Source is regular file. We use sequential file access to avoid depleting - // the standby list on Windows. On Linux, this equates to a regular os.OpenFile. - file, err := sequential.OpenFile(dstPath, os.O_CREATE|os.O_WRONLY, hdrInfo.Mode()) + // Source is a regular file. Use os.Root.OpenFile so that all + // path resolution is bounded within root using openat(2) semantics. + // os.Root.OpenFile only accepts the nine least-significant permission + // bits; special bits are applied afterward by handleLChmod. + // We use sequential file access to avoid depleting the standby list + // on Windows (go1.26). On Linux, this equates to a regular os.OpenFile. + file, err := root.OpenFile(dstPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC|windows_O_FILE_FLAG_SEQUENTIAL_SCAN, hdrInfo.Mode()&0o777) if err != nil { return err } @@ -466,14 +501,22 @@ func createTarFile(dstPath, extractDir string, hdr *tar.Header, reader io.Reader log.G(context.TODO()).WithFields(log.Fields{"path": dstPath, "type": hdr.Typeflag}).Debug("skipping device nodes in a userns") return nil } - // Handle this is an OS-specific way - if err := handleTarTypeBlockCharFifo(hdr, dstPath); err != nil { + // os.Root has no mknod support; use absPath so the path stays bounded. + ap, err := absPath() + if err != nil { + return err + } + if err := handleTarTypeBlockCharFifo(hdr, ap); err != nil { return err } case tar.TypeFifo: - // Handle this is an OS-specific way - if err := handleTarTypeBlockCharFifo(hdr, dstPath); err != nil { + // os.Root has no mknod support; use absPath so the path stays bounded. + ap, err := absPath() + if err != nil { + return err + } + if err := handleTarTypeBlockCharFifo(hdr, ap); err != nil { if inUserns && errors.Is(err, syscall.EPERM) { // In most cases, cannot create a fifo if running in user namespace log.G(context.TODO()).WithFields(log.Fields{"error": err, "path": dstPath, "type": hdr.Typeflag}).Debug("creating fifo node in a userns") @@ -483,27 +526,29 @@ func createTarFile(dstPath, extractDir string, hdr *tar.Header, reader io.Reader } case tar.TypeLink: - // #nosec G305 -- The target path is checked for path traversal. - linkTarget := filepath.Join(extractDir, hdr.Linkname) - // check for hardlink breakout - if !strings.HasPrefix(linkTarget, extractDir) { - return breakoutError(fmt.Errorf("invalid hardlink %q -> %q", linkTarget, hdr.Linkname)) + // Defence in depth: root.Link's containment is limited when + // dest is a volume root. + linkname := path.Clean(hdr.Linkname) + if linkname == "." || !filepath.IsLocal(linkname) { + return breakoutError(fmt.Errorf("invalid hardlink target %q", hdr.Linkname)) } - if err := os.Link(linkTarget, dstPath); err != nil { + if err := root.Link(filepath.FromSlash(linkname), dstPath); err != nil { return err } case tar.TypeSymlink: - // path -> hdr.Linkname = targetPath - // e.g. /extractDir/path/to/symlink -> ../2/file = /extractDir/path/2/file - targetPath := filepath.Join(filepath.Dir(dstPath), hdr.Linkname) // #nosec G305 -- The target path is checked for path traversal. - - // the reason we don't need to check symlinks in the path (with FollowSymlinkInScope) is because - // that symlink would first have to be created, which would be caught earlier, at this very check: - if !strings.HasPrefix(targetPath, extractDir) { - return breakoutError(fmt.Errorf("invalid symlink %q -> %q", dstPath, hdr.Linkname)) - } - if err := os.Symlink(hdr.Linkname, dstPath); err != nil { + // Symlink targets are archive data, not filesystem paths. Preserve the + // target verbatim rather than cleaning or converting it (filepath.FromSlash). + linkTarget := hdr.Linkname + + // os.Root.Symlink contains the symlink's location (newname) within + // root but stores the target (oldname) verbatim, so absolute targets + // such as /usr/lib -- common and legitimate in container images -- are + // preserved rather than rejected. The symlink node is therefore always + // created within root via openat(2) semantics, without resolving to an + // absolute path; containment applies when the symlink is followed, not + // at creation. + if err := root.Symlink(linkTarget, dstPath); err != nil { return err } @@ -520,7 +565,7 @@ func createTarFile(dstPath, extractDir string, hdr *tar.Header, reader io.Reader if chownOpts == nil { chownOpts = &ChownOpts{UID: hdr.Uid, GID: hdr.Gid} } - if err := os.Lchown(dstPath, chownOpts.UID, chownOpts.GID); err != nil { + if err := root.Lchown(dstPath, chownOpts.UID, chownOpts.GID); err != nil { var msg string if inUserns && errors.Is(err, syscall.EINVAL) { msg = " (try increasing the number of subordinate IDs in /etc/subuid and /etc/subgid)" @@ -535,7 +580,13 @@ func createTarFile(dstPath, extractDir string, hdr *tar.Header, reader io.Reader if !ok { continue } - if err := lsetxattr(dstPath, xattr, []byte(value), 0); err != nil { + // os.Root has no xattr support; use the absolute path derived from + // the root so the path remains bounded. + ap, err := absPath() + if err != nil { + return err + } + if err := lsetxattr(ap, xattr, []byte(value), 0); err != nil { if bestEffortXattrs && errors.Is(err, syscall.ENOTSUP) || errors.Is(err, syscall.EPERM) { // EPERM occurs if modifying xattrs is not allowed. This can // happen when running in userns with restrictions (ChromeOS). @@ -554,7 +605,7 @@ func createTarFile(dstPath, extractDir string, hdr *tar.Header, reader io.Reader // There is no LChmod, so ignore mode for symlink. Also, this // must happen after chown, as that can modify the file mode - if err := handleLChmod(hdr, dstPath, hdrInfo); err != nil { + if err := handleLChmod(root, dstPath, hdr, hdrInfo); err != nil { return err } @@ -564,20 +615,20 @@ func createTarFile(dstPath, extractDir string, hdr *tar.Header, reader io.Reader switch hdr.Typeflag { case tar.TypeSymlink: // Apply timestamps to the symlink itself (AT_SYMLINK_NOFOLLOW). - if err := lchtimes(dstPath, aTime, mTime); err != nil { + if err := lchtimes(root, dstPath, aTime, mTime); err != nil { return err } case tar.TypeLink: // Follow the hardlink only when its target is not itself a symlink. - fi, err := os.Lstat(hdr.Linkname) + fi, err := root.Lstat(filepath.FromSlash(path.Clean(hdr.Linkname))) if err == nil && fi.Mode()&os.ModeSymlink == 0 { - if err := chtimes(dstPath, aTime, mTime); err != nil { + if err := root.Chtimes(dstPath, aTime, mTime); err != nil { return err } } default: // All other file types follow symlinks. - if err := chtimes(dstPath, aTime, mTime); err != nil { + if err := root.Chtimes(dstPath, aTime, mTime); err != nil { return err } } @@ -823,14 +874,28 @@ func (t *Tarballer) Do() { } } +// unpackedDir records a directory whose mtime must be restored after all +// entries are extracted, along with the root-relative entry name used during +// extraction. +type unpackedDir struct { + hdr *tar.Header + name string // root-relative entry name +} + // Unpack unpacks the decompressedArchive to dest with options. func Unpack(decompressedArchive io.Reader, dest string, options *TarOptions) error { if options == nil { options = &TarOptions{} } + root, err := os.OpenRoot(dest) + if err != nil { + return err + } + defer func() { _ = root.Close() }() + tr := tar.NewReader(decompressedArchive) - var dirs []*tar.Header + var dirs []unpackedDir whiteoutConverter := getWhiteoutConverter(options.WhiteoutFormat) // Iterate through the files in the archive. @@ -851,54 +916,56 @@ loop: continue } - // Normalize name, for safety and for a simple is-root check - // This keeps "../" as-is, but normalizes "/../" to "/". Or Windows: - // This keeps "..\" as-is, but normalizes "\..\" to "\". - hdr.Name = filepath.Clean(hdr.Name) - + // 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 + } + if !filepath.IsLocal(name) { + return breakoutError(fmt.Errorf("invalid entry name %q", hdr.Name)) + } for _, exclude := range options.ExcludePatterns { - if strings.HasPrefix(hdr.Name, exclude) { + if strings.HasPrefix(name, exclude) { continue loop } } + hdr.Name = name + // Skip entries whose name (or hardlink target) Windows cannot represent. if err := unrepresentableOnWindows(hdr); err != nil { log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err) continue loop } - // Ensure that the parent directory exists. - err = createImpliedDirectories(dest, hdr, options) - if err != nil { - return err - } + // dstPath is the native (host-separator) form of the entry name, + // used at all filesystem boundaries (os.Root methods, fsRootPath). + // hdr.Name stays POSIX (forward-slash) for logical string checks. + dstPath := filepath.FromSlash(hdr.Name) - // #nosec G305 -- The joined path is checked for path traversal. - dstPath := filepath.Join(dest, hdr.Name) - rel, err := filepath.Rel(dest, dstPath) + // Ensure that the parent directory exists. + err = createImpliedDirectories(root, hdr, options) if err != nil { return err } - if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { - return breakoutError(fmt.Errorf("%q is outside of %q", hdr.Name, dest)) - } // If dstPath exists we almost always just want to remove and replace it. // The only exception is when it is a directory *and* the file from // the layer is also a directory. Then we want to merge them (i.e. // just apply the metadata from the layer). - if fi, err := os.Lstat(dstPath); err == nil { + if fi, err := root.Lstat(dstPath); err == nil { if options.NoOverwriteDirNonDir && fi.IsDir() && hdr.Typeflag != tar.TypeDir { // If NoOverwriteDirNonDir is true then we cannot replace // an existing directory with a non-directory from the archive. - return fmt.Errorf("cannot overwrite directory %q with non-directory %q", dstPath, dest) + return fmt.Errorf("cannot overwrite directory %q with non-directory %q", hdr.Name, dest) } if options.NoOverwriteDirNonDir && !fi.IsDir() && hdr.Typeflag == tar.TypeDir { // If NoOverwriteDirNonDir is true then we cannot replace // an existing non-directory with a directory from the archive. - return fmt.Errorf("cannot overwrite non-directory %q with directory %q", dstPath, dest) + return fmt.Errorf("cannot overwrite non-directory %q with directory %q", hdr.Name, dest) } if fi.IsDir() && hdr.Name == "." { @@ -906,7 +973,7 @@ loop: } if !fi.IsDir() || hdr.Typeflag != tar.TypeDir { - if err := os.RemoveAll(dstPath); err != nil { + if err := root.RemoveAll(dstPath); err != nil { return err } } @@ -917,7 +984,14 @@ loop: } if whiteoutConverter != nil { - writeFile, err := whiteoutConverter.ConvertRead(hdr, dstPath) + // ConvertRead implementations (e.g. overlayWhiteoutConverter) + // make direct syscalls with the path, so they need the absolute + // path bounded within dest rather than the root-relative name. + absPath, err := fsRootPath(root.Name(), dstPath) + if err != nil { + return err + } + writeFile, err := whiteoutConverter.ConvertRead(hdr, absPath) if err != nil { return err } @@ -926,21 +1000,20 @@ loop: } } - if err := createTarFile(dstPath, dest, hdr, tr, options); err != nil { + if err := createTarFile(root, dstPath, hdr, tr, options); err != nil { return err } // Directory mtimes must be handled at the end to avoid further // file creation in them to modify the directory mtime if hdr.Typeflag == tar.TypeDir { - dirs = append(dirs, hdr) + dirs = append(dirs, unpackedDir{hdr: hdr, name: dstPath}) } } - for _, hdr := range dirs { - // #nosec G305 -- The header was checked for path traversal before it was appended to the dirs slice. - dstPath := filepath.Join(dest, hdr.Name) - if err := chtimes(dstPath, boundTime(latestTime(hdr.AccessTime, hdr.ModTime)), boundTime(hdr.ModTime)); err != nil { + for _, d := range dirs { + aTime := boundTime(latestTime(d.hdr.AccessTime, d.hdr.ModTime)) + if err := root.Chtimes(d.name, aTime, boundTime(d.hdr.ModTime)); err != nil { return err } } @@ -972,22 +1045,66 @@ func unrepresentableOnWindows(hdr *tar.Header) error { // not already exist. This is possible as the tar format supports 'implicit' directories, where their existence is // defined by the paths of files in the tar, but there are no header entries for the directories themselves, and thus // we most both create them and choose metadata like permissions. -func createImpliedDirectories(dest string, hdr *tar.Header, options *TarOptions) error { +// +// The caller must have normalized hdr.Name (no leading ".." components). +// All directory creation is performed via root so it is bounded within the +// destination at the OS level (openat(2) semantics), preventing escape via +// symlinks in the destination tree. +func createImpliedDirectories(root *os.Root, hdr *tar.Header, options *TarOptions) error { // For non-directory entries, ensure that the parent directory exists. if hdr.Typeflag != tar.TypeDir { - parent := filepath.Dir(hdr.Name) - parentPath := filepath.Join(dest, parent) - if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) { - if options.NoLchown { - return os.MkdirAll(parentPath, ImpliedDirectoryMode) + parent := filepath.FromSlash(path.Dir(strings.TrimSuffix(hdr.Name, "/"))) + // Skip when the parent is the root itself; nothing to create. + if parent == "." || parent == "" { + return nil + } + if _, err := root.Lstat(parent); err == nil { + return nil + } else if !os.IsNotExist(err) { + return err + } + // RootPair() is confined inside this loop as most cases will not require a call, so we can spend some + // unneeded function calls in the uncommon case to encapsulate logic -- implied directories are a niche + // usage that reduces the portability of an image. + uid, gid := options.IDMap.RootPair() + + // Similar to [user.MkdirAllAndChown] + // + // [user.MkdirAllAndChown]: https://pkg.go.dev/github.com/moby/sys/user#MkdirAllAndChown + var cur string + for c := range strings.SplitSeq(parent, string(os.PathSeparator)) { + if c == "" { + continue } - // RootPair() is confined inside this loop as most cases will not require a call, so we can spend some - // unneeded function calls in the uncommon case to encapsulate logic -- implied directories are a niche - // usage that reduces the portability of an image. - uid, gid := options.IDMap.RootPair() + cur = filepath.Join(cur, c) + if err := root.Mkdir(cur, ImpliedDirectoryMode); err != nil { + if !errors.Is(err, os.ErrExist) { + return err + } - err = user.MkdirAllAndChown(parentPath, ImpliedDirectoryMode, uid, gid, user.WithOnlyNew) - if err != nil { + fi, err := root.Stat(cur) + if err != nil { + return err + } + if fi.IsDir() { + continue + } + return &os.PathError{Op: "mkdir", Path: cur, Err: syscall.ENOTDIR} + } + if options.NoLchown { + continue + } + // Only the successful Mkdir case is newly-created. + if uid != 0 || gid != 0 { + if err := root.Lchown(cur, uid, gid); err != nil { + return err + } + } + // root.Mkdir applies the mode subject to the process umask, so + // re-apply it with Chmod to guarantee ImpliedDirectoryMode + // independent of umask, matching the previous MkdirAllAndChown + // behavior. + if err := root.Chmod(cur, ImpliedDirectoryMode); err != nil { return err } } diff --git a/archive_test.go b/archive_test.go index c290b88..b4d62dd 100644 --- a/archive_test.go +++ b/archive_test.go @@ -574,7 +574,12 @@ func TestTarWithOptions(t *testing.T) { func TestTypeXGlobalHeaderDoesNotFail(t *testing.T) { hdr := tar.Header{Typeflag: tar.TypeXGlobalHeader} tmpDir := t.TempDir() - err := createTarFile(filepath.Join(tmpDir, "pax_global_header"), tmpDir, &hdr, nil, nil) + root, err := os.OpenRoot(tmpDir) + if err != nil { + t.Fatal(err) + } + defer root.Close() + err = createTarFile(root, "pax_global_header", &hdr, nil, nil) if err != nil { t.Fatal(err) } @@ -584,7 +589,6 @@ func TestTypeXGlobalHeaderDoesNotFail(t *testing.T) { // treated as opaque values and are preserved verbatim rather than converted to // platform-native path syntax during extraction. func TestCreateTarFileSymlinkPreservesLinkname(t *testing.T) { - t.Skip("FIXME: currently failing: enable once https://github.com/moby/go-archive/pull/45 is merged") tests := []struct { name string linkname string @@ -603,7 +607,13 @@ func TestCreateTarFileSymlinkPreservesLinkname(t *testing.T) { t.Run(tc.name, func(t *testing.T) { tmpDir := t.TempDir() - if err := os.Mkdir(filepath.Join(tmpDir, "bin"), 0o755); err != nil { + root, err := os.OpenRoot(tmpDir) + if err != nil { + t.Fatal(err) + } + defer root.Close() + + if err := root.Mkdir("bin", 0o755); err != nil { t.Fatal(err) } @@ -613,7 +623,7 @@ func TestCreateTarFileSymlinkPreservesLinkname(t *testing.T) { Linkname: tc.linkname, } - err := createTarFile(filepath.Join(tmpDir, hdr.Name), tmpDir, &hdr, nil, &TarOptions{ + err = createTarFile(root, hdr.Name, &hdr, nil, &TarOptions{ NoLchown: true, }) if err != nil { @@ -764,7 +774,6 @@ func TestUntarInvalidFilenames(t *testing.T) { // into the destination's parent. Regression test for the "write to the parent // of the extraction root" breakout. func TestUntarParentTraversalContained(t *testing.T) { - t.Skip("FIXME: currently failing: enable once https://github.com/moby/go-archive/pull/45 is merged") for _, tc := range []struct { name string entry string @@ -810,7 +819,6 @@ func TestUntarParentTraversalContained(t *testing.T) { // old string-prefix (HasPrefix) containment check, which treated such a sibling // as inside the destination. func TestUntarSiblingPrefixContained(t *testing.T) { - t.Skip("FIXME: currently failing: enable once https://github.com/moby/go-archive/pull/45 is merged") base := t.TempDir() dest := filepath.Join(base, "dest") assert.NilError(t, os.Mkdir(dest, 0o755)) @@ -1043,6 +1051,51 @@ func TestUntarInvalidSymlink(t *testing.T) { } } +// TestUntarSymlinkBreakout is a regression test for a tar path-traversal +// vulnerability: a two-hop symlink chain in a malicious archive can escape +// the extraction root at runtime while passing the static path checks that +// guard each entry name and symlink target. Two hops are needed because a +// direct out-of-root symlink target is already rejected by a static check in +// createTarFile; the first hop (go_up -> "..") fools that check for the +// second hop (escape -> "../victim") by appearing to stay within the root +// when paths are joined as strings, while the OS resolves go_up at runtime +// and places escape one level higher than the check assumed. +func TestUntarSymlinkBreakout(t *testing.T) { + tmpdir := t.TempDir() + dest := filepath.Join(tmpdir, "dest") + victim := filepath.Join(tmpdir, "victim") + if err := os.Mkdir(dest, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(victim, 0o755); err != nil { + t.Fatal(err) + } + + buf := &bytes.Buffer{} + tw := tar.NewWriter(buf) + for _, hdr := range []*tar.Header{ + {Name: "inner", Typeflag: tar.TypeDir, Mode: 0o755}, + {Name: "inner/go_up", Typeflag: tar.TypeSymlink, Linkname: ".."}, + {Name: "inner/go_up/escape", Typeflag: tar.TypeSymlink, Linkname: "../victim"}, + {Name: "inner/go_up/escape/newfile", Typeflag: tar.TypeReg, Mode: 0o644}, + } { + if err := tw.WriteHeader(hdr); err != nil { + t.Fatal(err) + } + } + _ = tw.Close() + + // Ignore any extraction error: a breakoutError means the escape was + // caught; no error means the write was safely redirected within dest. + // NoLchown suppresses the ownership call so the test runs without root. + _ = Untar(buf, dest, &TarOptions{NoLchown: true}) + + // victim/newfile must not exist; its presence proves a breakout. + if _, err := os.Lstat(filepath.Join(victim, "newfile")); err == nil { + t.Fatal("archive breakout: newfile was written outside extraction root via symlink chain") + } +} + func TestTempArchiveCloseMultipleTimes(t *testing.T) { reader := io.NopCloser(strings.NewReader("hello")) tmpArchive, err := newTempArchive(reader, "") diff --git a/archive_unix.go b/archive_unix.go index b8b4cad..fef0b31 100644 --- a/archive_unix.go +++ b/archive_unix.go @@ -83,15 +83,18 @@ func handleTarTypeBlockCharFifo(hdr *tar.Header, dstPath string) error { return mknod(dstPath, mode, unix.Mkdev(uint32(hdr.Devmajor), uint32(hdr.Devminor))) } -func handleLChmod(hdr *tar.Header, dstPath string, hdrInfo os.FileInfo) error { +// handleLChmod applies the mode from hdrInfo to dstPath within root, skipping +// symlinks (there is no lchmod). For hardlinks, the mode is applied only when +// the link target is itself not a symlink. +func handleLChmod(root *os.Root, dstPath string, hdr *tar.Header, hdrInfo os.FileInfo) error { if hdr.Typeflag == tar.TypeLink { - if fi, err := os.Lstat(hdr.Linkname); err == nil && (fi.Mode()&os.ModeSymlink == 0) { - if err := os.Chmod(dstPath, hdrInfo.Mode()); err != nil { + if fi, err := root.Lstat(filepath.FromSlash(hdr.Linkname)); err == nil && (fi.Mode()&os.ModeSymlink == 0) { + if err := root.Chmod(dstPath, hdrInfo.Mode()); err != nil { return err } } } else if hdr.Typeflag != tar.TypeSymlink { - if err := os.Chmod(dstPath, hdrInfo.Mode()); err != nil { + if err := root.Chmod(dstPath, hdrInfo.Mode()); err != nil { return err } } diff --git a/archive_windows.go b/archive_windows.go index ee8dbd6..70efa4a 100644 --- a/archive_windows.go +++ b/archive_windows.go @@ -52,7 +52,8 @@ func handleTarTypeBlockCharFifo(hdr *tar.Header, path string) error { return nil } -func handleLChmod(hdr *tar.Header, path string, hdrInfo os.FileInfo) error { +// handleLChmod is a no-op on Windows because chmod is not supported. +func handleLChmod(root *os.Root, path string, hdr *tar.Header, hdrInfo os.FileInfo) error { return nil } diff --git a/chrootarchive/archive_unix_test.go b/chrootarchive/archive_unix_test.go index 377a428..0a4532b 100644 --- a/chrootarchive/archive_unix_test.go +++ b/chrootarchive/archive_unix_test.go @@ -61,7 +61,12 @@ func TestUntarWithMaliciousSymlinks(t *testing.T) { err = UntarWithRoot(tee, safe, nil, root) assert.Assert(t, err != nil) - assert.ErrorContains(t, err, "open /safe/host-file: no such file or directory") + // Bounded extraction via os.Root may fail when opening the destination + // itself (the symlink target lies outside the chroot root) rather than + // when opening the file inside it. Accept either failure point; the + // security property — that the host file is not overwritten — is + // verified separately below. + assert.ErrorContains(t, err, "no such file or directory") // Make sure the "host" file is still in tact // Before the fix the host file would be overwritten diff --git a/diff.go b/diff.go index b2fdf77..75b84f9 100644 --- a/diff.go +++ b/diff.go @@ -7,8 +7,8 @@ import ( "fmt" "io" "os" + "path" "path/filepath" - "runtime" "strings" "github.com/containerd/log" @@ -20,9 +20,17 @@ import ( // compressed or uncompressed. // Returns the size in bytes of the contents of the layer. func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, err error) { + root, err := os.OpenRoot(dest) + if err != nil { + return 0, err + } + defer root.Close() + tr := tar.NewReader(layer) - var dirs []*tar.Header + var dirs []unpackedDir + // unpackedPaths tracks root-relative paths already written in this layer + // so that the AUFS opaque-whiteout walk knows which paths to preserve. unpackedPaths := make(map[string]struct{}) if options == nil { @@ -45,32 +53,26 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, size += hdr.Size - // Normalize name, for safety and for a simple is-root check - hdr.Name = filepath.Clean(hdr.Name) - - // Windows does not support filenames with colons in them. Ignore - // these files. This is not a problem though (although it might - // appear that it is). Let's suppose a client is running docker pull. - // The daemon it points to is Windows. Would it make sense for the - // client to be doing a docker pull Ubuntu for example (which has files - // with colons in the name under /usr/share/man/man3)? No, absolutely - // not as it would really only make sense that they were pulling a - // Windows image. However, for development, it is necessary to be able - // to pull Linux images which are in the repository. - // - // TODO Windows. Once the registry is aware of what images are Windows- - // specific or Linux-specific, this warning should be changed to an error - // to cater for the situation where someone does manage to upload a Linux - // image but have it tagged as Windows inadvertently. - if runtime.GOOS == "windows" { - if strings.Contains(hdr.Name, ":") { - log.G(context.TODO()).Warnf("Windows: Ignoring %s (is this a Linux image?)", hdr.Name) - continue - } + // 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 + } + if !filepath.IsLocal(name) { + return 0, breakoutError(fmt.Errorf("invalid entry name %q", hdr.Name)) + } + hdr.Name = name + + // Skip entries whose name (or hardlink target) Windows cannot represent. + if err := unrepresentableOnWindows(hdr); err != nil { + log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err) + continue } // Ensure that the parent directory exists. - err = createImpliedDirectories(dest, hdr, options) + err = createImpliedDirectories(root, hdr, options) if err != nil { return 0, err } @@ -81,7 +83,7 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, // We don't want this directory, but we need the files in them so that // such hardlinks can be resolved. if strings.HasPrefix(hdr.Name, WhiteoutLinkDir) && hdr.Typeflag == tar.TypeReg { - basename := filepath.Base(hdr.Name) + basename := path.Base(hdr.Name) aufsHardlinks[basename] = hdr if aufsTempdir == "" { if aufsTempdir, err = os.MkdirTemp(dest, "dockerplnk"); err != nil { @@ -89,47 +91,63 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, } defer os.RemoveAll(aufsTempdir) } - if err := createTarFile(filepath.Join(aufsTempdir, basename), dest, hdr, tr, options); err != nil { + aufsRoot, err := os.OpenRoot(aufsTempdir) + if err != nil { return 0, err } + cerr := createTarFile(aufsRoot, basename, hdr, tr, options) + _ = aufsRoot.Close() + if cerr != nil { + return 0, cerr + } } if hdr.Name != WhiteoutOpaqueDir { continue } } - // #nosec G305 -- The joined path is guarded against path traversal. - dstPath := filepath.Join(dest, 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)) - } + // dstPath is the native (host-separator) form of the entry name, + // used at all filesystem boundaries (os.Root methods, fsRootPath). + // The tar-header name (hdr.Name) is POSIX, so convert it here. + dstPath := filepath.FromSlash(hdr.Name) base := filepath.Base(dstPath) if strings.HasPrefix(base, WhiteoutPrefix) { dir := filepath.Dir(dstPath) if base == WhiteoutOpaqueDir { - _, err := os.Lstat(dir) + _, err := root.Lstat(dir) if err != nil { return 0, err } - err = filepath.WalkDir(dir, func(path string, info os.DirEntry, err error) error { + // Walk the absolute directory so we can call os.RemoveAll on + // paths outside the walk callback's reach, then convert each + // walked path back to a root-relative name for the + // unpackedPaths check. + // fsRootPath walks each path component and bounds any symlinks + // within the root to prevent TOCTOU symlink attacks. + absDir, err := fsRootPath(root.Name(), dir) + if err != nil { + return 0, err + } + err = filepath.WalkDir(absDir, func(p string, info os.DirEntry, err error) error { if err != nil { if os.IsNotExist(err) { - err = nil // parent was deleted + return nil // parent was deleted } return err } - if path == dir { + if p == absDir { return nil } - if _, exists := unpackedPaths[path]; !exists { - return os.RemoveAll(path) // #nosec G122 -- FIXME: consider root-scoped APIs (e.g. os.Root) to prevent symlink TOCTOU traversal + rel, err := filepath.Rel(root.Name(), p) + if err != nil { + return err + } + + // unpackedPaths is keyed by root-relative slash paths; convert + // filepath.WalkDir's native path before looking it up. + if _, exists := unpackedPaths[filepath.ToSlash(rel)]; !exists { + return root.RemoveAll(rel) } return nil }) @@ -139,7 +157,7 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, } else { originalBase := base[len(WhiteoutPrefix):] originalPath := filepath.Join(dir, originalBase) - if err := os.RemoveAll(originalPath); err != nil { + if err := root.RemoveAll(originalPath); err != nil { return 0, err } } @@ -148,9 +166,9 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, // The only exception is when it is a directory *and* the file from // the layer is also a directory. Then we want to merge them (i.e. // just apply the metadata from the layer). - if fi, err := os.Lstat(dstPath); err == nil { + if fi, err := root.Lstat(dstPath); err == nil { if !fi.IsDir() || hdr.Typeflag != tar.TypeDir { - if err := os.RemoveAll(dstPath); err != nil { + if err := root.RemoveAll(dstPath); err != nil { return 0, err } } @@ -161,8 +179,8 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, // Hard links into /.wh..wh.plnk don't work, as we don't extract that directory, so // we manually retarget these into the temporary files we extracted them into - if hdr.Typeflag == tar.TypeLink && strings.HasPrefix(filepath.Clean(hdr.Linkname), WhiteoutLinkDir) { - linkBasename := filepath.Base(hdr.Linkname) + if hdr.Typeflag == tar.TypeLink && strings.HasPrefix(path.Clean(hdr.Linkname), WhiteoutLinkDir) { + linkBasename := path.Base(hdr.Linkname) srcHdr = aufsHardlinks[linkBasename] if srcHdr == nil { return 0, errors.New("invalid aufs hardlink") @@ -179,23 +197,23 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, return 0, err } - if err := createTarFile(dstPath, dest, srcHdr, srcData, options); err != nil { + if err := createTarFile(root, dstPath, srcHdr, srcData, options); err != nil { return 0, err } // Directory mtimes must be handled at the end to avoid further // file creation in them to modify the directory mtime if hdr.Typeflag == tar.TypeDir { - dirs = append(dirs, hdr) + dirs = append(dirs, unpackedDir{hdr: hdr, name: dstPath}) } - unpackedPaths[dstPath] = struct{}{} + // unpackedPaths is keyed by the POSIX (forward-slash) name so it + // matches the ToSlash'd lookup in the opaque-whiteout walk above. + unpackedPaths[hdr.Name] = struct{}{} } } - for _, hdr := range dirs { - // #nosec G305 -- The header was checked for path traversal before it was appended to the dirs slice. - dstPath := filepath.Join(dest, hdr.Name) - if err := chtimes(dstPath, hdr.AccessTime, hdr.ModTime); err != nil { + for _, d := range dirs { + if err := root.Chtimes(d.name, boundTime(latestTime(d.hdr.AccessTime, d.hdr.ModTime)), boundTime(d.hdr.ModTime)); err != nil { return 0, err } } diff --git a/rootpath.go b/rootpath.go new file mode 100644 index 0000000..fdf605f --- /dev/null +++ b/rootpath.go @@ -0,0 +1,112 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package archive + +import ( + "errors" + "os" + "path/filepath" +) + +var errTooManyLinks = errors.New("too many links") + +// fsRootPath joins a path with a root, evaluating and bounding any +// symlink to the root directory. +func fsRootPath(root, path string) (string, error) { + if path == "" { + return root, nil + } + var linksWalked int // to protect against cycles + for { + i := linksWalked + newpath, err := walkLinks(root, path, &linksWalked) + if err != nil { + return "", err + } + path = newpath + if i == linksWalked { + newpath = filepath.Join("/", newpath) + if path == newpath { + return filepath.Join(root, newpath), nil + } + path = newpath + } + } +} + +func walkLink(root, path string, linksWalked *int) (newpath string, islink bool, err error) { + if *linksWalked > 255 { + return "", false, errTooManyLinks + } + + path = filepath.Join("/", path) + if path == "/" { + return path, false, nil + } + realPath := filepath.Join(root, path) + + fi, err := os.Lstat(realPath) + if err != nil { + // If path does not yet exist, treat as non-symlink + if os.IsNotExist(err) { + return path, false, nil + } + return "", false, err + } + if fi.Mode()&os.ModeSymlink == 0 { + return path, false, nil + } + newpath, err = os.Readlink(realPath) + if err != nil { + return "", false, err + } + *linksWalked++ + return newpath, true, nil +} + +func walkLinks(root, path string, linksWalked *int) (string, error) { + switch dir, file := filepath.Split(path); { + case dir == "": + newpath, _, err := walkLink(root, file, linksWalked) + return newpath, err + case file == "": + if os.IsPathSeparator(dir[len(dir)-1]) { + if dir == "/" { + return dir, nil + } + return walkLinks(root, dir[:len(dir)-1], linksWalked) + } + newpath, _, err := walkLink(root, dir, linksWalked) + return newpath, err + default: + newdir, err := walkLinks(root, dir, linksWalked) + if err != nil { + return "", err + } + newpath, islink, err := walkLink(root, filepath.Join(newdir, file), linksWalked) + if err != nil { + return "", err + } + if !islink { + return newpath, nil + } + if filepath.IsAbs(newpath) { + return newpath, nil + } + return filepath.Join(newdir, newpath), nil + } +} diff --git a/sequential_other.go b/sequential_other.go new file mode 100644 index 0000000..90edb13 --- /dev/null +++ b/sequential_other.go @@ -0,0 +1,6 @@ +//go:build !windows + +package archive + +// windows_O_FILE_FLAG_SEQUENTIAL_SCAN is not supported on go < 1.26. +const windows_O_FILE_FLAG_SEQUENTIAL_SCAN = 0 diff --git a/sequential_windows_go126.go b/sequential_windows_go126.go new file mode 100644 index 0000000..1e80d11 --- /dev/null +++ b/sequential_windows_go126.go @@ -0,0 +1,9 @@ +//go:build windows && go1.26 + +package archive + +// windows_O_FILE_FLAG_SEQUENTIAL_SCAN matches [golang.org/x/sys/windows.O_FILE_FLAG_SEQUENTIAL_SCAN]. +// Starting in Go 1.26, os.OpenFile supports passing this flag through. +// +// TODO(thaJeztah): use windows.O_FILE_FLAG_SEQUENTIAL_SCAN once we drop Go <1.26. +const windows_O_FILE_FLAG_SEQUENTIAL_SCAN = 0x08000000 diff --git a/sequential_windows_pre126.go b/sequential_windows_pre126.go new file mode 100644 index 0000000..2b28174 --- /dev/null +++ b/sequential_windows_pre126.go @@ -0,0 +1,6 @@ +//go:build windows && !go1.26 + +package archive + +// windows_O_FILE_FLAG_SEQUENTIAL_SCAN is not supported on go < 1.26. +const windows_O_FILE_FLAG_SEQUENTIAL_SCAN = 0 diff --git a/time_nonwindows.go b/time_nonwindows.go index 5bfdfa2..f959b90 100644 --- a/time_nonwindows.go +++ b/time_nonwindows.go @@ -3,7 +3,12 @@ package archive import ( + "errors" "os" + "path" + "path/filepath" + "strings" + "syscall" "time" "golang.org/x/sys/unix" @@ -28,14 +33,33 @@ func timeToTimespec(time time.Time) unix.Timespec { return unix.NsecToTimespec(time.UnixNano()) } -func lchtimes(name string, atime time.Time, mtime time.Time) error { +func lchtimes(root *os.Root, name string, atime, mtime time.Time) error { + dir, base := path.Split(filepath.ToSlash(name)) + if base == "" { + return &os.PathError{Op: "lchtimes", Path: name, Err: syscall.EINVAL} + } + + dir = strings.TrimSuffix(dir, "/") + if dir == "" { + dir = "." + } + + parent, err := root.Open(dir) + if err != nil { + return err + } + defer parent.Close() + utimes := [2]unix.Timespec{ timeToTimespec(atime), timeToTimespec(mtime), } - err := unix.UtimesNanoAt(unix.AT_FDCWD, name, utimes[0:], unix.AT_SYMLINK_NOFOLLOW) - if err != nil && err != unix.ENOSYS { - return err + // #nosec G115 -- ignore integer overflow conversion for parent.Fd + if err := unix.UtimesNanoAt(int(parent.Fd()), base, utimes[:], unix.AT_SYMLINK_NOFOLLOW); err != nil { + if errors.Is(err, unix.ENOSYS) { + return nil + } + return &os.PathError{Op: "lchtimes", Path: name, Err: err} } - return err + return nil } diff --git a/time_windows.go b/time_windows.go index af1f7c8..c4a007f 100644 --- a/time_windows.go +++ b/time_windows.go @@ -27,6 +27,6 @@ func chtimes(name string, atime time.Time, mtime time.Time) error { return windows.SetFileTime(h, &c, nil, nil) } -func lchtimes(name string, atime time.Time, mtime time.Time) error { +func lchtimes(root *os.Root, name string, atime time.Time, mtime time.Time) error { return nil } diff --git a/utils_test.go b/utils_test.go index fb0a1ac..3d2ea0c 100644 --- a/utils_test.go +++ b/utils_test.go @@ -96,7 +96,7 @@ func testBreakout(untarFn string, tmpdir string, headers []*tar.Header) error { } if err := untar(dest, reader); err != nil { var boErr *breakoutErr - if !errors.As(err, &boErr) { + if !errors.As(err, &boErr) && !isPathEscapes(err) { // If untar returns an error unrelated to an archive breakout, // then consider this an unexpected error and abort. return err @@ -160,6 +160,12 @@ func testBreakout(untarFn string, tmpdir string, headers []*tar.Header) error { // Since victim/hello was generated with time.Now(), it is safe to assume // that any file whose content matches exactly victim/hello, managed somehow // to access victim/hello. + // + // Symlinks are intentionally skipped: the os.Root security model allows + // extracting symlinks with targets outside the root (since the node itself + // is inside the root), and subsequent access through os.Root-bounded + // operations will catch any attempted escape. A symlink whose target + // resolves outside root does not constitute a breakout on its own. return filepath.WalkDir(dest, func(path string, info os.DirEntry, err error) error { if info.IsDir() { if err != nil { @@ -173,6 +179,11 @@ func testBreakout(untarFn string, tmpdir string, headers []*tar.Header) error { // skip file if error return nil } + // Skip symlinks: their targets may point outside the root, but that + // is safe under the os.Root access model. + if info.Type()&os.ModeSymlink != 0 { + return nil + } b, err := os.ReadFile(path) // #nosec G122 -- TOCTOU / filesystem traversal safe to ignore for tests. if err != nil { // Houston, we have a problem. Aborting (space)walk. From f43ca4dfa3054c1ff02295d5f40a7cc391225ae4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 17 Jul 2026 14:16:13 +0200 Subject: [PATCH 3/8] archive: handleLChmod: fall back when os.Root.Chmod is unsupported os.Root.Chmod relies on chmodat(AT_SYMLINK_NOFOLLOW), which is not supported on all Linux kernels and filesystems. When that operation fails with ENOTSUP or EOPNOTSUPP, fall back to chmod relative to the resolved parent directory. Symlink entries are excluded beforehand, and hardlink entries are only chmod'd when their target is not a symlink, preserving the existing no-follow semantics. Signed-off-by: Sebastiaan van Stijn --- archive_unix.go | 75 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 9 deletions(-) diff --git a/archive_unix.go b/archive_unix.go index fef0b31..108ea69 100644 --- a/archive_unix.go +++ b/archive_unix.go @@ -8,6 +8,7 @@ import ( "fmt" "math" "os" + "path" "path/filepath" "strings" "syscall" @@ -87,16 +88,72 @@ func handleTarTypeBlockCharFifo(hdr *tar.Header, dstPath string) error { // symlinks (there is no lchmod). For hardlinks, the mode is applied only when // the link target is itself not a symlink. func handleLChmod(root *os.Root, dstPath string, hdr *tar.Header, hdrInfo os.FileInfo) error { - if hdr.Typeflag == tar.TypeLink { - if fi, err := root.Lstat(filepath.FromSlash(hdr.Linkname)); err == nil && (fi.Mode()&os.ModeSymlink == 0) { - if err := root.Chmod(dstPath, hdrInfo.Mode()); err != nil { - return err - } - } - } else if hdr.Typeflag != tar.TypeSymlink { - if err := root.Chmod(dstPath, hdrInfo.Mode()); err != nil { - return err + switch hdr.Typeflag { + case tar.TypeSymlink: + return nil + + case tar.TypeLink: + // If the target is a symlink, there is no way to chmod the hardlink + // without following it. + fi, err := root.Lstat(filepath.FromSlash(path.Clean(hdr.Linkname))) + if err != nil || fi.Mode()&os.ModeSymlink != 0 { + return nil } + return chmodNoSymlink(root, dstPath, hdrInfo.Mode()) + + default: + return chmodNoSymlink(root, dstPath, hdrInfo.Mode()) + } +} + +// chmodNoSymlink applies mode to a non-symlink entry. +// +// Callers must have already excluded symlink entries. +func chmodNoSymlink(root *os.Root, name string, mode os.FileMode) error { + parent, err := root.OpenFile(filepath.Dir(name), os.O_RDONLY, 0) + if err != nil { + return err + } + defer parent.Close() + + base := filepath.Base(name) + perm := fileModeToPerm(mode) + // #nosec G115 -- ignore integer overflow conversion for parent.Fd + if err := unix.Fchmodat(int(parent.Fd()), base, perm, unix.AT_SYMLINK_NOFOLLOW); err == nil { + return nil + } else if !errors.Is(err, syscall.EOPNOTSUPP) { + return &os.PathError{Op: "fchmodat2", Path: name, Err: err} + } + + // Fallback for systems that cannot perform fchmodat with AT_SYMLINK_NOFOLLOW. + // Open the entry without following symlinks and apply the mode through the + // resulting file descriptor. + // #nosec G115 -- ignore integer overflow conversion for parent.Fd + fd, err := unix.Openat(int(parent.Fd()), base, unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0) + if err != nil { + return &os.PathError{Op: "openat", Path: name, Err: err} + } + defer unix.Close(fd) + + if err := unix.Fchmod(fd, perm); err != nil { + return &os.PathError{Op: "fchmod", Path: name, Err: err} } return nil } + +// fileModeToPerm returns the subset of an os.FileMode that can be applied +// by chmod. +func fileModeToPerm(mode os.FileMode) uint32 { + perm := uint32(mode.Perm()) + + if mode&os.ModeSetuid != 0 { + perm |= unix.S_ISUID + } + if mode&os.ModeSetgid != 0 { + perm |= unix.S_ISGID + } + if mode&os.ModeSticky != 0 { + perm |= unix.S_ISVTX + } + return perm +} From b6b52c77884e5e174b6aafbc383e26a8f0a4b085 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 20 Jul 2026 20:02:22 +0200 Subject: [PATCH 4/8] Unpack: move createImpliedDirectories later Some code-paths may return an error, in which case creating the parent paths isn't needed. Move it later in the function to avoid this. Signed-off-by: Sebastiaan van Stijn --- archive.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/archive.go b/archive.go index a45fb79..2c4e29a 100644 --- a/archive.go +++ b/archive.go @@ -945,12 +945,6 @@ loop: // hdr.Name stays POSIX (forward-slash) for logical string checks. dstPath := filepath.FromSlash(hdr.Name) - // Ensure that the parent directory exists. - err = createImpliedDirectories(root, hdr, options) - if err != nil { - return err - } - // If dstPath exists we almost always just want to remove and replace it. // The only exception is when it is a directory *and* the file from // the layer is also a directory. Then we want to merge them (i.e. @@ -983,6 +977,14 @@ loop: return err } + // Ensure that the parent directory exists. + // + // This must be done before whiteoutConverter.ConvertRead, which + // may set xattrs on the directory or create whiteout files. + if err := createImpliedDirectories(root, hdr, options); err != nil { + return err + } + if whiteoutConverter != nil { // ConvertRead implementations (e.g. overlayWhiteoutConverter) // make direct syscalls with the path, so they need the absolute From a450ae0d17895e598da1c8b0a6c5b89b105d48e6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 21 Jul 2026 22:17:37 +0200 Subject: [PATCH 5/8] archive: reuse directory handle for implied directory metadata Open newly-created implied directories once and apply ownership and permissions through the retained handle instead of separate os.Root operations. This avoids repeated doInRoot path resolution for Lchown/Chmod while continuing to apply metadata only to newly-created directories. Signed-off-by: Sebastiaan van Stijn --- archive.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/archive.go b/archive.go index 2c4e29a..0ee7fac 100644 --- a/archive.go +++ b/archive.go @@ -1097,8 +1097,13 @@ func createImpliedDirectories(root *os.Root, hdr *tar.Header, options *TarOption continue } // Only the successful Mkdir case is newly-created. + dir, err := root.Open(cur) + if err != nil { + return err + } if uid != 0 || gid != 0 { - if err := root.Lchown(cur, uid, gid); err != nil { + if err := dir.Chown(uid, gid); err != nil { + _ = dir.Close() return err } } @@ -1106,7 +1111,11 @@ func createImpliedDirectories(root *os.Root, hdr *tar.Header, options *TarOption // re-apply it with Chmod to guarantee ImpliedDirectoryMode // independent of umask, matching the previous MkdirAllAndChown // behavior. - if err := root.Chmod(cur, ImpliedDirectoryMode); err != nil { + if err := dir.Chmod(ImpliedDirectoryMode); err != nil { + _ = dir.Close() + return err + } + if err := dir.Close(); err != nil { return err } } From dd5ba1991b4446462438629689e0d5be97bda9fb Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 Jul 2026 03:27:38 +0200 Subject: [PATCH 6/8] archive: avoid redundant path resolution for whiteout conversion Pass the extraction root to the overlay whiteout converter and operate relative to opened directories instead of resolving absolute paths with fsRootPath. This avoids redundant path resolution before os.Root-based operations while reducing the TOCTOU surface. Signed-off-by: Sebastiaan van Stijn --- archive.go | 11 ++--------- archive_linux.go | 27 ++++++++++++++++++++------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/archive.go b/archive.go index 0ee7fac..fc9bfc5 100644 --- a/archive.go +++ b/archive.go @@ -287,7 +287,7 @@ func ReadSecurityXattrToTarHeader(filePath string, hdr *tar.Header) error { type tarWhiteoutConverter interface { ConvertWrite(*tar.Header, string, os.FileInfo) (*tar.Header, error) - ConvertRead(*tar.Header, string) (bool, error) + ConvertRead(*os.Root, *tar.Header, string) (bool, error) } type tarAppender struct { @@ -986,14 +986,7 @@ loop: } if whiteoutConverter != nil { - // ConvertRead implementations (e.g. overlayWhiteoutConverter) - // make direct syscalls with the path, so they need the absolute - // path bounded within dest rather than the root-relative name. - absPath, err := fsRootPath(root.Name(), dstPath) - if err != nil { - return err - } - writeFile, err := whiteoutConverter.ConvertRead(hdr, absPath) + writeFile, err := whiteoutConverter.ConvertRead(root, hdr, dstPath) if err != nil { return err } diff --git a/archive_linux.go b/archive_linux.go index bddef78..9341bc5 100644 --- a/archive_linux.go +++ b/archive_linux.go @@ -76,7 +76,7 @@ func (c overlayWhiteoutConverter) ConvertWrite(hdr *tar.Header, filePath string, }, nil } -func (c overlayWhiteoutConverter) ConvertRead(hdr *tar.Header, filePath string) (bool, error) { +func (c overlayWhiteoutConverter) ConvertRead(root *os.Root, hdr *tar.Header, filePath string) (bool, error) { base := filepath.Base(filePath) dir := filepath.Dir(filePath) @@ -85,9 +85,15 @@ func (c overlayWhiteoutConverter) ConvertRead(hdr *tar.Header, filePath string) return false, fmt.Errorf("invalid whiteout entry %q", hdr.Name) case WhiteoutOpaqueDir: + parent, err := root.Open(dir) + if err != nil { + return false, err + } + defer parent.Close() + // If a directory is marked as opaque by the AUFS special file, we need to translate that to overlay. - if err := unix.Setxattr(dir, c.opaqueXattr, []byte{'y'}, 0); err != nil { - return false, fmt.Errorf("setxattr('%s', %s=y): %w", dir, c.opaqueXattr, err) + if err := unix.Fsetxattr(int(parent.Fd()), c.opaqueXattr, []byte{'y'}, 0); err != nil { + return false, fmt.Errorf("fsetxattr('%s', %s=y): %w", dir, c.opaqueXattr, err) } // Don't write the whiteout file itself. return false, nil @@ -98,9 +104,16 @@ func (c overlayWhiteoutConverter) ConvertRead(hdr *tar.Header, filePath string) // Regular file. return true, nil } + + parent, err := root.Open(dir) + if err != nil { + return false, err + } + defer parent.Close() + // If a file was deleted, and we are using overlay, we need to create a character device. originalPath := filepath.Join(dir, originalBase) - if err := unix.Mknod(originalPath, unix.S_IFCHR, 0); err != nil { + if err := unix.Mknodat(int(parent.Fd()), originalBase, unix.S_IFCHR, 0); err != nil { return false, fmt.Errorf("failed to mknod('%s', S_IFCHR, 0): %w", originalPath, err) } @@ -117,9 +130,9 @@ func (c overlayWhiteoutConverter) ConvertRead(hdr *tar.Header, filePath string) // OverlayFS documents whiteouts in terms of a character device with device // number 0:0, not ownership: https://docs.kernel.org/filesystems/overlayfs.html#whiteouts-and-opaque-directories // - // If ownership is not required, this Lchown can be removed to avoid the remaining TOCTOU window. - if err := os.Lchown(originalPath, hdr.Uid, hdr.Gid); err != nil { - return false, err + // If ownership is not required, this Fchownat can be removed to avoid the remaining TOCTOU window. + if err := unix.Fchownat(int(parent.Fd()), originalBase, hdr.Uid, hdr.Gid, unix.AT_SYMLINK_NOFOLLOW); err != nil { + return false, &os.PathError{Op: "lchown", Path: originalPath, Err: err} } } From 51d1dd0275e73cb73e656cbf2a597303695de11f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 Jul 2026 12:29:28 +0200 Subject: [PATCH 7/8] archive: use os.Root for mknod path resolution Pass os.Root into handleTarTypeBlockCharFifo and perform the filesystem operation relative to the opened parent directory instead of constructing an absolute path. This removes an fsRootPath call, avoids an extra pathname resolution, and keeps the operation within os.Root. Signed-off-by: Sebastiaan van Stijn --- archive.go | 24 +++++------------------- archive_unix.go | 5 +++-- archive_unix_test.go | 6 +++++- archive_windows.go | 2 +- dev_darwin.go | 21 +++++++++++++++++++++ dev_freebsd.go | 17 ++++++++++++++++- dev_unix.go | 19 +++++++++++++++++-- 7 files changed, 68 insertions(+), 26 deletions(-) create mode 100644 dev_darwin.go diff --git a/archive.go b/archive.go index fc9bfc5..1654a07 100644 --- a/archive.go +++ b/archive.go @@ -460,13 +460,6 @@ func createTarFile(root *os.Root, dstPath string, hdr *tar.Header, reader io.Rea // so use hdrInfo.Mode() (they differ for e.g. setuid bits) hdrInfo := hdr.FileInfo() - // absPath is computed lazily. It is only required for mknod and xattrs. - // Symlinks intentionally use root.Symlink directly to preserve absolute - // targets; os.Root.Symlink rejects absolute targets like /usr/lib. - absPath := sync.OnceValues(func() (string, error) { - return fsRootPath(root.Name(), dstPath) - }) - switch hdr.Typeflag { case tar.TypeDir: // Create directory unless it already exists as one; merge in that case. @@ -501,22 +494,12 @@ func createTarFile(root *os.Root, dstPath string, hdr *tar.Header, reader io.Rea log.G(context.TODO()).WithFields(log.Fields{"path": dstPath, "type": hdr.Typeflag}).Debug("skipping device nodes in a userns") return nil } - // os.Root has no mknod support; use absPath so the path stays bounded. - ap, err := absPath() - if err != nil { - return err - } - if err := handleTarTypeBlockCharFifo(hdr, ap); err != nil { + if err := handleTarTypeBlockCharFifo(root, hdr, dstPath); err != nil { return err } case tar.TypeFifo: - // os.Root has no mknod support; use absPath so the path stays bounded. - ap, err := absPath() - if err != nil { - return err - } - if err := handleTarTypeBlockCharFifo(hdr, ap); err != nil { + if err := handleTarTypeBlockCharFifo(root, hdr, dstPath); err != nil { if inUserns && errors.Is(err, syscall.EPERM) { // In most cases, cannot create a fifo if running in user namespace log.G(context.TODO()).WithFields(log.Fields{"error": err, "path": dstPath, "type": hdr.Typeflag}).Debug("creating fifo node in a userns") @@ -575,6 +558,9 @@ func createTarFile(root *os.Root, dstPath string, hdr *tar.Header, reader io.Rea } var xattrErrs []string + absPath := sync.OnceValues(func() (string, error) { + return fsRootPath(root.Name(), dstPath) + }) for key, value := range hdr.PAXRecords { xattr, ok := strings.CutPrefix(key, paxSchilyXattr) if !ok { diff --git a/archive_unix.go b/archive_unix.go index 108ea69..e2dcc42 100644 --- a/archive_unix.go +++ b/archive_unix.go @@ -60,7 +60,7 @@ func getFileUIDGID(stat any) (int, int, error) { // // Creating device nodes is not supported when running in a user namespace, // produces a [syscall.EPERM] in most cases. -func handleTarTypeBlockCharFifo(hdr *tar.Header, dstPath string) error { +func handleTarTypeBlockCharFifo(root *os.Root, hdr *tar.Header, dstPath string) error { mode := uint32(hdr.Mode & 0o7777) switch hdr.Typeflag { case tar.TypeBlock: @@ -81,7 +81,8 @@ func handleTarTypeBlockCharFifo(hdr *tar.Header, dstPath string) error { return fmt.Errorf("device number %d:%d for %q out of range: %w", hdr.Devmajor, hdr.Devminor, hdr.Name, errInvalidArchive) } - return mknod(dstPath, mode, unix.Mkdev(uint32(hdr.Devmajor), uint32(hdr.Devminor))) + // Prefer mknodat; fall back to a bounded path where unavailable. + return mknodInRoot(root, dstPath, mode, unix.Mkdev(uint32(hdr.Devmajor), uint32(hdr.Devminor))) } // handleLChmod applies the mode from hdrInfo to dstPath within root, skipping diff --git a/archive_unix_test.go b/archive_unix_test.go index 5a2903d..0399ea2 100644 --- a/archive_unix_test.go +++ b/archive_unix_test.go @@ -491,7 +491,11 @@ func TestHandleTarTypeBlockCharFifoDeviceRange(t *testing.T) { Devmajor: tc.devmajor, Devminor: tc.devminor, } - err := handleTarTypeBlockCharFifo(hdr, filepath.Join(t.TempDir(), "dev")) + + // A nil root is sufficient here: invalid device numbers must + // be rejected before attempting any filesystem operation. + var root *os.Root + err := handleTarTypeBlockCharFifo(root, hdr, "dev") if !errors.Is(err, errInvalidArchive) { t.Fatalf("expected errInvalidArchive for %d:%d, got %v", tc.devmajor, tc.devminor, err) } diff --git a/archive_windows.go b/archive_windows.go index 70efa4a..ffe00a4 100644 --- a/archive_windows.go +++ b/archive_windows.go @@ -48,7 +48,7 @@ func getInodeFromStat(stat any) (uint64, error) { // handleTarTypeBlockCharFifo is an OS-specific helper function used by // createTarFile to handle the following types of header: Block; Char; Fifo -func handleTarTypeBlockCharFifo(hdr *tar.Header, path string) error { +func handleTarTypeBlockCharFifo(root *os.Root, hdr *tar.Header, path string) error { return nil } diff --git a/dev_darwin.go b/dev_darwin.go new file mode 100644 index 0000000..fcfe730 --- /dev/null +++ b/dev_darwin.go @@ -0,0 +1,21 @@ +//go:build darwin + +package archive + +import ( + "os" + + "golang.org/x/sys/unix" +) + +func mknod(path string, mode uint32, dev uint64) error { + return unix.Mknod(path, mode, int(dev)) // #nosec G115 -- Required conversion for the platform-specific Mknod API. +} + +func mknodInRoot(root *os.Root, path string, mode uint32, dev uint64) error { + abs, err := fsRootPath(root.Name(), path) + if err != nil { + return err + } + return unix.Mknod(abs, mode, int(dev)) // #nosec G115 -- Required conversion for the platform-specific Mknod API. +} diff --git a/dev_freebsd.go b/dev_freebsd.go index b3068fc..d18e829 100644 --- a/dev_freebsd.go +++ b/dev_freebsd.go @@ -2,8 +2,23 @@ package archive -import "golang.org/x/sys/unix" +import ( + "os" + "path/filepath" + + "golang.org/x/sys/unix" +) func mknod(path string, mode uint32, dev uint64) error { return unix.Mknod(path, mode, dev) } + +func mknodInRoot(root *os.Root, path string, mode uint32, dev uint64) error { + parent, err := root.OpenFile(filepath.Dir(path), os.O_RDONLY|unix.O_DIRECTORY, 0) + if err != nil { + return err + } + defer parent.Close() + + return unix.Mknodat(int(parent.Fd()), filepath.Base(path), mode, dev) +} diff --git a/dev_unix.go b/dev_unix.go index 2f9833b..0eb8b6d 100644 --- a/dev_unix.go +++ b/dev_unix.go @@ -1,9 +1,24 @@ -//go:build !windows && !freebsd +//go:build !darwin && !freebsd && !windows package archive -import "golang.org/x/sys/unix" +import ( + "os" + "path/filepath" + + "golang.org/x/sys/unix" +) func mknod(path string, mode uint32, dev uint64) error { return unix.Mknod(path, mode, int(dev)) // #nosec G115 -- Required conversion for the platform-specific Mknod API. } + +func mknodInRoot(root *os.Root, path string, mode uint32, dev uint64) error { + parent, err := root.OpenFile(filepath.Dir(path), os.O_RDONLY|unix.O_DIRECTORY, 0) + if err != nil { + return err + } + defer parent.Close() + + return unix.Mknodat(int(parent.Fd()), filepath.Base(path), mode, int(dev)) // #nosec G115 -- Required conversion for the platform-specific Mknod API. +} From 47e37ddcd67bb3094fe4e284b938485503e49374 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 22 Jul 2026 20:16:07 +0200 Subject: [PATCH 8/8] fsRootPath: use platform-native path-separators Signed-off-by: Sebastiaan van Stijn --- rootpath.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rootpath.go b/rootpath.go index fdf605f..3834af2 100644 --- a/rootpath.go +++ b/rootpath.go @@ -39,7 +39,7 @@ func fsRootPath(root, path string) (string, error) { } path = newpath if i == linksWalked { - newpath = filepath.Join("/", newpath) + newpath = filepath.Join(string(os.PathSeparator), newpath) if path == newpath { return filepath.Join(root, newpath), nil } @@ -53,8 +53,8 @@ func walkLink(root, path string, linksWalked *int) (newpath string, islink bool, return "", false, errTooManyLinks } - path = filepath.Join("/", path) - if path == "/" { + path = filepath.Join(string(os.PathSeparator), path) + if path == string(os.PathSeparator) { return path, false, nil } realPath := filepath.Join(root, path) @@ -85,7 +85,7 @@ func walkLinks(root, path string, linksWalked *int) (string, error) { return newpath, err case file == "": if os.IsPathSeparator(dir[len(dir)-1]) { - if dir == "/" { + if dir == string(os.PathSeparator) { return dir, nil } return walkLinks(root, dir[:len(dir)-1], linksWalked)