fix(R-1187 b): the import writes only through os.Root — never through a link that leaves the app's folder

Found by the automated security review of dfd0f8e. Red-proved. Commit only.

CHANGELOG.md

@@ -34,6 +34,9 @@ hub answers 400 to both and nothing else changes.
   runs `psql -v ON_ERROR_STOP=1`. Tests `internal/appexport/r1187_abdfg_test.go` (6), red-proved with the old behaviour
   behind the new function shapes (link packing error, a 10-s hang on the FIFO, link not restored, import not refused,
   bad paths accepted, postgis missed).
+  **Follow-up (the automated security review, same night):** the import writes only through `os.Root`, so no entry is
+  written through a link already in the folder that points outside; such an entry is refused and listed. Test
+  `TestR1187b_NoWriteThroughALinkThatLeavesTheFolder`, red-proved (the file landed outside with plain `os.Create`).
 - **R-1298:** the hold report skips an install's hold (`unhealthy_stop`, `update_failed`, `putback_pending`) of an app that
   is not installed (`holdreport.Reporter.SetInstalled`: the app's `app.yaml` exists — an unreadable one still counts,
   R-1220) and reports it cleared if it was reported; a restore hold is always reported (R-379). 9202's four orphan holds

controller/internal/appexport/r1187_abdfg_test.go

