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

archive_test.go

@@ -585,7 +585,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)
 	}
@@ -918,6 +923,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

@@ -70,15 +70,18 @@ func handleTarTypeBlockCharFifo(hdr *tar.Header, path string) error {
 	return mknod(path, mode, unix.Mkdev(uint32(hdr.Devmajor), uint32(hdr.Devminor)))
 }
 
-func handleLChmod(hdr *tar.Header, path string, hdrInfo os.FileInfo) error {
+// handleLChmod applies the mode from hdrInfo to name 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, name 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(path, hdrInfo.Mode()); err != nil {
+		if fi, err := root.Lstat(hdr.Linkname); err == nil && (fi.Mode()&os.ModeSymlink == 0) {
+			if err := root.Chmod(name, hdrInfo.Mode()); err != nil {
 				return err
 			}
 		}
 	} else if hdr.Typeflag != tar.TypeSymlink {
-		if err := os.Chmod(path, hdrInfo.Mode()); err != nil {
+		if err := root.Chmod(name, hdrInfo.Mode()); err != nil {
 			return err
 		}
 	}

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, name 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

diff.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -20,9 +21,19 @@ 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) {
+	// Open an os.Root for dest so that all extraction operations are bounded
+	// within dest at the OS level using openat(2) semantics.
+	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 {
@@ -48,8 +59,14 @@ 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)
+		// Strip a leading "/" so absolute entries stay root-relative, then
+		// Clean while keeping any ".." so the IsLocal check below rejects
+		// escapes instead of silently rewriting them.
+		hdr.Name = path.Clean(strings.TrimLeft(hdr.Name, "/"))
+		// Reject names that escape the extraction root (absolute or "..").
+		if !filepath.IsLocal(hdr.Name) {
+			return 0, breakoutError(fmt.Errorf("invalid entry name %q", hdr.Name))
+		}
 
 		// Windows does not support filenames with colons in them. Ignore
 		// these files. This is not a problem though (although it might
@@ -73,14 +90,14 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 		}
 
 		// Ensure that the parent directory exists.
-		err = createImpliedDirectories(dest, hdr, options)
+		err = createImpliedDirectories(root, hdr, options)
 		if err != nil {
 			return 0, err
 		}
 
 		// Skip AUFS metadata dirs
 		if strings.HasPrefix(hdr.Name, WhiteoutMetaPrefix) {
-			// Regular files inside /.wh..wh.plnk can be used as hardlink targets
+			// Regular files inside /.wh..wh.plnk can be used as hardlink targets.
 			// 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 {
@@ -92,47 +109,64 @@ 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 {
+				// aufsTempdir is outside dest, so open a separate root for it.
+				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.
-		path := filepath.Join(dest, hdr.Name)
-		rel, err := filepath.Rel(dest, path)
-		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))
-		}
+		// path is the root-relative name of the entry being processed.
+		path := hdr.Name
 		base := filepath.Base(path)
 
 		if strings.HasPrefix(base, WhiteoutPrefix) {
 			dir := filepath.Dir(path)
 			if base == WhiteoutOpaqueDir {
-				_, err := os.Lstat(dir)
+				_, err := root.Lstat(dir)
+				if err != nil {
+					return 0, err
+				}
+				// 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.
+				// safeResolve walks each path component and bounds any symlinks
+				// within the root to prevent TOCTOU symlink attacks.
+				absDir, err := safeResolve(root.Name(), dir)
 				if err != nil {
 					return 0, err
 				}
-				err = filepath.WalkDir(dir, func(path string, info os.DirEntry, err error) error {
+				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 err
 					}
-					if path == dir {
+					if p == absDir {
 						return nil
 					}
-					if _, exists := unpackedPaths[path]; !exists {
-						return os.RemoveAll(path)
+					// unpackedPaths is keyed by the root-relative, forward-slash
+					// form of hdr.Name (the result of path.Join("/", ...) in
+					// the normalisation step above). filepath.WalkDir yields
+					// OS-native separators, so convert to slash form before
+					// looking up so the comparison works on Windows too.
+					rel := filepath.ToSlash(strings.TrimPrefix(
+						p, root.Name()+string(os.PathSeparator),
+					))
+					if _, exists := unpackedPaths[rel]; !exists {
+						return os.RemoveAll(p)
 					}
 					return nil
 				})
@@ -142,18 +176,18 @@ 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
 				}
 			}
 		} else {
-			// If path exits 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(path); err == nil {
+			// If path 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 := root.Lstat(path); err == nil {
 				if !fi.IsDir() || hdr.Typeflag != tar.TypeDir {
-					if err := os.RemoveAll(path); err != nil {
+					if err := root.RemoveAll(path); err != nil {
 						return 0, err
 					}
 				}
@@ -162,8 +196,9 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 			srcData := io.Reader(tr)
 			srcHdr := hdr
 
-			// 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
+			// 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)
 				srcHdr = aufsHardlinks[linkBasename]
@@ -182,23 +217,21 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 				return 0, err
 			}
 
-			if err := createTarFile(path, dest, srcHdr, srcData, options); err != nil {
+			if err := createTarFile(root, path, 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
+			// 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: path})
 			}
 			unpackedPaths[path] = struct{}{}
 		}
 	}
 
-	for _, hdr := range dirs {
-		// #nosec G305 -- The header was checked for path traversal before it was appended to the dirs slice.
-		path := filepath.Join(dest, hdr.Name)
-		if err := chtimes(path, hdr.AccessTime, hdr.ModTime); err != nil {
+	for _, d := range dirs {
+		if err := root.Chtimes(d.name, d.hdr.AccessTime, d.hdr.ModTime); err != nil {
 			return 0, err
 		}
 	}