From 0963bae3c36faab5ca7cc4bf6f0ec7d775a87396 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sun, 18 Oct 2026 19:47:15 +0200 Subject: [PATCH] The partition probe fails closed on every path (R-1093 review, unreleased) PART_ENTRY_* comes from the parent's table, so it proves nothing about a partition's contents unless blkid also completed: lsblk's PTTYPE is now set aside only on a blkid exit 0 (exit 2, an I/O error or a missing binary keep "device has a partition table"). An lsblk that exits 0 with output that does not parse into a device, or lists none, leaves the device unprobed instead of blank (a hole that predates R-1093). Six more TestInspect_R1093_PartitionVerdicts rows, each red-proved. --- CHANGELOG.md | 6 ++++ internal/storage/hostops.go | 34 ++++++++++++----------- internal/storage/hostops_disk_test.go | 40 ++++++++++++++++++++++++--- 3 files changed, 60 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b9fdbe..7a162d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ Committed, not released — the release and its vouch are the operator's. that differs keeps the old, safe reading. **No new command, no sudoers or config-bundle change** (the two probes are the same allow-listed argv). Test `TestInspect_R1093_PartitionVerdicts` (8 rows), red-proved: with the old `if pttype != ""` both empty-partition rows fail `DataBearing=true reason="device has a partition table"`. + **Hardened after the same night's review (fail-closed on every path):** `PART_ENTRY_*` is read from the PARENT's + table, so lsblk's PTTYPE is set aside only when blkid also **exited 0** — exit 2, an I/O error or a missing binary + keep „device has a partition table"; and an `lsblk` that exits 0 with output that does not parse into a device (or + lists none) now leaves the device UNPROBED („could not be reliably inspected") instead of „blank" — that hole + predates R-1093. Six more rows in the same test, each red-proved (`blkidErr` check removed → „device is blank"; + `&& parsed` removed → `Probed=true … „device is blank"`). - **R-978 — an abandoned second escrow ceremony no longer makes the customer's recovery code useless.** The root ceremony (the FELHOM_ESCROW argv, unchanged: `--upload --output=json`) used to upload the new package before the new code was claimed, and the hub overwrites the current package when the sealed secrets are unchanged — so a customer diff --git a/internal/storage/hostops.go b/internal/storage/hostops.go index c3b6f71..4a55a7a 100644 --- a/internal/storage/hostops.go +++ b/internal/storage/hostops.go @@ -419,7 +419,7 @@ func (h *SudoHostOps) InspectDevice(ctx context.Context, device string) (DeviceP // any TYPE/PTTYPE/USAGE line is positive data-bearing evidence. Its exit code is NOT relied // on (blkid exits 2 on a blank device) — output presence is what matters. A broken/empty // blkid simply adds no positive evidence; lsblk (below) is the read-success authority. - bout, _, _ := h.runner.Run(ctx, h.bins.Blkid, "-p", "-o", "export", device) + bout, _, berr := h.runner.Run(ctx, h.bins.Blkid, "-p", "-o", "export", device) blk := parseBlkidExport(bout) for k, v := range blk { switch k { @@ -440,11 +440,11 @@ func (h *SudoHostOps) InspectDevice(ctx context.Context, device string) (DeviceP // lsblk -J is the READ-SUCCESS authority + the partition/mount view. It exits 0 on any valid // device (blank or not), so a clean parse means the agent reliably read the device. If lsblk // fails, Probed stays false → DataBearing()=true (fail-safe — never call a device blank on a - // failed read). + // failed read). An exit 0 whose output does not parse into a device is NOT a read either (R-1093 + // review): it stays unprobed, never "blank". lout, _, lerr := h.runner.Run(ctx, h.bins.Lsblk, "-J", "-o", "NAME,FSTYPE,PTTYPE,MOUNTPOINT", device) - if lerr == nil { + if hasChildren, mounted, fstype, pttype, parsed := parseLsblkDevice(lout); lerr == nil && parsed { probe.Probed = true - hasChildren, mounted, fstype, pttype := parseLsblkDevice(lout) if hasChildren { probe.HasPartitions = true } @@ -457,7 +457,7 @@ func (h *SudoHostOps) InspectDevice(ctx context.Context, device string) (DeviceP probe.FSType = fstype } } - if pttype != "" && !inheritedPartitionTable(blk, pttype) { + if pttype != "" && !inheritedPartitionTable(blk, berr, pttype) { probe.HasPartitionTable = true } } @@ -470,13 +470,14 @@ func (h *SudoHostOps) InspectDevice(ctx context.Context, device string) (DeviceP // // The partition's OWN signatures come from `blkid -p` on the partition itself (above): a nested table // inside it is its PTTYPE, a filesystem is its TYPE. So lsblk's value is set aside ONLY when blkid -// positively proved two things — that it read the device (it printed output), and that the device is a -// partition of a table of exactly that type (PART_ENTRY_NUMBER + PART_ENTRY_SCHEME == lsblk's PTTYPE). -// Anything else keeps the old, safe reading: a blkid that failed or printed nothing, a whole disk, or a -// scheme that differs from lsblk's (then lsblk sees a table blkid did not attribute to the parent). -// Pinned by TestInspect_R1093_* (hostops_disk_test.go). -func inheritedPartitionTable(blkid map[string]string, lsblkPTType string) bool { - if blkid["PART_ENTRY_NUMBER"] == "" { +// positively proved two things — that it completed its probe of the device (exit 0: PART_ENTRY_* alone +// comes from the PARENT's table, so output without a clean exit does not prove the partition's contents +// were read), and that the device is a partition of a table of exactly that type (PART_ENTRY_NUMBER + +// PART_ENTRY_SCHEME == lsblk's PTTYPE). Anything else keeps the old, safe reading: a blkid that failed, +// exited non-zero or printed nothing, a whole disk, or a scheme that differs from lsblk's (then lsblk sees +// a table blkid did not attribute to the parent). Pinned by TestInspect_R1093_* (hostops_disk_test.go). +func inheritedPartitionTable(blkid map[string]string, blkidErr error, lsblkPTType string) bool { + if blkidErr != nil || blkid["PART_ENTRY_NUMBER"] == "" { return false } scheme := blkid["PART_ENTRY_SCHEME"] @@ -561,13 +562,14 @@ type lsblkDevice struct { } // parseLsblkDevice parses `lsblk -J -o NAME,FSTYPE,PTTYPE,MOUNTPOINT ` for the top device: -// whether it has child partitions, is mounted (itself or any child), and its fstype/pttype. -func parseLsblkDevice(out []byte) (hasChildren, mounted bool, fstype, pttype string) { +// whether it has child partitions, is mounted (itself or any child), and its fstype/pttype. parsed=false +// when the output is not a device listing — the caller must then treat the device as unprobed. +func parseLsblkDevice(out []byte) (hasChildren, mounted bool, fstype, pttype string, parsed bool) { var doc struct { BlockDevices []lsblkDevice `json:"blockdevices"` } if json.Unmarshal(out, &doc) != nil || len(doc.BlockDevices) == 0 { - return false, false, "", "" + return false, false, "", "", false } d := doc.BlockDevices[0] fstype, pttype = d.FSType, d.PTType @@ -578,7 +580,7 @@ func parseLsblkDevice(out []byte) (hasChildren, mounted bool, fstype, pttype str mounted = true } } - return hasChildren, mounted, fstype, pttype + return hasChildren, mounted, fstype, pttype, true } // NoopHostOps is the safe fallback when the privileged surface is unavailable or declined diff --git a/internal/storage/hostops_disk_test.go b/internal/storage/hostops_disk_test.go index 42b9e58..c21904c 100644 --- a/internal/storage/hostops_disk_test.go +++ b/internal/storage/hostops_disk_test.go @@ -238,9 +238,12 @@ const lsblkPartInheritsGPT = `{"blockdevices":[{"name":"sdb1","fstype":null,"ptt // TestInspect_R1093_PartitionVerdicts pins the CONSEQUENCE (DataBearing + Reason, what decides whether // the typed erase confirmation is asked) for a partition, in both directions. // -// COMPANION RED-PROOF (observed 2026-10-18): restore the pre-fix `if pttype != "" {` (drop the +// COMPANION RED-PROOFS (observed 2026-10-18): (1) restore the pre-fix `if pttype != "" {` (drop the // inheritedPartitionTable call) → BOTH empty-partition rows (GPT and dos) FAIL with -// `DataBearing=true reason="device has a partition table", want blank`. Restored. +// `DataBearing=true reason="device has a partition table", want blank`. (2) drop the `blkidErr != nil` +// check → the "exit 2" and "I/O error" rows FAIL with `DataBearing=false reason="device is blank"`. +// (3) drop `&& parsed` → both lsblk-exit-0 rows FAIL with `Probed=true … reason="device is blank"` +// (the unparsable-output hole predates R-1093; closed here). Restored. func TestInspect_R1093_PartitionVerdicts(t *testing.T) { cases := []struct { name string @@ -248,11 +251,13 @@ func TestInspect_R1093_PartitionVerdicts(t *testing.T) { blkid string blkidErr error lsblk string + lsblkErr error wantBear bool wantReason string + unprobed bool // the verdict must be "could not be reliably inspected" }{ {name: "empty GPT partition is blank (the R-1093 case)", dev: "/dev/sdb1", - blkid: blkidEmptyGPTPart, blkidErr: errors.New("exit status 2"), lsblk: lsblkPartInheritsGPT, + blkid: blkidEmptyGPTPart, lsblk: lsblkPartInheritsGPT, wantBear: false, wantReason: "device is blank"}, {name: "empty MBR (dos) partition is blank", dev: "/dev/sdb2", blkid: "DEVNAME=/dev/sdb2\nPART_ENTRY_SCHEME=dos\nPART_ENTRY_TYPE=0x83\nPART_ENTRY_NUMBER=2\nPART_ENTRY_DISK=8:16\n", @@ -269,6 +274,26 @@ func TestInspect_R1093_PartitionVerdicts(t *testing.T) { {name: "blkid printed nothing: lsblk's PTTYPE stands (fail-safe)", dev: "/dev/sdb1", blkid: "", blkidErr: errors.New("sudo: a password is required"), lsblk: lsblkPartInheritsGPT, wantBear: true, wantReason: "device has a partition table"}, + // Fail-closed paths (the 2026-10-18 review): PART_ENTRY_* is read from the PARENT's table, so it + // proves nothing about the partition's contents unless blkid also completed (exit 0). + {name: "blkid exit 2 with only PART_ENTRY output: lsblk stands", dev: "/dev/sdb1", + blkid: blkidEmptyGPTPart, blkidErr: errors.New("exit status 2"), lsblk: lsblkPartInheritsGPT, + wantBear: true, wantReason: "device has a partition table"}, + {name: "blkid I/O error after PART_ENTRY output: lsblk stands", dev: "/dev/sdb1", + blkid: blkidEmptyGPTPart, blkidErr: errors.New("exit status 1"), lsblk: lsblkPartInheritsGPT, + wantBear: true, wantReason: "device has a partition table"}, + {name: "blkid binary missing: lsblk stands", dev: "/dev/sdb1", + blkid: "", blkidErr: errors.New(`exec: "/usr/sbin/blkid": file does not exist`), lsblk: lsblkPartInheritsGPT, + wantBear: true, wantReason: "device has a partition table"}, + {name: "lsblk exit 0 with unparsable output: unprobed", dev: "/dev/sdb1", + blkid: blkidEmptyGPTPart, lsblk: `lsblk: something odd`, + wantBear: true, unprobed: true}, + {name: "lsblk exit 0 with no device listed: unprobed", dev: "/dev/sdb1", + blkid: blkidEmptyGPTPart, lsblk: `{"blockdevices":[]}`, + wantBear: true, unprobed: true}, + {name: "lsblk failed: unprobed", dev: "/dev/sdb1", + blkid: blkidEmptyGPTPart, lsblk: "", lsblkErr: errors.New("exit status 32"), + wantBear: true, unprobed: true}, {name: "scheme differs from lsblk's PTTYPE: lsblk stands", dev: "/dev/sdb1", blkid: strings.Replace(blkidEmptyGPTPart, "PART_ENTRY_SCHEME=gpt", "PART_ENTRY_SCHEME=dos", 1), lsblk: lsblkPartInheritsGPT, @@ -285,12 +310,19 @@ func TestInspect_R1093_PartitionVerdicts(t *testing.T) { t.Run(c.name, func(t *testing.T) { r := &scriptedRunner{ outputs: map[string][]byte{"blkid": []byte(c.blkid), "lsblk": []byte(c.lsblk)}, - errs: map[string]error{"blkid": c.blkidErr}, + errs: map[string]error{"blkid": c.blkidErr, "lsblk": c.lsblkErr}, } p, err := newSudo(r).InspectDevice(context.Background(), c.dev) if err != nil { t.Fatal(err) } + if c.unprobed { + if p.Probed || !p.DataBearing() || p.Reason() != "device could not be reliably inspected" { + t.Fatalf("Probed=%v DataBearing=%v reason=%q, want unprobed + data-bearing (probe %+v)", + p.Probed, p.DataBearing(), p.Reason(), p) + } + return + } if !p.Probed { t.Fatal("expected a clean probe (lsblk read cleanly)") }