amadan

carlos / platform Public

moves/archive: Verify and Extract share one set of stream rules (Task 7 fix round 2)

The directory-before-contents rule lived only in Extract, so a sealed
archive with files/a/b before files/a/ passed Verify and failed at
landing. streamRules in limits.go now holds every per-entry stream rule
(entry limit, name and type, duplicates, directory order, content cap)
and both readers admit each header through it. The manifest lists
regular files only and keeps the shared name check.

TestVerifyRefusesEveryStreamExtractRefuses runs every stream the Extract
refusal tests use through Verify with a correct manifest and requires the
same refusal, plus the missing-parent and late-parent cases.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 924d3132c927321dd0151091a10e67de7aeeeb27 parent e068c53
4 files changed, +136 −53
  • internal/moves/archive/archive_test.go +76 −12
  • internal/moves/archive/extract.go +4 −24
  • internal/moves/archive/limits.go +51 −0
  • internal/moves/archive/verify.go +5 −17
diff --git a/internal/moves/archive/archive_test.go b/internal/moves/archive/archive_test.go
index fb70123cd..4cf281c17 100644
--- a/internal/moves/archive/archive_test.go
+++ b/internal/moves/archive/archive_test.go
@@ -213,19 +213,23 @@ func assertUntouched(t *testing.T, parent, files, env string) {
}
}
+// duplicateStreams name one destination twice.
+func duplicateStreams() map[string][]rawEntry {
+ return map[string][]rawEntry{
+ "env twice": {reg("env", "x"), reg("env", "y")},
+ "file twice": {reg("files/a", "x"), reg("files/a", "y")},
+ "dir then file": {dirEntry("files/a/"), reg("files/a", "y")},
+ "file then env dup": {reg("files/a", "x"), reg("env", "e"), reg("files/a", "z")},
+ }
+}
+
// A crafted archive naming one destination twice (two "env" entries, a
// file twice, or a directory and then a file of one name) is refused
// before any byte reaches the destination: Extract writes into a private
// staging directory and moves it into place only once the whole archive
// has been read and checked.
func TestExtractRefusesDuplicateDestinations(t *testing.T) {
- cases := map[string][]rawEntry{
- "env twice": {reg("env", "x"), reg("env", "y")},
- "file twice": {reg("files/a", "x"), reg("files/a", "y")},
- "dir then file": {dirEntry("files/a/"), reg("files/a", "y")},
- "file then env dup": {reg("files/a", "x"), reg("env", "e"), reg("files/a", "z")},
- }
- for name, entries := range cases {
+ for name, entries := range duplicateStreams() {
t.Run(name, func(t *testing.T) {
parent, files, env := extractTarget(t)
err := Extract(bytes.NewReader(tarZst(t, entries, nil)), files, env)
@@ -239,11 +243,15 @@ func TestExtractRefusesDuplicateDestinations(t *testing.T) {
// Traversal, the protocol marker, entries outside files/ and env, and any
// type but a regular file or a directory are refused, and nothing lands.
-func TestExtractRefusesTraversalAndSpecialEntries(t *testing.T) {
- cases := map[string]struct {
- entries []rawEntry
- want error
- }{
+// refusedStreamCase is one hostile stream and the refusal every reader owes it.
+type refusedStreamCase struct {
+ entries []rawEntry
+ want error
+}
+
+// refusedStreams are the hostile streams Extract and Verify both refuse.
+func refusedStreams() map[string]refusedStreamCase {
+ return map[string]refusedStreamCase{
"dotdot": {[]rawEntry{reg("files/../x", "x")}, ErrBadPath},
"leading dotdot": {[]rawEntry{reg("../x", "x")}, ErrBadPath},
"absolute": {[]rawEntry{reg("/etc/x", "x")}, ErrBadPath},
@@ -255,6 +263,8 @@ func TestExtractRefusesTraversalAndSpecialEntries(t *testing.T) {
"marker dir": {[]rawEntry{dirEntry("files/.files-gen/")}, ErrBadPath},
"marker child": {[]rawEntry{reg("files/.files-gen/child", "x")}, ErrBadPath},
"before its dir": {[]rawEntry{reg("files/a/b", "x")}, ErrBadPath},
+ "late dir": {[]rawEntry{reg("files/a/b", "x"), dirEntry("files/a/")}, ErrBadPath},
+ "nested late dir": {[]rawEntry{dirEntry("files/a/"), reg("files/a/b/c", "x"), dirEntry("files/a/b/")}, ErrBadPath},
"env as dir": {[]rawEntry{dirEntry("env/")}, ErrBadPath},
"too long": {[]rawEntry{reg("files/"+strings.Repeat("a", MaxPathLen+1), "x")}, ErrBadPath},
"symlink": {[]rawEntry{{hdr: tar.Header{Name: "files/l", Typeflag: tar.TypeSymlink, Linkname: "/etc/passwd"}}}, ErrSpecialFile},
@@ -264,6 +274,10 @@ func TestExtractRefusesTraversalAndSpecialEntries(t *testing.T) {
"late bad entry": {[]rawEntry{reg("files/a", "fine"), reg("env", "e"), reg("files/../escape", "x")}, ErrBadPath},
"over cap header": {[]rawEntry{{hdr: tar.Header{Name: "files/big", Typeflag: tar.TypeReg, Size: Cap + 1}}}, ErrOverCap},
}
+}
+
+func TestExtractRefusesTraversalAndSpecialEntries(t *testing.T) {
+ cases := refusedStreams()
for name, c := range cases {
t.Run(name, func(t *testing.T) {
var raw []byte
@@ -1182,3 +1196,53 @@ func TestExtractFsyncTiming(t *testing.T) {
}
}
}
+
+// Fix round 2: Verify and Extract share one set of stream rules, so a
+// stream Extract would refuse never passes Verify only to fail at landing.
+// Every refused stream above is sealed with a manifest that names its
+// valid regular entries correctly, so only the stream rules can refuse it,
+// and Verify must give the same refusal Extract does.
+func TestVerifyRefusesEveryStreamExtractRefuses(t *testing.T) {
+ a := threeChunkArchive(t)
+ all := refusedStreams()
+ for name, entries := range duplicateStreams() {
+ all["duplicate: "+name] = refusedStreamCase{entries, ErrBadPath}
+ }
+ for name, c := range all {
+ t.Run(name, func(t *testing.T) {
+ raw := overCapArchive(t)
+ if c.want != ErrOverCap {
+ raw = tarZst(t, c.entries, nil)
+ }
+ var entries []Entry
+ named := map[string]bool{}
+ for _, e := range c.entries {
+ if e.hdr.Typeflag != tar.TypeReg || named[e.hdr.Name] {
+ continue
+ }
+ if _, _, err := checkName(e.hdr.Name, false); err != nil {
+ continue
+ }
+ named[e.hdr.Name] = true
+ entries = append(entries, Entry{Path: e.hdr.Name, Size: int64(len(e.body)), SHA256: digestBytes(e.body)})
+ }
+ sealed, m := a.sealRaw(t, raw, entries)
+ if err := m.check(); err != nil {
+ t.Fatalf("fixture manifest refused on its own: %v", err)
+ }
+ if err := Verify(bytes.NewReader(sealed), m, a.priv, a.b); !errors.Is(err, c.want) {
+ t.Fatalf("Verify: %v, want %v", err, c.want)
+ }
+ })
+ }
+ // The two directory-order shapes, named as the review asked.
+ for name, entries := range map[string][]rawEntry{
+ "missing parent": {reg("files/a/b", "x")},
+ "late parent": {reg("files/a/b", "x"), dirEntry("files/a/")},
+ } {
+ sealed, m := a.sealRaw(t, tarZst(t, entries, nil), []Entry{{Path: "files/a/b", Size: 1, SHA256: digestBytes([]byte("x"))}})
+ if err := Verify(bytes.NewReader(sealed), m, a.priv, a.b); !errors.Is(err, ErrBadPath) || !strings.Contains(err.Error(), "before its directory") {
+ t.Fatalf("%s: %v", name, err)
+ }
+ }
+}
diff --git a/internal/moves/archive/extract.go b/internal/moves/archive/extract.go
index 417a58d3d..1c3a4092a 100644
--- a/internal/moves/archive/extract.go
+++ b/internal/moves/archive/extract.go
@@ -7,7 +7,6 @@ import (
"io"
"io/fs"
"os"
- "path"
"path/filepath"
"runtime"
"sort"
@@ -52,7 +51,7 @@ func Extract(r io.Reader, filesDir, envPath string) error {
if err != nil {
return err
}
- x := &extraction{stage: stage, envPath: envPath, dirSet: map[string]bool{}, files: newSyncPool(syncWorkers)}
+ x := &extraction{stage: stage, envPath: envPath, files: newSyncPool(syncWorkers)}
defer x.cleanup()
if x.root, err = os.OpenRoot(stage); err != nil {
return err
@@ -104,7 +103,6 @@ type extraction struct {
envPath string
envTmp string // the staged env file, if the archive carried one
dirs []modeFix
- dirSet map[string]bool
files *syncPool // fsyncs and closes each staged file as it is written
}
@@ -144,9 +142,8 @@ func (x *extraction) read(r io.Reader) error {
defer zr.Close()
dec := &boundedReader{r: zr, left: decodedLimit}
tr := tar.NewReader(dec)
- seen := map[string]bool{}
- var content int64
- for count := 0; ; count++ {
+ rules := newStreamRules()
+ for {
hdr, err := tr.Next()
if err == io.EOF {
break
@@ -154,22 +151,10 @@ func (x *extraction) read(r io.Reader) error {
if err != nil {
return err
}
- if count >= entryLimit {
- return ErrTooManyEntries
- }
- name, rel, isEnv, err := headerName(hdr)
+ _, rel, isEnv, err := rules.admit(hdr)
if err != nil {
return err
}
- if seen[name] {
- return fmt.Errorf("%w: %q named twice", ErrBadPath, hdr.Name)
- }
- seen[name] = true
- if !isEnv {
- if dir := path.Dir(rel); dir != "." && !x.dirSet[dir] {
- return fmt.Errorf("%w: %q comes before its directory", ErrBadPath, hdr.Name)
- }
- }
mode := fs.FileMode(hdr.Mode).Perm()
if hdr.Typeflag == tar.TypeDir {
// Created writable so children can land; the archived mode is
@@ -178,13 +163,8 @@ func (x *extraction) read(r io.Reader) error {
return err
}
x.dirs = append(x.dirs, modeFix{rel, mode})
- x.dirSet[rel] = true
continue
}
- if hdr.Size > Cap-content {
- return ErrOverCap
- }
- content += hdr.Size
if isEnv {
err = x.writeEnv(tr, hdr.Size, mode)
} else {
diff --git a/internal/moves/archive/limits.go b/internal/moves/archive/limits.go
index 866068be9..d61e0b29a 100644
--- a/internal/moves/archive/limits.go
+++ b/internal/moves/archive/limits.go
@@ -119,6 +119,57 @@ func headerName(hdr *tar.Header) (name, rel string, isEnv bool, err error) {
return name, rel, isEnv, err
}
+// streamRules are the rules every reader of an archive stream enforces,
+// in one place so Verify and Extract cannot drift apart (fix round 2): a
+// stream Extract would refuse must not pass Verify and fail at landing.
+// Per entry, in order: the entry limit; a valid name and type
+// (headerName); no destination named twice; every entry's directory
+// listed before it (Build's sorted order always does, and Extract needs
+// it so each directory carries its archived mode); and the content cap,
+// checked on the header before any content is read. The manifest lists
+// regular files only, so directory order is a property of the stream
+// alone; Manifest.check applies the same name rule (checkName).
+type streamRules struct {
+ seen map[string]bool
+ dirs map[string]bool
+ count int
+ content int64
+}
+
+func newStreamRules() *streamRules {
+ return &streamRules{seen: map[string]bool{}, dirs: map[string]bool{}}
+}
+
+// admit checks one header against the rules and records it.
+func (s *streamRules) admit(hdr *tar.Header) (name, rel string, isEnv bool, err error) {
+ if s.count >= entryLimit {
+ return "", "", false, ErrTooManyEntries
+ }
+ s.count++
+ name, rel, isEnv, err = headerName(hdr)
+ if err != nil {
+ return "", "", false, err
+ }
+ if s.seen[name] {
+ return "", "", false, fmt.Errorf("%w: %q named twice", ErrBadPath, hdr.Name)
+ }
+ s.seen[name] = true
+ if !isEnv {
+ if dir := path.Dir(rel); dir != "." && !s.dirs[dir] {
+ return "", "", false, fmt.Errorf("%w: %q comes before its directory", ErrBadPath, hdr.Name)
+ }
+ }
+ if hdr.Typeflag == tar.TypeDir {
+ s.dirs[rel] = true
+ return name, rel, isEnv, nil
+ }
+ if hdr.Size > Cap-s.content {
+ return "", "", false, ErrOverCap
+ }
+ s.content += hdr.Size
+ return name, rel, isEnv, nil
+}
+
// boundedReader stops a decoded stream at its limit: past it, one more
// byte is ErrTooLarge, so a stream exactly at the limit still ends cleanly.
type boundedReader struct {
diff --git a/internal/moves/archive/verify.go b/internal/moves/archive/verify.go
index 78bbcba92..81c39daff 100644
--- a/internal/moves/archive/verify.go
+++ b/internal/moves/archive/verify.go
@@ -109,9 +109,8 @@ func verifyStream(r io.Reader, m Manifest) (string, error) {
for _, e := range m.Entries {
want[e.Path] = e
}
- seen := map[string]bool{}
- var content int64
- for count := 0; ; count++ {
+ rules := newStreamRules()
+ for {
hdr, err := tr.Next()
if err == io.EOF {
break
@@ -119,24 +118,13 @@ func verifyStream(r io.Reader, m Manifest) (string, error) {
if err != nil {
return "", err
}
- if count >= entryLimit {
- return "", ErrTooManyEntries
- }
- name, _, _, err := headerName(hdr)
+ name, _, _, err := rules.admit(hdr)
if err != nil {
return "", err
}
- if seen[name] {
- return "", fmt.Errorf("%w: %q named twice", ErrBadPath, hdr.Name)
- }
- seen[name] = true
if hdr.Typeflag == tar.TypeDir {
continue
}
- if hdr.Size > Cap-content {
- return "", ErrOverCap
- }
- content += hdr.Size
e, ok := want[name]
if !ok {
return "", fmt.Errorf("archive: entry %q is not in the manifest", hdr.Name)
@@ -167,8 +155,8 @@ func verifyStream(r io.Reader, m Manifest) (string, error) {
if len(want) != 0 {
return "", fmt.Errorf("archive: %d manifest entries missing from the archive", len(want))
}
- if content != m.Bytes {
- return "", fmt.Errorf("archive: the archive holds %d bytes, manifest says %d", content, m.Bytes)
+ if rules.content != m.Bytes {
+ return "", fmt.Errorf("archive: the archive holds %d bytes, manifest says %d", rules.content, m.Bytes)
}
return digestString(h), nil
}