fix(security): extract archives through an os.Root handle

cli/cookbook/extract.go

@@ -31,8 +31,16 @@ var (
 // archive creates (Supermarket tarballs are rooted at <cookbook>/...).
 //
 // Entries whose paths escape destDir are refused, so a hostile tarball
-// can't write outside the destination.
+// can't write outside the destination. Every write goes through an os.Root
+// handle on destDir, so the kernel refuses an escape even when the lexical
+// check cannot see one (a symlink already sitting in destDir, say).
 func ExtractArchive(r io.Reader, destDir string) (string, error) {
+	destRoot, err := os.OpenRoot(destDir)
+	if err != nil {
+		return "", fmt.Errorf("open destination %s: %w", destDir, err)
+	}
+	defer func() { _ = destRoot.Close() }()
+
 	gz, err := gzip.NewReader(r)
 	if err != nil {
 		return "", fmt.Errorf("open gzip: %w", err)
@@ -51,7 +59,7 @@ func ExtractArchive(r io.Reader, destDir string) (string, error) {
 			return "", fmt.Errorf("read tar: %w", err)
 		}
 
-		target, err := safeJoin(destDir, hdr.Name)
+		rel, err := safeRel(destDir, hdr.Name)
 		if err != nil {
 			return "", err
 		}
@@ -61,11 +69,11 @@ func ExtractArchive(r io.Reader, destDir string) (string, error) {
 
 		switch hdr.Typeflag {
 		case tar.TypeDir:
-			if err := os.MkdirAll(target, extractDirMode); err != nil {
-				return "", fmt.Errorf("mkdir %s: %w", target, err)
+			if err := destRoot.MkdirAll(rel, extractDirMode); err != nil {
+				return "", fmt.Errorf("mkdir %s: %w", rel, err)
 			}
 		case tar.TypeReg:
-			if err := writeFile(tr, target, hdr.Name, &total); err != nil {
+			if err := writeFile(destRoot, tr, rel, hdr.Name, &total); err != nil {
 				return "", err
 			}
 		default:
@@ -83,15 +91,18 @@ func ExtractArchive(r io.Reader, destDir string) (string, error) {
 	return filepath.Join(destDir, root), nil
 }
 
-// safeJoin joins name onto destDir and verifies the result stays within
-// destDir, guarding against path-traversal ("zip slip") entries.
-func safeJoin(destDir, name string) (string, error) {
+// safeRel resolves a tar entry name to a path relative to destDir, refusing
+// entries that would escape it ("zip slip"). The result is what the os.Root
+// handle is asked to create, so containment is checked twice: lexically here,
+// for a clear error naming the offending entry, and again by the kernel when
+// the file is actually opened.
+func safeRel(destDir, name string) (string, error) {
 	target := filepath.Join(destDir, filepath.FromSlash(name))
 	rel, err := filepath.Rel(destDir, target)
 	if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
 		return "", fmt.Errorf("archive entry %q escapes the destination directory", name)
 	}
-	return target, nil
+	return rel, nil
 }
 
 // topLevel returns the first real path segment of a tar entry name, or ""
@@ -107,23 +118,25 @@ func topLevel(name string) string {
 	return root
 }
 
-// writeFile creates target (with parent directories) and copies the
+// writeFile creates rel (with parent directories) under root and copies the
 // current tar entry into it, capping output so a zip bomb can't fill the
 // disk. total accumulates across every entry in one archive.
-func writeFile(r io.Reader, target, name string, total *int64) error {
-	if err := os.MkdirAll(filepath.Dir(target), extractDirMode); err != nil {
-		return fmt.Errorf("mkdir %s: %w", filepath.Dir(target), err)
+func writeFile(root *os.Root, r io.Reader, rel, name string, total *int64) error {
+	if dir := filepath.Dir(rel); dir != "." {
+		if err := root.MkdirAll(dir, extractDirMode); err != nil {
+			return fmt.Errorf("mkdir %s: %w", dir, err)
+		}
 	}
-	f, err := os.OpenFile(target, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode)
+	f, err := root.OpenFile(rel, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode)
 	if err != nil {
-		return fmt.Errorf("create %s: %w", target, err)
+		return fmt.Errorf("create %s: %w", rel, err)
 	}
 	if err := boundedCopy(f, r, name, total); err != nil {
 		_ = f.Close() // already returning an error
-		return fmt.Errorf("write %s: %w", target, err)
+		return fmt.Errorf("write %s: %w", rel, err)
 	}
 	if err := f.Close(); err != nil {
-		return fmt.Errorf("close %s: %w", target, err)
+		return fmt.Errorf("close %s: %w", rel, err)
 	}
 	return nil
 }

cli/cookbook/extract_test.go

@@ -195,3 +195,25 @@ func TestExtractArchiveClampsFileMode(t *testing.T) {
 		t.Errorf("extracted file mode = %o, want %o", got, extractFileMode)
 	}
 }
+
+// TestExtractArchiveDoesNotFollowSymlinkOutOfDest covers the hole a purely
+// lexical containment check leaves open. "nginx/metadata.rb" is inside destDir
+// by every string comparison, so safeJoin passes it; if destDir already holds
+// a "nginx" symlink pointing elsewhere, the create still lands on the far side
+// of that link. Containment has to be enforced when the file is opened, not
+// when its name is computed.
+func TestExtractArchiveDoesNotFollowSymlinkOutOfDest(t *testing.T) {
+	dest := t.TempDir()
+	outside := t.TempDir()
+	if err := os.Symlink(outside, filepath.Join(dest, "nginx")); err != nil {
+		t.Skipf("symlinks unavailable: %v", err)
+	}
+
+	archive := buildCookbookTarball(t, map[string]string{"nginx/metadata.rb": "pwned"})
+	if _, err := ExtractArchive(bytes.NewReader(archive), dest); err == nil {
+		t.Error("ExtractArchive wrote through a symlink in destDir without complaint")
+	}
+	if _, err := os.Stat(filepath.Join(outside, "metadata.rb")); err == nil {
+		t.Fatal("archive entry escaped destDir through a pre-existing symlink")
+	}
+}

cli/policyfile/extract.go

@@ -58,8 +58,16 @@
 // entries under dest, stripping the single leading path segment that
 // Supermarket tarballs wrap a cookbook in (e.g. "nginx/metadata.rb" lands at
 // dest/metadata.rb), so dest ends up holding the cookbook root directly. Paths
-// that would escape dest are rejected.
+// that would escape dest are rejected, and every write goes through an os.Root
+// handle so the kernel refuses an escape the lexical check cannot see (a
+// symlink already sitting in dest, say).
 func extractCookbookTarball(r io.Reader, dest string) error {
+	root, err := os.OpenRoot(dest)
+	if err != nil {
+		return fmt.Errorf("supermarket: open destination %s: %w", dest, err)
+	}
+	defer func() { _ = root.Close() }()
+
 	gz, err := gzip.NewReader(r)
 	if err != nil {
 		return fmt.Errorf("supermarket: open gzip: %w", err)
@@ -80,20 +88,22 @@
 		if rel == "" {
 			continue
 		}
-		target := filepath.Join(dest, rel)
-		if !withinDir(dest, target) {
+		rel = filepath.FromSlash(rel)
+		if !withinDir(dest, filepath.Join(dest, rel)) {
 			return fmt.Errorf("supermarket: unsafe path in tarball: %q", hdr.Name)
 		}
 		switch hdr.Typeflag {
 		case tar.TypeDir:
-			if err := os.MkdirAll(target, extractDirMode); err != nil {
+			if err := root.MkdirAll(rel, extractDirMode); err != nil {
 				return err
 			}
 		case tar.TypeReg:
-			if err := os.MkdirAll(filepath.Dir(target), extractDirMode); err != nil {
-				return err
+			if dir := filepath.Dir(rel); dir != "." {
+				if err := root.MkdirAll(dir, extractDirMode); err != nil {
+					return err
+				}
 			}
-			f, err := os.OpenFile(target, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode)
+			f, err := root.OpenFile(rel, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode)
 			if err != nil {
 				return err
 			}

cli/policyfile/extract_test.go

@@ -0,0 +1,67 @@
+package policyfile
+
+import (
+	"archive/tar"
+	"bytes"
+	"compress/gzip"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// buildTarball gzips a tarball from the given entries (name -> body).
+func buildTarball(t *testing.T, files map[string]string) []byte {
+	t.Helper()
+	var buf bytes.Buffer
+	gz := gzip.NewWriter(&buf)
+	tw := tar.NewWriter(gz)
+	for name, body := range files {
+		hdr := &tar.Header{Name: name, Mode: 0o644, Typeflag: tar.TypeReg, Size: int64(len(body))}
+		if err := tw.WriteHeader(hdr); err != nil {
+			t.Fatal(err)
+		}
+		if _, err := tw.Write([]byte(body)); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if err := tw.Close(); err != nil {
+		t.Fatal(err)
+	}
+	if err := gz.Close(); err != nil {
+		t.Fatal(err)
+	}
+	return buf.Bytes()
+}
+
+// TestExtractCookbookTarballRejectsTraversal pins the plain "../" case.
+func TestExtractCookbookTarballRejectsTraversal(t *testing.T) {
+	dest := t.TempDir()
+	archive := buildTarball(t, map[string]string{"nginx/../../escape.txt": "pwned"})
+	if err := extractCookbookTarball(bytes.NewReader(archive), dest); err == nil {
+		t.Error("expected a traversal entry to be refused")
+	}
+	if _, err := os.Stat(filepath.Join(filepath.Dir(dest), "escape.txt")); err == nil {
+		t.Fatal("traversal entry escaped dest")
+	}
+}
+
+// TestExtractCookbookTarballDoesNotFollowSymlinkOutOfDest covers what a
+// lexical check cannot see. After the leading cookbook segment is stripped,
+// "nginx/cache/evil.rb" resolves to dest/cache/evil.rb, which is inside dest
+// by string comparison; if dest already holds a "cache" symlink the write
+// still lands outside it.
+func TestExtractCookbookTarballDoesNotFollowSymlinkOutOfDest(t *testing.T) {
+	dest := t.TempDir()
+	outside := t.TempDir()
+	if err := os.Symlink(outside, filepath.Join(dest, "cache")); err != nil {
+		t.Skipf("symlinks unavailable: %v", err)
+	}
+
+	archive := buildTarball(t, map[string]string{"nginx/cache/evil.rb": "pwned"})
+	if err := extractCookbookTarball(bytes.NewReader(archive), dest); err == nil {
+		t.Error("extractCookbookTarball wrote through a symlink in dest without complaint")
+	}
+	if _, err := os.Stat(filepath.Join(outside, "evil.rb")); err == nil {
+		t.Fatal("archive entry escaped dest through a pre-existing symlink")
+	}
+}

cli/policyfile/rubyeval/loader.go

@@ -204,8 +204,16 @@ func verifyFileSHA256(path, wantHex string) error {
 }
 
 // extractTarGz unpacks a .tar.gz archive under dest. It guards against path
-// traversal (a "../" entry escaping dest is rejected).
+// traversal (a "../" entry escaping dest is rejected) and writes through an
+// os.Root handle, so the kernel refuses an escape the lexical check cannot
+// see (a symlink already sitting in dest, say).
 func extractTarGz(archive []byte, dest string) error {
+	root, err := os.OpenRoot(dest)
+	if err != nil {
+		return fmt.Errorf("policyfile: open extraction dir %s: %w", dest, err)
+	}
+	defer func() { _ = root.Close() }()
+
 	gz, err := gzip.NewReader(bytes.NewReader(archive))
 	if err != nil {
 		return err
@@ -224,16 +232,22 @@ func extractTarGz(archive []byte, dest string) error {
 		if !withinDir(dest, target) {
 			return fmt.Errorf("policyfile: archive entry %q escapes extraction dir", hdr.Name)
 		}
+		rel, err := filepath.Rel(dest, target)
+		if err != nil {
+			return fmt.Errorf("policyfile: archive entry %q escapes extraction dir", hdr.Name)
+		}
 		switch hdr.Typeflag {
 		case tar.TypeDir:
-			if err := os.MkdirAll(target, 0o755); err != nil {
+			if err := root.MkdirAll(rel, 0o755); err != nil {
 				return err
 			}
 		case tar.TypeReg:
-			if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil {
-				return err
+			if dir := filepath.Dir(rel); dir != "." {
+				if err := root.MkdirAll(dir, 0o755); err != nil {
+					return err
+				}
 			}
-			f, err := os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.FileMode(hdr.Mode)&0o777)
+			f, err := root.OpenFile(rel, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.FileMode(hdr.Mode)&0o777)
 			if err != nil {
 				return err
 			}

cli/policyfile/rubyeval/loader_test.go

@@ -169,3 +169,22 @@ func makeTarGz(t *testing.T, files map[string]string) []byte {
 	}
 	return buf.Bytes()
 }
+
+// TestExtractTarGzDoesNotFollowSymlinkOutOfDest covers the case the lexical
+// check misses: "usr/evil" stays inside dest by string comparison, so a "usr"
+// symlink already in dest redirects the write outside it.
+func TestExtractTarGzDoesNotFollowSymlinkOutOfDest(t *testing.T) {
+	dir := t.TempDir()
+	outside := t.TempDir()
+	if err := os.Symlink(outside, filepath.Join(dir, "usr")); err != nil {
+		t.Skipf("symlinks unavailable: %v", err)
+	}
+
+	archive := makeTarGz(t, map[string]string{"usr/evil": "pwned"})
+	if err := extractTarGz(archive, dir); err == nil {
+		t.Error("extractTarGz wrote through a symlink in dest without complaint")
+	}
+	if _, err := os.Stat(filepath.Join(outside, "evil")); err == nil {
+		t.Fatal("archive entry escaped dest through a pre-existing symlink")
+	}
+}