@@ -109,7 +109,7 @@ func TestR1187b_TheImportKeepsOwnersModesAndInsideLinks(t *testing.T) {
 
 	var chowned []string
 	old := lchownFn
-	lchownFn = func(p string, uid, gid int) error {
+	lchownFn = func(_ *os.Root, p string, uid, gid int) error {
 		chowned = append(chowned, filepath.Base(p)+":"+strconv.Itoa(uid)+":"+strconv.Itoa(gid))
 		return nil
 	}
@@ -190,3 +190,29 @@ func TestR1187g_PostgisIsPostgresAndPsqlStopsOnError(t *testing.T) {
 		t.Fatalf("psql does not stop on error: %s", args)
 	}
 }
+
+// (b), the security review's case: a link already in the destination folder (left by the old app, or made by an earlier
+// entry) that points OUTSIDE must never be written through — a tar entry „evil/passwd" would otherwise create a file
+// wherever „evil" points. Every write is confined to the app's folder (os.Root). RED-PROOF: with plain os.Create the
+// outside file is created.
+func TestR1187b_NoWriteThroughALinkThatLeavesTheFolder(t *testing.T) {
+	outside := t.TempDir()
+	dest := t.TempDir()
+	os.Symlink(outside, filepath.Join(dest, "evil")) // pre-existing, pointing out
+	tarPath := filepath.Join(t.TempDir(), "d.tar")
+	f, _ := os.Create(tarPath)
+	tw := tar.NewWriter(f)
+	tw.WriteHeader(&tar.Header{Name: "evil/passwd", Typeflag: tar.TypeReg, Mode: 0o644, Size: 3})
+	tw.Write([]byte("pwn"))
+	tw.WriteHeader(&tar.Header{Name: "ok.txt", Typeflag: tar.TypeReg, Mode: 0o644, Size: 2})
+	tw.Write([]byte("ok"))
+	tw.Close()
+	f.Close()
+	old := lchownFn
+	lchownFn = func(*os.Root, string, int, int) error { return nil }
+	defer func() { lchownFn = old }()
+	extractTar(tarPath, dest)
+	if _, err := os.Stat(filepath.Join(outside, "passwd")); err == nil {
+		t.Fatal("a file was written OUTSIDE the app's folder through a pre-existing link")
+	}
+}

controller/internal/appexport/restore.go

@@ -1204,7 +1204,7 @@ func extractTarGz(tgzPath, destDir string) error {
 }
 
 // lchownFn is the owner seam (a test records it; production is os.Lchown).
-var lchownFn = os.Lchown
+var lchownFn = func(r *os.Root, name string, uid, gid int) error { return r.Lchown(name, uid, gid) }
 
 // extractTar extracts a plain tar archive (one drive folder) into destDir and returns the entries it REFUSED.
 //
@@ -1219,6 +1219,14 @@ func extractTar(tarPath, destDir string) ([]string, error) {
 	}
 	defer f.Close()
 	root := filepath.Clean(destDir)
+	// Every write goes through os.Root: no entry can create or write a file outside destDir, not even through a link
+	// already in the folder (left by the old app, or made by an earlier entry) that points out — the security review's
+	// case, pinned by TestR1187b_NoWriteThroughALinkThatLeavesTheFolder.
+	rt, err := os.OpenRoot(root)
+	if err != nil {
+		return nil, err
+	}
+	defer rt.Close()
 	var refused []string
 	tr := tar.NewReader(f)
 	for {
@@ -1229,53 +1237,59 @@ func extractTar(tarPath, destDir string) ([]string, error) {
 		if err != nil {
 			return refused, err
 		}
-		target := filepath.Join(root, hdr.Name)
+		name := filepath.Clean(hdr.Name)
-		if !strings.HasPrefix(filepath.Clean(target), root+string(os.PathSeparator)) {
+		if name == "." || filepath.IsAbs(name) || name == ".." || strings.HasPrefix(name, ".."+string(os.PathSeparator)) {
 			continue // path traversal
 		}
 		switch hdr.Typeflag {
 		case tar.TypeDir:
-			if err := os.MkdirAll(target, os.FileMode(hdr.Mode).Perm()); err != nil {
+			if err := rt.MkdirAll(name, os.FileMode(hdr.Mode).Perm()); err != nil {
 				return refused, err
 			}
-			os.Chmod(target, os.FileMode(hdr.Mode).Perm())
+			rt.Chmod(name, os.FileMode(hdr.Mode).Perm())
 		case tar.TypeReg:
-			if err := os.MkdirAll(filepath.Dir(target), 0755); err != nil {
+			if dir := filepath.Dir(name); dir != "." {
-				return refused, err
+				if err := rt.MkdirAll(dir, 0755); err != nil {
+					return refused, err
+				}
 			}
-			outFile, err := os.Create(target)
+			out, err := rt.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.FileMode(hdr.Mode).Perm())
 			if err != nil {
+				refused = append(refused, hdr.Name) // e.g. its path goes through a link that leaves the folder
+				continue
+			}
+			if _, err := io.Copy(out, tr); err != nil {
+				out.Close()
 				return refused, err
 			}
-			if _, err := io.Copy(outFile, tr); err != nil {
+			out.Close()
-				outFile.Close()
+			rt.Chmod(name, os.FileMode(hdr.Mode).Perm())
-				return refused, err
-			}
-			outFile.Close()
-			os.Chmod(target, os.FileMode(hdr.Mode).Perm())
 		case tar.TypeSymlink:
 			dst := hdr.Linkname
 			if filepath.IsAbs(dst) {
 				refused = append(refused, hdr.Name)
 				continue
 			}
-			resolved := filepath.Clean(filepath.Join(filepath.Dir(target), dst))
+			resolved := filepath.Clean(filepath.Join(filepath.Dir(filepath.Join(root, name)), dst))
 			if resolved != root && !strings.HasPrefix(resolved, root+string(os.PathSeparator)) {
 				refused = append(refused, hdr.Name)
 				continue
 			}
-			if err := os.MkdirAll(filepath.Dir(target), 0755); err != nil {
+			if dir := filepath.Dir(name); dir != "." {
-				return refused, err
+				if err := rt.MkdirAll(dir, 0755); err != nil {
+					return refused, err
+				}
 			}
-			os.Remove(target)
+			rt.Remove(name)
-			if err := os.Symlink(dst, target); err != nil {
+			if err := rt.Symlink(dst, name); err != nil {
-				return refused, err
+				refused = append(refused, hdr.Name)
+				continue
 			}
 		default:
 			refused = append(refused, hdr.Name)
 			continue
 		}
-		if err := lchownFn(target, hdr.Uid, hdr.Gid); err != nil && !os.IsPermission(err) {
+		if err := lchownFn(rt, name, hdr.Uid, hdr.Gid); err != nil && !os.IsPermission(err) {
 			return refused, fmt.Errorf("setting the owner of %s: %w", hdr.Name, err)
 		}
 	}