fix(R-1187 b): restored links point only downward — no absolute target, no '..', no link on the way
gates / gates (push) Successful in 54s
gates / gates (push) Successful in 54s
Third automated security review finding, red-proved.
This commit is contained in:
@@ -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
|
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
|
(`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.
|
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
|
- **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,
|
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
|
R-1220) and reports it cleared if it was reported; a restore hold is always reported (R-379). 9202's four orphan holds
|
||||||
|
|||||||
@@ -248,3 +248,29 @@ func TestR1187b_NoLinkThroughALinkedParent(t *testing.T) {
|
|||||||
t.Fatalf("the refused link must be listed, got %v", refused)
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -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.
|
// 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 {
|
func parentThroughLink(rt *os.Root, name string) bool {
|
||||||
dir := filepath.Dir(name)
|
dir := filepath.Dir(name)
|
||||||
@@ -1285,23 +1313,16 @@ func extractTar(tarPath, destDir string) ([]string, error) {
|
|||||||
out.Close()
|
out.Close()
|
||||||
rt.Chmod(name, os.FileMode(hdr.Mode).Perm())
|
rt.Chmod(name, os.FileMode(hdr.Mode).Perm())
|
||||||
case tar.TypeSymlink:
|
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
|
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 dir := filepath.Dir(name); dir != "." {
|
||||||
if err := rt.MkdirAll(dir, 0755); err != nil {
|
if err := rt.MkdirAll(dir, 0755); err != nil {
|
||||||
return refused, err
|
return refused, err
|
||||||
|
|||||||
Reference in New Issue
Block a user