From 106325de4306d15c068e91ddbf41ebc20b92583f Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 26 Oct 2026 20:28:04 +0100 Subject: [PATCH] =?UTF-8?q?fix(R-1187=20b):=20the=20import=20writes=20only?= =?UTF-8?q?=20through=20os.Root=20=E2=80=94=20never=20through=20a=20link?= =?UTF-8?q?=20that=20leaves=20the=20app's=20folder?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the automated security review of dfd0f8e. Red-proved. Commit only. --- CHANGELOG.md | 3 + .../internal/appexport/r1187_abdfg_test.go | 28 +++++++++- controller/internal/appexport/restore.go | 56 ++++++++++++------- 3 files changed, 65 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c66a0bc..9f5664bb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/controller/internal/appexport/r1187_abdfg_test.go b/controller/internal/appexport/r1187_abdfg_test.go index 07ba147b..42377904 100644 --- a/controller/internal/appexport/r1187_abdfg_test.go +++ b/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") + } +} diff --git a/controller/internal/appexport/restore.go b/controller/internal/appexport/restore.go index 53a88b82..8e2d8c10 100644 --- a/controller/internal/appexport/restore.go +++ b/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) - if !strings.HasPrefix(filepath.Clean(target), root+string(os.PathSeparator)) { + name := filepath.Clean(hdr.Name) + 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 { - return refused, err + if dir := filepath.Dir(name); dir != "." { + 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 { - outFile.Close() - return refused, err - } - outFile.Close() - os.Chmod(target, os.FileMode(hdr.Mode).Perm()) + out.Close() + rt.Chmod(name, 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 { - return refused, err + if dir := filepath.Dir(name); dir != "." { + if err := rt.MkdirAll(dir, 0755); err != nil { + return refused, err + } } - os.Remove(target) - if err := os.Symlink(dst, target); err != nil { - return refused, err + rt.Remove(name) + if err := rt.Symlink(dst, name); err != nil { + 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) } }