amadan
moves floor gate: pin the old-prev fallback and the bounded read
Two tests from the review of ac96d0f5. The shell-selection test gains a carlos.prev that rejects the gate's verb with an unknown-verb exit: the units then run the current binary. And a floor file that passes the fstat's size check and grows before the read (a hook between the two, floorGateAfterStat) is refused by the bounded read, with both files grown alike so only the bound can refuse it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+57
−11
internal/host/host_test.go+19 −11internal/host/movesfloorgate_linux.go+5 −0internal/host/movesfloorgate_linux_test.go+33 −0
CI passed — run details
diff --git a/internal/host/host_test.go b/internal/host/host_test.go| index 9fe591b3a..1c1074b1d 100644 |
| --- a/internal/host/host_test.go |
| +++ b/internal/host/host_test.go |
| @@ -1847,18 +1847,22 @@ func TestSystemUnitsSelectPrevByTheFloor(t *testing.T) { |
| noPrev bool |
| want string |
| gate string // which binary ran the floor gate, "" for none |
| + prevNoVerb bool // carlos.prev predates the gate and rejects its verb |
| }{ |
| - {"no journal", false, "", "", false, false, "carlos", ""}, |
| - {"no floor", true, "", "", false, false, "carlos.prev", ""}, |
| - {"no floor, no prev", true, "", "", false, true, "carlos", ""}, |
| - {"floor and a verdict naming it", true, "v0.30.0\n", "v0.30.0\n", false, false, "carlos.prev", "carlos.prev"}, |
| - {"a verdict for an older floor", true, "v0.30.0\n", "v0.29.0\n", false, false, "carlos", "carlos.prev"}, |
| - {"floor and no verdict", true, "v0.30.0\n", "", false, false, "carlos", "carlos.prev"}, |
| - {"no journal, armed and allowed", false, "v0.30.0\n", "v0.30.0\n", false, false, "carlos", ""}, |
| - {"armed, no prev", true, "v0.30.0\n", "v0.30.0\n", false, true, "carlos", ""}, |
| + {"no journal", false, "", "", false, false, "carlos", "", false}, |
| + {"no floor", true, "", "", false, false, "carlos.prev", "", false}, |
| + {"no floor, no prev", true, "", "", false, true, "carlos", "", false}, |
| + {"floor and a verdict naming it", true, "v0.30.0\n", "v0.30.0\n", false, false, "carlos.prev", "carlos.prev", false}, |
| + {"a verdict for an older floor", true, "v0.30.0\n", "v0.29.0\n", false, false, "carlos", "carlos.prev", false}, |
| + {"floor and no verdict", true, "v0.30.0\n", "", false, false, "carlos", "carlos.prev", false}, |
| + {"no journal, armed and allowed", false, "v0.30.0\n", "v0.30.0\n", false, false, "carlos", "", false}, |
| + {"armed, no prev", true, "v0.30.0\n", "v0.30.0\n", false, true, "carlos", "", false}, |
| // The agent owns /data: a symlink there must not choose the |
| // file root compares (cmp -s followed it). |
| - {"a symlinked floor naming the verdict", true, "v0.30.0\n", "v0.30.0\n", true, false, "carlos", "carlos.prev"}, |
| + {"a symlinked floor naming the verdict", true, "v0.30.0\n", "v0.30.0\n", true, false, "carlos", "carlos.prev", false}, |
| + // A carlos.prev older than the gate answers its verb with |
| + // an unknown-verb exit: a mismatch, so carlos runs. |
| + {"a carlos.prev without the gate verb", true, "v0.30.0\n", "v0.30.0\n", false, false, "carlos", "carlos.prev", true}, |
| } { |
| t.Run(tc.name, func(t *testing.T) { |
| root := t.TempDir() |
| @@ -1870,11 +1874,15 @@ func TestSystemUnitsSelectPrevByTheFloor(t *testing.T) { |
| // test binary (TestFloorGateHelperProcess), noting which |
| // stub was asked; anything else records the stub as run. |
| stub := func(name string) { |
| + answer := " shift 2\n" + |
| + " exec " + self + " -test.run='^TestFloorGateHelperProcess$' -- \"$@\"\n" |
| + if name == "carlos.prev" && tc.prevNoVerb { |
| + answer = " echo 'system: unknown verb \"floor-allows-prev\"' >&2\n exit 2\n" |
| + } |
| sh := "#!/bin/sh\n" + |
| "if [ \"$1\" = system ] && [ \"$2\" = floor-allows-prev ]; then\n" + |
| " echo " + name + " >> " + gate + "\n" + |
| - " shift 2\n" + |
| - " exec " + self + " -test.run='^TestFloorGateHelperProcess$' -- \"$@\"\n" + |
| + answer + |
| "fi\n" + |
| "echo " + name + " > " + ran + "\n" |
| os.WriteFile(filepath.Join(opt, name), []byte(sh), 0o755) |
diff --git a/internal/host/movesfloorgate_linux.go b/internal/host/movesfloorgate_linux.go| index 993075b97..badf8c23e 100644 |
| --- a/internal/host/movesfloorgate_linux.go |
| +++ b/internal/host/movesfloorgate_linux.go |
| @@ -73,6 +73,7 @@ func readFloorGateFile(path string, owners []uint32, pinDirs bool) ([]byte, erro |
| if err := checkFloorGateStat(floorGateStat{mode: st.Mode, uid: st.Uid, nlink: uint64(st.Nlink), size: st.Size}, owners); err != nil { |
| return nil, fmt.Errorf("%s %w", path, err) |
| } |
| + floorGateAfterStat(path) |
| // Read one byte past the limit, so a file that grew after the fstat is |
| // refused rather than cut short into a match. |
| b, err := io.ReadAll(io.LimitReader(f, floorGateMaxBytes+1)) |
| @@ -85,6 +86,10 @@ func readFloorGateFile(path string, owners []uint32, pinDirs bool) ([]byte, erro |
| return b, nil |
| } |
| +// floorGateAfterStat runs between a file's fstat and its read; a test sets |
| +// it to grow the file in that window. |
| +var floorGateAfterStat = func(path string) {} |
| + |
| // floorGateStat is what checkFloorGateStat judges, from an fstat. |
| type floorGateStat struct { |
| mode uint32 |
diff --git a/internal/host/movesfloorgate_linux_test.go b/internal/host/movesfloorgate_linux_test.go| index 1054f0d01..2cc7d5901 100644 |
| --- a/internal/host/movesfloorgate_linux_test.go |
| +++ b/internal/host/movesfloorgate_linux_test.go |
| @@ -254,6 +254,39 @@ func TestFloorGateRefusesOversizedFiles(t *testing.T) { |
| }) |
| } |
| +// A file that passes the fstat's size check and then grows before the read |
| +// is refused by the bounded read. Both files grow by the same bytes, so |
| +// without that bound the two reads, cut at the same length, would match. |
| +func TestFloorGateRefusesAFileThatGrowsAfterTheStat(t *testing.T) { |
| + f := newFloorGateFixture(t) |
| + f.write(t) |
| + grown := map[string]bool{} |
| + orig := floorGateAfterStat |
| + floorGateAfterStat = func(path string) { |
| + grown[path] = true |
| + w, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND, 0) |
| + if err != nil { |
| + t.Error(err) |
| + return |
| + } |
| + defer w.Close() |
| + if _, err := w.WriteString(strings.Repeat("v", 2*floorGateMaxBytes)); err != nil { |
| + t.Error(err) |
| + } |
| + } |
| + t.Cleanup(func() { floorGateAfterStat = orig }) |
| + err := f.run() |
| + if err == nil { |
| + t.Fatal("a floor that grew past the limit after the fstat passed") |
| + } |
| + if !errors.Is(err, errFloorGateUnsafe) { |
| + t.Fatalf("err = %v, want the bounded read's refusal", err) |
| + } |
| + if !grown[f.floor] { |
| + t.Fatal("the hook never ran between the floor's fstat and its read") |
| + } |
| +} |
| + |
| func TestFloorGateRefusesAnUnexpectedOwner(t *testing.T) { |
| // Without root a test cannot chown, so the expected owners move instead. |
| other := uint32(os.Getuid()) + 1 |