Harden bundle promotion against symlink replacement

2026-08-10 02:45:51 +00:00
parent 701b6726d7
commit b99bd38eb4
3 changed files with 535 additions and 284 deletions
--- a/internal/fileops/directory.go
+++ b/internal/fileops/directory.go
@@ -30,6 +30,19 @@ func PromoteDirectory(src, dst string) error {
 }

 func promoteDirectory(src, dst string, install func(string, string) error) error {
+	return promoteDirectoryWithHooks(src, dst, install, sourceTraversalHooks{})
+}
+
+type sourceTraversalHooks struct {
+	afterRootInspect  func()
+	afterEntryInspect func(string)
+}
+
+func promoteDirectoryWithHooks(
+	src, dst string,
+	install func(string, string) error,
+	hooks sourceTraversalHooks,
+) error {
 	if strings.TrimSpace(src) == "" || strings.TrimSpace(dst) == "" {
 		return fmt.Errorf("source and destination directory paths are required")
 	}
@@ -76,7 +89,13 @@ func promoteDirectory(src, dst string, install func(string, string) error) error
 		}
 	}()

-	if err := copyRegularTree(src, temporary); err != nil {
+	sourceRoot, err := openVerifiedSourceRoot(src, sourceInfo, hooks)
+	if err != nil {
+		return err
+	}
+	defer func() { _ = sourceRoot.Close() }()
+
+	if err := copyRegularTree(sourceRoot, src, temporary, hooks); err != nil {
 		return err
 	}
 	if err := os.Chmod(temporary, promotedDirectoryMode); err != nil {
@@ -96,62 +115,153 @@ func promoteDirectory(src, dst string, install func(string, string) error) error
 	return nil
 }

-func copyRegularTree(src, dst string) error {
-	entries, err := os.ReadDir(src)
+func openVerifiedSourceRoot(path string, inspected os.FileInfo, hooks sourceTraversalHooks) (*os.Root, error) {
+	if hooks.afterRootInspect != nil {
+		hooks.afterRootInspect()
+	}
+
+	root, err := os.OpenRoot(path)
 	if err != nil {
-		return fmt.Errorf("read source directory %q: %w", src, err)
+		return nil, fmt.Errorf("open source directory %q: %w", path, err)
+	}
+	verified := false
+	defer func() {
+		if !verified {
+			_ = root.Close()
+		}
+	}()
+
+	opened, err := root.Stat(".")
+	if err != nil {
+		return nil, fmt.Errorf("inspect opened source directory %q: %w", path, err)
+	}
+	if !opened.IsDir() || !os.SameFile(inspected, opened) {
+		return nil, fmt.Errorf("source directory %q changed while being opened", path)
+	}
+	current, err := os.Lstat(path)
+	if err != nil {
+		return nil, fmt.Errorf("reinspect source directory %q: %w", path, err)
+	}
+	if current.Mode()&os.ModeSymlink != 0 || !current.IsDir() || !os.SameFile(opened, current) {
+		return nil, fmt.Errorf("source directory %q changed while being opened", path)
+	}
+
+	verified = true
+	return root, nil
+}
+
+func copyRegularTree(src *os.Root, sourcePath, dst string, hooks sourceTraversalHooks) error {
+	directory, err := src.Open(".")
+	if err != nil {
+		return fmt.Errorf("open source directory %q for traversal: %w", sourcePath, err)
+	}
+	defer func() { _ = directory.Close() }()
+
+	entries, err := directory.ReadDir(-1)
+	if err != nil {
+		return fmt.Errorf("read source directory %q: %w", sourcePath, err)
 	}
 	sort.Slice(entries, func(i, j int) bool {
 		return entries[i].Name() < entries[j].Name()
 	})

 	for _, entry := range entries {
-		sourcePath := filepath.Join(src, entry.Name())
+		entryPath := filepath.Join(sourcePath, entry.Name())
 		destinationPath := filepath.Join(dst, entry.Name())
-		info, err := os.Lstat(sourcePath)
+		info, err := src.Lstat(entry.Name())
 		if err != nil {
-			return fmt.Errorf("inspect source entry %q: %w", sourcePath, err)
+			return fmt.Errorf("inspect source entry %q: %w", entryPath, err)
 		}

 		switch {
 		case info.Mode().IsRegular():
-			if err := copyRegularFile(sourcePath, destinationPath, info); err != nil {
+			if hooks.afterEntryInspect != nil {
+				hooks.afterEntryInspect(entryPath)
+			}
+			if err := copyRegularFile(src, entry.Name(), entryPath, destinationPath, info); err != nil {
 				return err
 			}
 		case info.IsDir():
-			if err := os.Mkdir(destinationPath, promotedDirectoryMode); err != nil {
-				return fmt.Errorf("create destination directory %q: %w", destinationPath, err)
+			if hooks.afterEntryInspect != nil {
+				hooks.afterEntryInspect(entryPath)
 			}
-			if err := copyRegularTree(sourcePath, destinationPath); err != nil {
+			if err := copyRegularDirectory(src, entry.Name(), entryPath, destinationPath, info, hooks); err != nil {
 				return err
 			}
-			if err := os.Chmod(destinationPath, promotedDirectoryMode); err != nil {
-				return fmt.Errorf("set destination directory permissions %q: %w", destinationPath, err)
-			}
-			if err := syncDirectory(destinationPath); err != nil {
-				return fmt.Errorf("sync destination directory %q: %w", destinationPath, err)
-			}
 		default:
-			return fmt.Errorf("source entry %q has unsupported file type %s", sourcePath, info.Mode().Type())
+			return fmt.Errorf("source entry %q has unsupported file type %s", entryPath, info.Mode().Type())
 		}
 	}

 	return nil
 }

-func copyRegularFile(src, dst string, inspected os.FileInfo) error {
-	in, err := os.Open(src)
+func copyRegularDirectory(
+	parent *os.Root,
+	name, sourcePath, dst string,
+	inspected os.FileInfo,
+	hooks sourceTraversalHooks,
+) error {
+	child, err := parent.OpenRoot(name)
 	if err != nil {
-		return fmt.Errorf("open source file %q: %w", src, err)
+		return fmt.Errorf("open source directory %q: %w", sourcePath, err)
+	}
+	defer func() { _ = child.Close() }()
+
+	opened, err := child.Stat(".")
+	if err != nil {
+		return fmt.Errorf("inspect opened source directory %q: %w", sourcePath, err)
+	}
+	if !opened.IsDir() || !os.SameFile(inspected, opened) {
+		return fmt.Errorf("source directory %q changed while being copied", sourcePath)
+	}
+	current, err := parent.Lstat(name)
+	if err != nil {
+		return fmt.Errorf("reinspect source directory %q: %w", sourcePath, err)
+	}
+	if current.Mode()&os.ModeSymlink != 0 || !current.IsDir() || !os.SameFile(opened, current) {
+		return fmt.Errorf("source directory %q changed while being copied", sourcePath)
+	}
+
+	if err := os.Mkdir(dst, promotedDirectoryMode); err != nil {
+		return fmt.Errorf("create destination directory %q: %w", dst, err)
+	}
+	if err := copyRegularTree(child, sourcePath, dst, hooks); err != nil {
+		return err
+	}
+	if err := os.Chmod(dst, promotedDirectoryMode); err != nil {
+		return fmt.Errorf("set destination directory permissions %q: %w", dst, err)
+	}
+	if err := syncDirectory(dst); err != nil {
+		return fmt.Errorf("sync destination directory %q: %w", dst, err)
+	}
+	return nil
+}
+
+func copyRegularFile(
+	root *os.Root,
+	name, sourcePath, dst string,
+	inspected os.FileInfo,
+) error {
+	in, err := root.Open(name)
+	if err != nil {
+		return fmt.Errorf("open source file %q: %w", sourcePath, err)
 	}
 	defer func() { _ = in.Close() }()

-	openedInfo, err := in.Stat()
+	opened, err := in.Stat()
 	if err != nil {
-		return fmt.Errorf("inspect opened source file %q: %w", src, err)
+		return fmt.Errorf("inspect opened source file %q: %w", sourcePath, err)
 	}
-	if !openedInfo.Mode().IsRegular() || !os.SameFile(inspected, openedInfo) {
-		return fmt.Errorf("source file %q changed while being copied", src)
+	if !opened.Mode().IsRegular() || !os.SameFile(inspected, opened) {
+		return fmt.Errorf("source file %q changed while being copied", sourcePath)
+	}
+	current, err := root.Lstat(name)
+	if err != nil {
+		return fmt.Errorf("reinspect source file %q: %w", sourcePath, err)
+	}
+	if current.Mode()&os.ModeSymlink != 0 || !current.Mode().IsRegular() || !os.SameFile(opened, current) {
+		return fmt.Errorf("source file %q changed while being copied", sourcePath)
 	}

 	out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_EXCL, promotedFileMode)
@@ -166,7 +276,7 @@ func copyRegularFile(src, dst string, inspected os.FileInfo) error {
 	}()

 	if _, err := io.Copy(out, in); err != nil {
-		return fmt.Errorf("copy source file %q: %w", src, err)
+		return fmt.Errorf("copy source file %q: %w", sourcePath, err)
 	}
 	if err := out.Chmod(promotedFileMode); err != nil {
 		return fmt.Errorf("set destination file permissions %q: %w", dst, err)
--- a/internal/fileops/directory_test.go
+++ b/internal/fileops/directory_test.go
@@ -159,6 +159,151 @@ func TestPromoteDirectoryRejectsSymlinksWithoutFollowingThem(t *testing.T) {
 	assertFileBytes(t, filepath.Join(externalDirectory, "secret.txt"), []byte("secret"))
 }

+func TestPromoteDirectoryRejectsSymlinkSourceRoot(t *testing.T) {
+	root := t.TempDir()
+	target := filepath.Join(root, "target")
+	src := filepath.Join(root, "source")
+	dst := filepath.Join(root, "destination")
+	mustWriteFile(t, filepath.Join(target, "value.txt"), []byte("outside"), 0o644)
+	if err := os.Symlink(target, src); err != nil {
+		t.Skipf("Symlink() unavailable: %v", err)
+	}
+
+	if err := PromoteDirectory(src, dst); err == nil {
+		t.Fatal("PromoteDirectory() error = nil, want source-root symlink rejection")
+	}
+	assertFileBytes(t, filepath.Join(target, "value.txt"), []byte("outside"))
+	assertPathMissing(t, dst)
+	assertNoMatchingTempDirectories(t, root, ".destination.tmp-")
+}
+
+func TestPromoteDirectoryRejectsSourceRootReplacementBeforeOpen(t *testing.T) {
+	root := t.TempDir()
+	src := filepath.Join(root, "source")
+	preserved := filepath.Join(root, "source-preserved")
+	replacement := filepath.Join(root, "replacement")
+	dst := filepath.Join(root, "destination")
+	mustWriteFile(t, filepath.Join(src, "value.txt"), []byte("original"), 0o644)
+	mustWriteFile(t, filepath.Join(replacement, "value.txt"), []byte("replacement"), 0o644)
+
+	err := promoteDirectoryWithHooks(src, dst, renameDirectoryNoReplace, sourceTraversalHooks{
+		afterRootInspect: func() {
+			if err := os.Rename(src, preserved); err != nil {
+				t.Fatalf("Rename(original source) error = %v", err)
+			}
+			if err := os.Rename(replacement, src); err != nil {
+				t.Fatalf("Rename(replacement source) error = %v", err)
+			}
+		},
+	})
+	if err == nil {
+		t.Fatal("promoteDirectoryWithHooks() error = nil, want source identity failure")
+	}
+	assertFileBytes(t, filepath.Join(preserved, "value.txt"), []byte("original"))
+	assertFileBytes(t, filepath.Join(src, "value.txt"), []byte("replacement"))
+	assertPathMissing(t, dst)
+	assertNoMatchingTempDirectories(t, root, ".destination.tmp-")
+}
+
+func TestPromoteDirectoryRejectsInspectedDirectorySymlinkReplacement(t *testing.T) {
+	root := t.TempDir()
+	src := filepath.Join(root, "source")
+	child := filepath.Join(src, "child")
+	preserved := filepath.Join(src, "child-preserved")
+	outside := filepath.Join(root, "outside")
+	dst := filepath.Join(root, "destination")
+	mustWriteFile(t, filepath.Join(child, "value.txt"), []byte("original"), 0o644)
+	mustWriteFile(t, filepath.Join(outside, "sentinel.txt"), []byte("outside"), 0o644)
+
+	replaced := false
+	err := promoteDirectoryWithHooks(src, dst, renameDirectoryNoReplace, sourceTraversalHooks{
+		afterEntryInspect: func(path string) {
+			if replaced || path != child {
+				return
+			}
+			replaced = true
+			if err := os.Rename(child, preserved); err != nil {
+				t.Fatalf("Rename(inspected child) error = %v", err)
+			}
+			if err := os.Symlink(filepath.Join("..", "outside"), child); err != nil {
+				t.Skipf("Symlink() unavailable: %v", err)
+			}
+			if err := os.Mkdir(dst, 0o755); err != nil {
+				t.Fatalf("Mkdir(concurrent destination) error = %v", err)
+			}
+			mustWriteFile(t, filepath.Join(dst, "value.txt"), []byte("concurrent"), 0o644)
+		},
+	})
+	if err == nil {
+		t.Fatal("promoteDirectoryWithHooks() error = nil, want symlink replacement failure")
+	}
+	assertFileBytes(t, filepath.Join(preserved, "value.txt"), []byte("original"))
+	assertFileBytes(t, filepath.Join(outside, "sentinel.txt"), []byte("outside"))
+	assertFileBytes(t, filepath.Join(dst, "value.txt"), []byte("concurrent"))
+	assertPathMissing(t, filepath.Join(dst, "sentinel.txt"))
+	assertNoMatchingTempDirectories(t, root, ".destination.tmp-")
+}
+
+func TestPromoteDirectoryRejectsInspectedFileIdentityMismatch(t *testing.T) {
+	root := t.TempDir()
+	src := filepath.Join(root, "source")
+	file := filepath.Join(src, "value.txt")
+	preserved := filepath.Join(src, "value-preserved.txt")
+	dst := filepath.Join(root, "destination")
+	mustWriteFile(t, file, []byte("original"), 0o644)
+
+	replaced := false
+	err := promoteDirectoryWithHooks(src, dst, renameDirectoryNoReplace, sourceTraversalHooks{
+		afterEntryInspect: func(path string) {
+			if replaced || path != file {
+				return
+			}
+			replaced = true
+			if err := os.Rename(file, preserved); err != nil {
+				t.Fatalf("Rename(inspected file) error = %v", err)
+			}
+			mustWriteFile(t, file, []byte("replacement"), 0o644)
+		},
+	})
+	if err == nil {
+		t.Fatal("promoteDirectoryWithHooks() error = nil, want file identity failure")
+	}
+	assertFileBytes(t, preserved, []byte("original"))
+	assertFileBytes(t, file, []byte("replacement"))
+	assertPathMissing(t, dst)
+	assertNoMatchingTempDirectories(t, root, ".destination.tmp-")
+}
+
+func TestPromoteDirectoryRejectsInspectedDirectoryIdentityMismatch(t *testing.T) {
+	root := t.TempDir()
+	src := filepath.Join(root, "source")
+	child := filepath.Join(src, "child")
+	preserved := filepath.Join(src, "child-preserved")
+	dst := filepath.Join(root, "destination")
+	mustWriteFile(t, filepath.Join(child, "value.txt"), []byte("original"), 0o644)
+
+	replaced := false
+	err := promoteDirectoryWithHooks(src, dst, renameDirectoryNoReplace, sourceTraversalHooks{
+		afterEntryInspect: func(path string) {
+			if replaced || path != child {
+				return
+			}
+			replaced = true
+			if err := os.Rename(child, preserved); err != nil {
+				t.Fatalf("Rename(inspected directory) error = %v", err)
+			}
+			mustWriteFile(t, filepath.Join(child, "value.txt"), []byte("replacement"), 0o644)
+		},
+	})
+	if err == nil {
+		t.Fatal("promoteDirectoryWithHooks() error = nil, want directory identity failure")
+	}
+	assertFileBytes(t, filepath.Join(preserved, "value.txt"), []byte("original"))
+	assertFileBytes(t, filepath.Join(child, "value.txt"), []byte("replacement"))
+	assertPathMissing(t, dst)
+	assertNoMatchingTempDirectories(t, root, ".destination.tmp-")
+}
+
 func TestPromoteDirectoryDoesNotReplaceDestinationCreatedBeforeInstall(t *testing.T) {
 	root := t.TempDir()
 	src := filepath.Join(root, "source")
@@ -217,6 +362,13 @@ func assertFileBytes(t *testing.T, path string, want []byte) {
 	}
 }

+func assertPathMissing(t *testing.T, path string) {
+	t.Helper()
+	if _, err := os.Lstat(path); !os.IsNotExist(err) {
+		t.Fatalf("Lstat(%q) error = %v, want not exist", path, err)
+	}
+}
+
 func treeLayout(t *testing.T, root string) []string {
 	t.Helper()
 	var layout []string