amadan
claim-step6-artifacts fix round 2: cap ELF section names, never remove a reserved --out
Astra's re-review; controller rulings: - F1 (fix): release.CheckELFBounds also caps the section-name table (SHT_STRTAB, 1 byte to 64 KiB), every section's name offset inside it and every name at 256 bytes up to its NUL, before buildinfo.Read. debug/elf copies each section's name, so the copies are bounded at 256 x 256 bytes. A 32 MiB name repeated across every section is refused through publish-identity with no write; real stripped cmd/console and carlos linux-arm64 builds pass. No further parser hardening on this branch. - F5 (fix): a reserved --out file is never removed. abandon is gone; on any failure the file stays and the error names it, says whether the phase wrote to the bucket first, and asks for a new --out path. Census: new overlay plan14-root-reads-claim-step6-artifacts-fix2.tsv, PROPOSED (reserveOutput new, reserveActivationOutputAt removed, two envelopes re-pinned); 9B8 unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8 files changed,
+195
−54
docs/superpowers/notes/census-proposal-claim-step6-artifacts.md+9 −0internal/claimactivation/artifacts_elf_test.go+23 −1internal/host/activation_artifacts_linux.go+32 −25internal/host/activation_artifacts_linux_test.go+43 −24internal/provision/plan14_root_reads_test.go+7 −0internal/provision/testdata/plan14-root-reads-claim-step6-artifacts-fix2.tsv+7 −0internal/release/artifactbuild_test.go+37 −0internal/release/elfbounds.go+37 −4
diff --git a/docs/superpowers/notes/census-proposal-claim-step6-artifacts.md b/docs/superpowers/notes/census-proposal-claim-step6-artifacts.md| index 0b536f682..b23ea802f 100644 |
| --- a/docs/superpowers/notes/census-proposal-claim-step6-artifacts.md |
| +++ b/docs/superpowers/notes/census-proposal-claim-step6-artifacts.md |
| @@ -77,3 +77,12 @@ Astra's review blocked the merge on four findings. The controller ruled F1, F3 a |
| Root-read rows: `ArtifactBuild` and `runSystemClaims` are changed; `reserveActivationOutputAt`, `activationOutput.finish` and `activationOutput.ours` are new, all authority G,J4, executing as the operator CLI, uid 0, on the trusted signing host. The `internal/claimactivation`, `internal/host` and `internal/release` envelopes are re-pinned. The seal stands. |
| Record-write census (9B8): one new entry, `internal/host/activation_artifacts_linux.go:putAttempts.Put`, proposed `nonrecord` (`internal/provision/claim_step6_artifacts_census_test.go`, digest `0c8bbda8...b848`). It wraps the store claims run hands the two phases, only to record that a write was attempted. Every Put it passes on comes from the runner's checkedStore and its allow-list, and it adds no key, etag or body. |
| + |
| +## Fix round 2 (PROPOSED, `rev3=PROPOSED-claim-step6-artifacts-fix2`) |
| + |
| +Astra's re-review found F1 incomplete and a new LOW (F5). The controller ruled both FIX. Overlay `internal/provision/testdata/plan14-root-reads-claim-step6-artifacts-fix2.tsv`, loaded after fix1, awaits a ruling. |
| + |
| +- **F1, section names.** `release.CheckELFBounds` also caps the section-name table: type SHT_STRTAB, 1 byte to 64 KiB, every section's name offset inside it, and every name at most 256 bytes up to its NUL. debug/elf copies each section's name, so the names it can copy are at most 256 x 256 bytes. No read site is added, `ArtifactBuild`'s digest is unchanged, and the `internal/release` envelope is re-pinned. A real stripped `cmd/console` and carlos linux-arm64 build pass. A 32 MiB name repeated across every section is refused through publish-identity with no write. |
| +- **F5, never remove a reservation.** `activationOutput.abandon` is gone. On any failure the reserved `--out` file stays, and the error names it, says whether the phase wrote to the bucket first, and asks for a new `--out` path. `reserveActivationOutputAt` is now a wrapper with no read site (`@remove`). Its reads moved unchanged into `reserveOutput` (new row, authority G,J4, operator CLI uid 0 on the trusted signing host), which also names the full path in its errors. The `internal/host` envelope is re-pinned. |
| + |
| +The 9B8 inventory is unchanged: `putAttempts.Put` is byte for byte as ruled. The seal stands. |
diff --git a/internal/claimactivation/artifacts_elf_test.go b/internal/claimactivation/artifacts_elf_test.go| index 1decf69d3..664709ffc 100644 |
| --- a/internal/claimactivation/artifacts_elf_test.go |
| +++ b/internal/claimactivation/artifacts_elf_test.go |
| @@ -70,7 +70,10 @@ func TestPublishIdentityBoundsTheConsoleBinary(t *testing.T) { |
| h := c[shoff+shstrndx*64:] |
| le.PutUint64(h[8:], le.Uint64(h[8:])|0x800) |
| }), false}, |
| - "oversized shnum": {mutate(func(c []byte) { le.PutUint16(c[60:], 65000) }), false}, |
| + "oversized shnum": {mutate(func(c []byte) { le.PutUint16(c[60:], 65000) }), false}, |
| + // Astra's re-review: every section named by one 32 MiB string, which |
| + // debug/elf would copy once per section. |
| + "repeated 32 MiB name": {repeatedName(console, 32<<20), false}, |
| "console stamped another tag": {realConsole(t, "v0.28.1"), false}, |
| } { |
| t.Run(name, func(t *testing.T) { |
| @@ -105,3 +108,22 @@ func TestPublishIdentityBoundsTheConsoleBinary(t *testing.T) { |
| }) |
| } |
| } |
| + |
| +// repeatedName appends a name table holding one name of size bytes to a |
| +// real build and points the section-name table, and every section's name, |
| +// at it. |
| +func repeatedName(b []byte, size int) []byte { |
| + c := append(append([]byte(nil), b...), make([]byte, size+1)...) |
| + for i := len(b); i < len(b)+size; i++ { |
| + c[i] = 'a' |
| + } |
| + le := binary.LittleEndian |
| + shoff, shnum, idx := le.Uint64(c[40:]), uint64(le.Uint16(c[60:])), uint64(le.Uint16(c[62:])) |
| + h := c[shoff+idx*64:] |
| + le.PutUint64(h[24:], uint64(len(b))) |
| + le.PutUint64(h[32:], uint64(size+1)) |
| + for i := uint64(0); i < shnum; i++ { |
| + le.PutUint32(c[shoff+i*64:], 0) |
| + } |
| + return c |
| +} |
diff --git a/internal/host/activation_artifacts_linux.go b/internal/host/activation_artifacts_linux.go| index e5aedbe92..2708a9d11 100644 |
| --- a/internal/host/activation_artifacts_linux.go |
| +++ b/internal/host/activation_artifacts_linux.go |
| @@ -29,6 +29,12 @@ import ( |
| // naming the same path fails its own reservation and publishes nothing. The |
| // phase's bytes are then written through the held descriptor, synced and |
| // read back. A dry run only checks the path is free. |
| +// |
| +// A reservation is never removed (controller ruling F5): on any failure the |
| +// file stays, empty or partial, and the error names it and asks for a new |
| +// --out path. Removing it safely would need its identity check and the |
| +// unlink to be one step, which a directory another root writer can change |
| +// does not offer. |
| type activationOutput struct { |
| dir *os.File |
| ownDir bool |
| @@ -48,7 +54,7 @@ func reserveActivationOutput(path string, dry bool) (*activationOutput, error) { |
| if err != nil { |
| return nil, err |
| } |
| - o, err := reserveActivationOutputAt(dir, filepath.Base(path), dry) |
| + o, err := reserveOutput(dir, filepath.Base(path), path, dry) |
| if err != nil { |
| dir.Close() |
| return nil, err |
| @@ -60,6 +66,11 @@ func reserveActivationOutput(path string, dry bool) (*activationOutput, error) { |
| // reserveActivationOutputAt reserves name under an already checked |
| // directory. Any existing entry is refused without following it. |
| func reserveActivationOutputAt(dir *os.File, name string, dry bool) (*activationOutput, error) { |
| + return reserveOutput(dir, name, name, dry) |
| +} |
| + |
| +// reserveOutput is the reservation, naming path in its errors. |
| +func reserveOutput(dir *os.File, name, path string, dry bool) (*activationOutput, error) { |
| if name != filepath.Base(name) || name == "." || name == ".." || name == "/" { |
| return nil, errActivation |
| } |
| @@ -71,7 +82,7 @@ func reserveActivationOutputAt(dir *os.File, name string, dry bool) (*activation |
| if err != unix.ENOENT { |
| return nil, err |
| } |
| - o := &activationOutput{dir: dir, name: name, path: name} |
| + o := &activationOutput{dir: dir, name: name, path: path} |
| if dry { |
| return o, nil |
| } |
| @@ -91,8 +102,8 @@ func reserveActivationOutputAt(dir *os.File, name string, dry bool) (*activation |
| } |
| o.f, o.dev, o.ino = f, uint64(st.Dev), st.Ino |
| if err != nil { |
| - o.abandon() |
| - return nil, err |
| + f.Close() |
| + return nil, outputKept(err, path, false) |
| } |
| return o, nil |
| } |
| @@ -135,18 +146,6 @@ func (o *activationOutput) finish(raw []byte) error { |
| return unix.Fsync(int(o.dir.Fd())) |
| } |
| -// abandon removes the reserved file, only while the entry is still the file |
| -// this reservation created, then closes it. It is for failures before |
| -// anything reached the bucket. |
| -func (o *activationOutput) abandon() { |
| - if o.f != nil && o.ours() { |
| - if unix.Unlinkat(int(o.dir.Fd()), o.name, 0) == nil { |
| - _ = unix.Fsync(int(o.dir.Fd())) |
| - } |
| - } |
| - o.close() |
| -} |
| - |
| // close releases the reservation and keeps whatever the file holds. |
| func (o *activationOutput) close() { |
| if o.f != nil { |
| @@ -161,7 +160,7 @@ func (o *activationOutput) close() { |
| // putAttempts wraps a phase's store and records whether any write was |
| // attempted. A write that failed may still have committed, so an attempt, |
| -// not a success, is what keeps the reserved file after a failure. |
| +// not a success, is what the error after a failure reports. |
| type putAttempts struct { |
| release.Store |
| attempted atomic.Bool |
| @@ -179,9 +178,10 @@ func (s *putAttempts) Delete(ctx context.Context, key string) error { |
| // runProducingPhase runs sign-policy or publish-identity with its output |
| // already reserved (out), on a runner whose store is store. On success the |
| -// output holds the phase's file. On a failure before any bucket write the |
| -// reservation is removed; after one, the file is kept and the error says |
| -// so, because the bucket may already hold what the phase published. |
| +// output holds the phase's file. On any failure the reserved file is kept |
| +// and the error names it, asks for a new --out path and says whether the |
| +// phase wrote to the bucket first, because the bucket may then already hold |
| +// what it published. |
| func runProducingPhase(ctx context.Context, r claimactivation.Runner, store *putAttempts, plan claimactivation.Plan, out *activationOutput, dry bool) (claimactivation.Result, error) { |
| r.Directory.Store = store |
| result, err := r.Run(ctx, plan, dry) |
| @@ -192,12 +192,19 @@ func runProducingPhase(ctx context.Context, r claimactivation.Runner, store *put |
| out.close() |
| return result, nil |
| } |
| - if !dry && store.attempted.Load() { |
| - out.close() |
| - return result, fmt.Errorf("%w; the phase wrote to the bucket before it failed, so %s was kept", err, out.path) |
| + out.close() |
| + if dry { |
| + return result, err |
| + } |
| + return result, outputKept(err, out.path, store.attempted.Load()) |
| +} |
| + |
| +// outputKept is a failed phase's error once its reserved --out file stays. |
| +func outputKept(err error, path string, wrote bool) error { |
| + if wrote { |
| + return fmt.Errorf("%w; the phase wrote to the bucket before it failed, so %s was kept. run it again with a new --out path", err, path) |
| } |
| - out.abandon() |
| - return result, err |
| + return fmt.Errorf("%w; %s was kept. run it again with a new --out path", err, path) |
| } |
| // consoleInstallDir is where install-console puts the protected console |
diff --git a/internal/host/activation_artifacts_linux_test.go b/internal/host/activation_artifacts_linux_test.go| index 5e4eebc3a..2ff4b715d 100644 |
| --- a/internal/host/activation_artifacts_linux_test.go |
| +++ b/internal/host/activation_artifacts_linux_test.go |
| @@ -111,12 +111,9 @@ func writeOut(dir *os.File, name string, raw []byte, dry bool) error { |
| if err != nil { |
| return err |
| } |
| - if err = out.finish(raw); err != nil { |
| - out.abandon() |
| - return err |
| - } |
| + err = out.finish(raw) |
| out.close() |
| - return nil |
| + return err |
| } |
| func openDir(t *testing.T) (*os.File, string) { |
| @@ -313,41 +310,64 @@ func TestActivationOutputCompetingInvocations(t *testing.T) { |
| } |
| } |
| -// A failure before any bucket write removes the reservation, and only the |
| -// file this run created: an entry someone put in its place stays. |
| -func TestActivationOutputAbandonRemovesOnlyItsOwnFile(t *testing.T) { |
| +// F5: a failure keeps the reservation, never removes it, and the error |
| +// names it and asks for a new --out path. A failure before any bucket write |
| +// says only that. Nothing is removed even when the entry was replaced: the |
| +// write refuses and the replacement stays. |
| +func TestActivationOutputFailureKeepsTheReservation(t *testing.T) { |
| w := newArtifactWorld(t) |
| dir, dirPath := openDir(t) |
| path := filepath.Join(dirPath, "policy.json") |
| stale := w.policyPlan(t) |
| stale.ExpectedRevision-- |
| - out, err := reserveActivationOutputAt(dir, "policy.json", false) |
| + out, err := reserveOutput(dir, "policy.json", path, false) |
| if err != nil { |
| t.Fatal(err) |
| } |
| attempts := &putAttempts{Store: w.store} |
| - if _, err = runProducingPhase(t.Context(), w.r, attempts, stale, out, false); err == nil || attempts.attempted.Load() { |
| - t.Fatal("stale plan accepted", err) |
| + _, err = runProducingPhase(t.Context(), w.r, attempts, stale, out, false) |
| + if err == nil || attempts.attempted.Load() || !strings.HasSuffix(err.Error(), "; "+path+" was kept. run it again with a new --out path") { |
| + t.Fatal("stale plan", err) |
| + } |
| + if st, err := os.Lstat(path); err != nil || st.Size() != 0 || st.Mode() != 0o600 { |
| + t.Fatal("reservation not kept", st, err) |
| } |
| - if _, err = os.Lstat(path); !os.IsNotExist(err) { |
| - t.Fatal("reservation left behind", err) |
| + if _, err = reserveOutput(dir, "policy.json", path, false); err == nil { |
| + t.Fatal("rerun on the kept path accepted") |
| } |
| - out, err = reserveActivationOutputAt(dir, "policy.json", false) |
| + // The dry run reserves nothing, so it has nothing to keep. |
| + other := filepath.Join(dirPath, "dry.json") |
| + out, err = reserveOutput(dir, "dry.json", other, true) |
| if err != nil { |
| t.Fatal(err) |
| } |
| - if err = os.Rename(path, path+".moved"); err != nil { |
| + if _, err = runProducingPhase(t.Context(), w.r, &putAttempts{Store: w.store}, stale, out, true); err == nil || strings.Contains(err.Error(), "was kept") { |
| + t.Fatal("dry run", err) |
| + } |
| + if _, err = os.Lstat(other); !os.IsNotExist(err) { |
| + t.Fatal("dry run created its output", err) |
| + } |
| + // A replaced entry: the write refuses and nothing is removed. |
| + replaced := filepath.Join(dirPath, "identity.json") |
| + out, err = reserveOutput(dir, "identity.json", replaced, false) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if err = os.Rename(replaced, replaced+".moved"); err != nil { |
| t.Fatal(err) |
| } |
| - if err = os.WriteFile(path, []byte("someone else's"), 0o600); err != nil { |
| + if err = os.WriteFile(replaced, []byte("someone else's"), 0o600); err != nil { |
| t.Fatal(err) |
| } |
| - if err = out.finish([]byte("{}")); err == nil { |
| - t.Fatal("finished into a replaced entry") |
| + _, err = runProducingPhase(t.Context(), w.r, &putAttempts{Store: w.store}, w.policyPlan(t), out, false) |
| + if err == nil || !strings.HasSuffix(err.Error(), replaced+" was kept. run it again with a new --out path") { |
| + t.Fatal("finished into a replaced entry", err) |
| + } |
| + if b, err := os.ReadFile(replaced); err != nil || string(b) != "someone else's" { |
| + t.Fatal("the replacement changed", err) |
| } |
| - out.abandon() |
| - if b, err := os.ReadFile(path); err != nil || string(b) != "someone else's" { |
| - t.Fatal("abandon removed a file it did not create", err) |
| + if _, err = os.Lstat(replaced + ".moved"); err != nil { |
| + t.Fatal("the reservation was removed", err) |
| } |
| } |
| @@ -379,14 +399,13 @@ func (s *failingReadback) Get(ctx context.Context, key string) ([]byte, string, |
| func TestActivationOutputKeptAfterPublication(t *testing.T) { |
| w := newArtifactWorld(t) |
| dir, dirPath := openDir(t) |
| - out, err := reserveActivationOutputAt(dir, "identity.json", false) |
| + out, err := reserveOutput(dir, "identity.json", filepath.Join(dirPath, "identity.json"), false) |
| if err != nil { |
| t.Fatal(err) |
| } |
| - out.path = filepath.Join(dirPath, "identity.json") |
| key := "control/system/router/releases/v0.28.2.json" |
| _, err = runProducingPhase(t.Context(), w.r, &putAttempts{Store: &failingReadback{Store: w.store, key: key}}, w.identityPlan(t), out, false) |
| - if err == nil || !strings.Contains(err.Error(), "the phase wrote to the bucket before it failed, so "+out.path+" was kept") { |
| + if err == nil || !strings.Contains(err.Error(), "the phase wrote to the bucket before it failed, so "+out.path+" was kept. run it again with a new --out path") { |
| t.Fatal("failure after publication", err) |
| } |
| if _, err = os.Lstat(out.path); err != nil { |
diff --git a/internal/provision/plan14_root_reads_test.go b/internal/provision/plan14_root_reads_test.go| index cd6afeccb..2fae6cbaf 100644 |
| --- a/internal/provision/plan14_root_reads_test.go |
| +++ b/internal/provision/plan14_root_reads_test.go |
| @@ -851,6 +851,13 @@ func TestPlan14RootReadCensus(t *testing.T) { |
| if err = loadRootReadInventory(filepath.Join(root, "internal/provision/testdata/plan14-root-reads-claim-step6-artifacts-fix1.tsv"), &inv, true); err != nil { |
| t.Fatal(err) |
| } |
| + // Census overlay claim-step6-artifacts-fix2 (fix round 2: F1 name caps, |
| + // F5 a reserved --out file is never removed), loaded last. PROPOSED for |
| + // the controller's ruling. It adds no accepted-legacy row, so the seal |
| + // stands. |
| + if err = loadRootReadInventory(filepath.Join(root, "internal/provision/testdata/plan14-root-reads-claim-step6-artifacts-fix2.tsv"), &inv, true); err != nil { |
| + t.Fatal(err) |
| + } |
| errs := append(compareRootReads(got, inv), checkRootReadLegacy(inv, seal)...) |
| errs = append(errs, checkRootReadDeclarations(got, inv)...) |
| if len(errs) > 0 { |
diff --git a/internal/provision/testdata/plan14-root-reads-claim-step6-artifacts-fix2.tsv b/internal/provision/testdata/plan14-root-reads-claim-step6-artifacts-fix2.tsv| new file mode 100644 |
| index 000000000..34dfa817c |
| --- /dev/null |
| +++ b/internal/provision/testdata/plan14-root-reads-claim-step6-artifacts-fix2.tsv |
| @@ -0,0 +1,7 @@ |
| +# Census overlay claim-step6-artifacts-fix2 (fix round 2 after Astra's re-review: controller rulings F1 and F5), loaded LAST, after claim-step6-artifacts-fix1. PROPOSED: every row marked rev3=PROPOSED-claim-step6-artifacts-fix2 waits for the controller's ruling; docs/superpowers/notes/census-proposal-claim-step6-artifacts.md, section "Fix round 2". |
| +# F1 internal/release CheckELFBounds (no read site) also caps the section-name table at 64 KiB, every name offset inside it and every name at 256 bytes up to its NUL, before ArtifactBuild calls buildinfo.Read (ArtifactBuild's digest is unchanged). F5 internal/host: a reserved --out file is never removed; activationOutput.abandon is gone, and every failure keeps the file and asks for a new --out path (outputKept, no read site). reserveActivationOutputAt is now a wrapper with no read site; its reads moved unchanged into reserveOutput, which also names the full path in its errors. |
| +# The seal stands: no accepted-legacy row. No box agent, update, recovery or edge path reaches any changed unit. The 9B8 inventory is unchanged. |
| +@coverage internal/host fd2371702b42080690d2b8c0442695bec53bcb34d8842e487007d7825d5fbf7b |
| +@coverage internal/release f4b2953a76bf7cb0b50abfe6a7022181e127f1018710b41159349c6a8dd0f5bf |
| +@remove internal/host/activation_artifacts_linux.go reserveActivationOutputAt |
| +internal/host/activation_artifacts_linux.go reserveOutput authority G,J4 rev3=PROPOSED-claim-step6-artifacts-fix2;identity=explicit operator CLI uid 0 on the trusted signing host;basis=checked root-owned parent; refuses any existing entry without following it; exclusive 0600 create held open, owner checked, directory synced; never removes what it created: a failure after the create keeps the file and the error asks for a new --out path; dry-run only checks the path is free file:Fstat@653,file:Fstatat@230 45021760106ca7a1a2216e160a770e407e91b1f42c67fc47bb2076283ba034f5 |
diff --git a/internal/release/artifactbuild_test.go b/internal/release/artifactbuild_test.go| index f8629f6dc..192478d10 100644 |
| --- a/internal/release/artifactbuild_test.go |
| +++ b/internal/release/artifactbuild_test.go |
| @@ -105,6 +105,26 @@ func elfOversizedShnum(b []byte) []byte { |
| return elfMutate(b, func(c []byte, _, _ uint64) { binary.LittleEndian.PutUint16(c[60:], 65000) }) |
| } |
| +// elfRepeatedName appends a name table holding one name of size bytes |
| +// to a real build, points its section-name table at it and every section's |
| +// name at that one name: the shape debug/elf would copy once per section |
| +// (Astra's re-review of F1). |
| +func elfRepeatedName(b []byte, size int) []byte { |
| + c := append(append([]byte(nil), b...), make([]byte, size+1)...) |
| + for i := len(b); i < len(b)+size; i++ { |
| + c[i] = 'a' |
| + } |
| + le := binary.LittleEndian |
| + shoff, shnum, idx := le.Uint64(c[40:]), uint64(le.Uint16(c[60:])), uint64(le.Uint16(c[62:])) |
| + h := c[shoff+idx*64:] |
| + le.PutUint64(h[24:], uint64(len(b))) |
| + le.PutUint64(h[32:], uint64(size+1)) |
| + for i := uint64(0); i < shnum; i++ { |
| + le.PutUint32(c[shoff+i*64:], 0) |
| + } |
| + return c |
| +} |
| + |
| func TestCheckELFBoundsRefusesUnboundedMetadata(t *testing.T) { |
| if testing.Short() { |
| t.Skip("builds Go binaries") |
| @@ -113,6 +133,10 @@ func TestCheckELFBoundsRefusesUnboundedMetadata(t *testing.T) { |
| if err := CheckELFBounds(real); err != nil { |
| t.Fatal("a real stripped linux-arm64 build refused", err) |
| } |
| + // A 256-byte name, repeated, is inside the caps: 256 x 256 bytes at most. |
| + if err := CheckELFBounds(elfRepeatedName(real, 256)); err != nil { |
| + t.Fatal("a name at the cap refused", err) |
| + } |
| le := binary.LittleEndian |
| for name, crafted := range map[string][]byte{ |
| "compressed section-name table": elfShstrtabCompressed(real), |
| @@ -146,6 +170,19 @@ func TestCheckELFBoundsRefusesUnboundedMetadata(t *testing.T) { |
| le.PutUint64(b[32:], 16) |
| } |
| }), |
| + "repeated 32 MiB name": elfRepeatedName(real, 32<<20), |
| + "name table over 64 KiB": elfRepeatedName(real, 64<<10), |
| + "repeated 257-byte name": elfRepeatedName(real, 257), |
| + "name table not a strtab": elfMutate(real, func(c []byte, shoff, _ uint64) { |
| + le.PutUint32(c[shoff+uint64(le.Uint16(c[62:]))*64+4:], 1) |
| + }), |
| + "empty name table": elfMutate(real, func(c []byte, shoff, _ uint64) { |
| + le.PutUint64(c[shoff+uint64(le.Uint16(c[62:]))*64+32:], 0) |
| + }), |
| + "name offset outside the table": elfMutate(real, func(c []byte, shoff, _ uint64) { |
| + size := le.Uint64(c[shoff+uint64(le.Uint16(c[62:]))*64+32:]) |
| + le.PutUint32(c[shoff+64:], uint32(size)) |
| + }), |
| "x86-64": elfMutate(real, func(c []byte, _, _ uint64) { le.PutUint16(c[18:], 62) }), |
| "32-bit class": elfMutate(real, func(c []byte, _, _ uint64) { c[4] = 1 }), |
| "big endian": elfMutate(real, func(c []byte, _, _ uint64) { c[5] = 2 }), |
diff --git a/internal/release/elfbounds.go b/internal/release/elfbounds.go| index 9c5648279..d27c20dea 100644 |
| --- a/internal/release/elfbounds.go |
| +++ b/internal/release/elfbounds.go |
| @@ -1,6 +1,7 @@ |
| package release |
| import ( |
| + "bytes" |
| "encoding/binary" |
| "errors" |
| "sort" |
| @@ -19,7 +20,10 @@ const ( |
| elfPhentSize = 56 |
| elfShentSize = 64 |
| elfMachAArch64 = 183 |
| - elfNoBits = 8 // SHT_NOBITS: no file contents |
| + elfStrTab = 3 // SHT_STRTAB |
| + elfNoBits = 8 // SHT_NOBITS: no file contents |
| + elfMaxNameTab = 64 << 10 |
| + elfMaxName = 256 |
| elfCompressed = 0x800 // SHF_COMPRESSED |
| elfXIndex = 0xffff |
| ) |
| @@ -36,10 +40,17 @@ const ( |
| // - every segment's file range inside the file; |
| // - every section's file range inside the file, no section flagged |
| // SHF_COMPRESSED (whose decompression is unbounded by the file size), |
| -// and no two sections with contents overlapping. |
| +// and no two sections with contents overlapping; |
| +// - a section-name table (e_shstrndx) of type SHT_STRTAB, 1 byte to |
| +// 64 KiB, every section's name offset inside it, and every name at |
| +// most 256 bytes up to its NUL. |
| // |
| -// With no compressed section and every range inside the bounded file, the |
| -// general ELF reader's allocations are bounded by the artefact's size. |
| +// debug/elf copies each section's name out of the name table, so names |
| +// that all point at one huge string would multiply it by the section count |
| +// (Astra's re-review, controller ruling F1). With the caps, the names it |
| +// copies are at most 256 x 256 bytes, no section is decompressed, and every |
| +// range lies inside the bounded file. No further parser hardening is in |
| +// scope for this branch. |
| func CheckELFBounds(b []byte) error { |
| n := uint64(len(b)) |
| if n < elfHeaderSize || string(b[:4]) != "\x7fELF" || b[4] != 2 || b[5] != 1 || b[6] != 1 { |
| @@ -86,6 +97,28 @@ func CheckELFBounds(b []byte) error { |
| } |
| spans = append(spans, span{off, off + size}) |
| } |
| + names := b[shoff+shstrndx*elfShentSize:] |
| + if le.Uint32(names[4:]) != elfStrTab { |
| + return ErrELFBounds |
| + } |
| + nameOff, nameSize := le.Uint64(names[24:]), le.Uint64(names[32:]) |
| + if nameSize == 0 || nameSize > elfMaxNameTab || !within(nameOff, nameSize) { |
| + return ErrELFBounds |
| + } |
| + table := b[nameOff : nameOff+nameSize] |
| + for i := uint64(0); i < shnum; i++ { |
| + at := uint64(le.Uint32(b[shoff+i*elfShentSize:])) |
| + if at >= nameSize { |
| + return ErrELFBounds |
| + } |
| + name := table[at:] |
| + if len(name) > elfMaxName+1 { |
| + name = name[:elfMaxName+1] |
| + } |
| + if bytes.IndexByte(name, 0) < 0 { |
| + return ErrELFBounds |
| + } |
| + } |
| sort.Slice(spans, func(i, j int) bool { return spans[i].off < spans[j].off }) |
| for i := 1; i < len(spans); i++ { |
| if spans[i].off < spans[i-1].end { |