archive: harden tar extraction against path traversal

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}
 			}
 		}
 

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, "")

archive_unix.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"math"
 	"os"
+	"path"
 	"path/filepath"
 	"strings"
 	"syscall"
@@ -59,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:
@@ -80,20 +81,80 @@ 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)))
 }
 
-func handleLChmod(hdr *tar.Header, dstPath string, 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 {
-				return err
-			}
-		}
-	} else if hdr.Typeflag != tar.TypeSymlink {
-		if err := os.Chmod(dstPath, hdrInfo.Mode()); err != nil {
-			return err
+// 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 {
+	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
+}

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)
 			}

archive_windows.go

@@ -48,11 +48,12 @@ 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
 }
 
-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
 }
 

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

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.
+}

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)
+}

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.
+}