diff --git a/CHANGELOG.md b/CHANGELOG.md index f716c3fc..2122e91f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,9 @@ hub answers 400 to both and nothing else changes. passed the path-text check while really pointing above the folder — now refused and listed (`TestR1187b_NoLinkThroughALinkedParent`, red-proved). psql takes `--set=ON_ERROR_STOP=1` (the `-v` short form tripped the docker `-v` gate); the four new message keys are listed in `scripts/i18n_go_keys.json` as born keys. + A third review finding: a link TARGET through a link already in the folder („x" → „evil/y", evil pointing out). The rule + is now strict and simple: a restored link points only DOWNWARD — relative, no „..", no link on its path or in its + target; everything else is refused and listed (`safeDownwardLink`; `TestR1187b_NoLinkTargetThroughALink`, red-proved). - **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 ea2ca2eb..06464644 100644 --- a/controller/internal/appexport/r1187_abdfg_test.go +++ b/controller/internal/appexport/r1187_abdfg_test.go @@ -248,3 +248,29 @@ func TestR1187b_NoLinkThroughALinkedParent(t *testing.T) { t.Fatalf("the refused link must be listed, got %v", refused) } } + +// (b), the security review's third case: a link TARGET that passes through a link already in the folder („evil" → an +// outside folder, then „x" → „evil/y") reads as inside by its text and really points outside. Restored links may only +// point DOWNWARD to real folders: no absolute target, no „..", no link on the way. RED-PROOF: the text check created „x". +func TestR1187b_NoLinkTargetThroughALink(t *testing.T) { + outside := t.TempDir() + dest := t.TempDir() + os.Symlink(outside, filepath.Join(dest, "evil")) + tarPath := filepath.Join(t.TempDir(), "d.tar") + f, _ := os.Create(tarPath) + tw := tar.NewWriter(f) + tw.WriteHeader(&tar.Header{Name: "x", Typeflag: tar.TypeSymlink, Linkname: "evil/y"}) + tw.WriteHeader(&tar.Header{Name: "up", Typeflag: tar.TypeSymlink, Linkname: "sub/../sub"}) + tw.Close() + f.Close() + old := lchownFn + lchownFn = func(*os.Root, string, int, int) error { return nil } + defer func() { lchownFn = old }() + refused, _ := extractTar(tarPath, dest) + if _, err := os.Lstat(filepath.Join(dest, "x")); err == nil { + t.Fatal("a link whose target passes through an outside-pointing link was created") + } + if strings.Join(refused, ",") != "x,up" { + t.Fatalf("both links must be refused and listed, got %v", refused) + } +} diff --git a/controller/internal/appexport/restore.go b/controller/internal/appexport/restore.go index 9b16c3bf..fa44be3c 100644 --- a/controller/internal/appexport/restore.go +++ b/controller/internal/appexport/restore.go @@ -1203,6 +1203,34 @@ func extractTarGz(tgzPath, destDir string) error { } } +// safeDownwardLink is the rule for a restored link: a relative target with no „.." component, and no symbolic link on +// the way — not among the folders of the link's own path, not among the existing components of its target. +func safeDownwardLink(rt *os.Root, name, target string) bool { + if target == "" || filepath.IsAbs(target) { + return false + } + for _, part := range strings.Split(filepath.ToSlash(target), "/") { + if part == ".." { + return false + } + } + if parentThroughLink(rt, name) { + return false + } + acc := filepath.Dir(name) + for _, part := range strings.Split(filepath.Clean(target), string(os.PathSeparator)) { + acc = filepath.Join(acc, part) + fi, err := rt.Lstat(acc) + if err != nil { + break // not there (yet): nothing further down can be a link that exists now + } + if fi.Mode()&os.ModeSymlink != 0 { + return false + } + } + return true +} + // parentThroughLink is true when any folder on name's path (inside the root) is a symbolic link. func parentThroughLink(rt *os.Root, name string) bool { dir := filepath.Dir(name) @@ -1285,23 +1313,16 @@ func extractTar(tarPath, destDir string) ([]string, error) { out.Close() rt.Chmod(name, os.FileMode(hdr.Mode).Perm()) case tar.TypeSymlink: + // A restored link may only point DOWNWARD inside the folder, through real folders: no absolute target, no + // „..", and no link on the way — neither on the link's own path nor in its target. Such a link can never + // resolve outside the app's folder, whatever else the folder holds. Everything else is refused and listed. + // (Three rounds of the automated security review: a link written through, a linked parent, a target through + // a link. Pinned by TestR1187b_*.) + if !safeDownwardLink(rt, name, hdr.Linkname) { + refused = append(refused, hdr.Name) + continue + } dst := hdr.Linkname - if filepath.IsAbs(dst) { - refused = append(refused, hdr.Name) - continue - } - // The text check below is exact only when no folder on the link's path is itself a link (a „a" → „." makes - // „a/b/l" really „b/l"): such an entry is refused (the security review's second case, pinned by - // TestR1187b_NoLinkThroughALinkedParent). - if parentThroughLink(rt, name) { - refused = append(refused, hdr.Name) - continue - } - 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 dir := filepath.Dir(name); dir != "." { if err := rt.MkdirAll(dir, 0755); err != nil { return refused, err