fix(security): confine feature archive extraction

Author: aledbf

internal/cli/extract_targz_test.go

@@ -86,3 +86,25 @@ func TestExtractTarGz_ZipSlip(t *testing.T) {
 		t.Error("zip-slip guard failed: a file was written outside the destination")
 	}
 }
+
+// TestExtractTarGz_SymlinkEscape proves that lexical path checks are not the
+// only boundary: an existing link below the extraction directory must not let
+// a regular archive entry write outside it.
+func TestExtractTarGz_SymlinkEscape(t *testing.T) {
+	arc := writeTarGz(t, [][2]string{
+		{"link/escaped.txt", "pwned"},
+	})
+	dest := t.TempDir()
+	outside := t.TempDir()
+	if err := os.Symlink(outside, filepath.Join(dest, "link")); err != nil {
+		t.Skipf("symlinks are unavailable: %v", err)
+	}
+
+	err := extractTarGz(arc, dest)
+	if err == nil {
+		t.Fatal("expected a symlink escape rejection, got nil")
+	}
+	if _, statErr := os.Stat(filepath.Join(outside, "escaped.txt")); !os.IsNotExist(statErr) {
+		t.Fatalf("symlink escape wrote outside the destination: %v", statErr)
+	}
+}

internal/cli/feature_install.go

@@ -12,6 +12,7 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"path"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -764,6 +765,12 @@ func extractTarGz(archivePath, destDir string) error {
 	}
 
 	tr := tar.NewReader(reader)
+	root, err := os.OpenRoot(destDir)
+	if err != nil {
+		return fmt.Errorf("open extraction root: %w", err)
+	}
+	defer root.Close()
+
 	for {
 		header, err := tr.Next()
 		if err == io.EOF {
@@ -773,41 +780,41 @@ func extractTarGz(archivePath, destDir string) error {
 			return fmt.Errorf("tar read: %w", err)
 		}
 
-		cleanName := filepath.Clean(header.Name)
-		target := filepath.Join(destDir, cleanName)
-
-		// Zip-slip guard: reject entries whose path escapes destDir (e.g.
-		// "../../etc/x"), so a malicious Feature tarball cannot write outside the
-		// extraction directory. filepath.Join cleans "..", so compare the result.
-		if target != destDir && !strings.HasPrefix(target, destDir+string(os.PathSeparator)) {
+		// Tar paths always use forward slashes. Localize rejects absolute paths,
+		// parent traversal, and names that cannot be represented safely on the
+		// current platform. Root also prevents escapes through symlinks already
+		// present below destDir.
+		cleanName := path.Clean(header.Name)
+		localName, err := filepath.Localize(cleanName)
+		if err != nil || !filepath.IsLocal(localName) {
 			return fmt.Errorf("tar entry %q escapes the destination directory", header.Name)
 		}
 
 		switch header.Typeflag {
 		case tar.TypeDir:
-			if err := os.MkdirAll(target, 0755); err != nil {
-				return err
+			if err := root.MkdirAll(localName, 0755); err != nil {
+				return fmt.Errorf("create directory for tar entry %q: %w", header.Name, err)
 			}
 		case tar.TypeReg:
-			if err := os.MkdirAll(filepath.Dir(target), 0755); err != nil {
-				return err
+			if err := root.MkdirAll(filepath.Dir(localName), 0755); err != nil {
+				return fmt.Errorf("create parent for tar entry %q: %w", header.Name, err)
 			}
-			out, err := os.Create(target)
+			out, err := root.Create(localName)
 			if err != nil {
-				return err
+				return fmt.Errorf("create tar entry %q: %w", header.Name, err)
 			}
 			// A truncated copy must fail the install, not silently produce a
 			// corrupt Feature that we report as success.
 			if _, err := io.Copy(out, tr); err != nil {
 				out.Close()
-				return fmt.Errorf("extract %s: %w", target, err)
+				return fmt.Errorf("extract tar entry %q: %w", header.Name, err)
 			}
 			if err := out.Close(); err != nil {
-				return fmt.Errorf("close %s: %w", target, err)
+				return fmt.Errorf("close tar entry %q: %w", header.Name, err)
 			}
 			if header.Mode != 0 {
-				if err := os.Chmod(target, os.FileMode(header.Mode)); err != nil {
-					return err
+				if err := root.Chmod(localName, os.FileMode(header.Mode)); err != nil {
+					return fmt.Errorf("set mode on tar entry %q: %w", header.Name, err)
 				}
 			}
 		}