From 98f3cdd76af46ecf0fd765d3557ea0f0012f043c Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Fri, 16 Oct 2026 04:02:00 +0200 Subject: [PATCH] Security review of 912f7c1: the mkfs wrapper trusts no sibling claim; the internal-place helper fails closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - felhom-mkfs-guarded: the 0.158.0 exemption for a sibling part mounted at /mnt/ + under /mnt/felhom-drives is removed — the agent can make that corroborating mount itself. Every mount of every node outside /mnt/felhom-drives refuses (kernel mount table); an unlistable disk refuses. CandidatePartition.Formattable is false for every part of a disk with a mounted node, so it is never offered for formatting (attach still is). - felhom-internal-place: a failed lvs listing, a listing without the pool, or a blkid exit other than 0/2 refuses (they read as "nothing promised" / "no signature" before). Every new test was seen failing against the 0.158.0 versions. Claude-Session: https://claude.ai/code/session_012qRErfCoiTkvDK9N5XHbzb --- REUSE.md | 2 +- configs/felhom-internal-place | 23 +++++++++++--- configs/felhom-mkfs-guarded.sh | 22 ++++--------- configs/test_felhom_internal_place.py | 46 ++++++++++++++++++++++++--- configs/test_felhom_mkfs_guarded.py | 21 +++++++----- internal/storage/partitions.go | 21 +++++++++++- internal/storage/partitions_test.go | 18 +++++++++++ 7 files changed, 120 insertions(+), 33 deletions(-) diff --git a/REUSE.md b/REUSE.md index c8afa7d..cf130ce 100644 --- a/REUSE.md +++ b/REUSE.md @@ -44,7 +44,7 @@ | `storage.IdentityResolver` / `Relation` / `MountFor` | internal/storage/identity.go | `HostPath(path) PlaceIdentity`, `Device(dev) PlaceIdentity`, `Relation(a, b) (different, proven, reason)` | physical-disk identity of a storage place (felhom.eu `16-storage-physical-identity.md` §3, v0.157.0) | Reads /sys + /run/udev/data + /proc/mounts only (no block I/O). Judge a drive ONLY on its own mount (`localapi.ownMountIdentity`) — never the containing fs, or an absent drive reads as the root disk. A shared disk key is "same, proven" even when unknown. Pinned by identity_test.go (fixture trees) + samedisk_test.go | | `localapi.Server.EnsureInternalPlace` / `InternalPlaceRecord` | internal/localapi/internalplace.go | `EnsureInternalPlace(ctx)` (startup + 20 s tick); `ReadInternalPlace` / `WriteInternalPlace` | the one-disk box's internal data place (felhom.eu `16` §5, v0.158.0): mounts it at /mnt/belso by fs-UUID like an enrolled drive, records it `enrolled`; the controller binds + registers it | Never takes /mnt/belso from a household drive (a foreign mount, or a unit naming another UUID, waits). Never formats/ejects (dm on the system disk → role system). The ONLY maker is the root helper `configs/felhom-internal-place` (installer + signed `add_internal_place`). Pinned by internalplace_test.go | | `configs/felhom-internal-place` | configs/ (bundle → /usr/local/sbin) | `plan\|create ` | the ruled size rule (reserve max(16, 10 %) ≤ 48 GiB; ≥ 20 GiB or nothing) + the only lvcreate/mkfs of a system-disk volume | Formats only a felhom-belso with NO signature; refuses a foreign one; never resizes/removes. Sudoers: `^(plan\|create) 0$` only. Pinned by configs/test_felhom_internal_place.py | -| `storage.candidatePartitions` / `CandidatePartition` | internal/storage/partitions.go | `CandidateDisk.Partitions` | partitions as places (`16` §6): empty / filesystem / in_use per part of a free disk | A part with children or a member type is never listed; a raw /mnt mount counts as Felhom's only when corroborated (felhomOwnedMounts). Pinned by partitions_test.go | +| `storage.candidatePartitions` / `CandidatePartition` | internal/storage/partitions.go | `CandidateDisk.Partitions` | partitions as places (`16` §6): empty / filesystem / in_use per part of a free disk; `Formattable` only while no node of the disk is mounted | A part with children or a member type is never listed; a raw /mnt mount counts as Felhom's only when corroborated (felhomOwnedMounts) — for LISTING only: the root mkfs wrapper trusts no such claim (v0.158.1). Pinned by partitions_test.go | | partition wipe id `byid:<…-partN>+fsuuid:` | internal/storage/durable_device.go | `DeviceDurableID` / `ResolveDurableDevice` | a data-bearing PART binds disk id + part number + filesystem (`16` §6) | Both halves must resolve to the SAME device or the wipe refuses. Whole disks keep the plain id. Pinned by partition_durable_test.go | | `ValidateUUID/MountPath/BlockDevice/FSType/SMARTDevice/LVMName`, `UnitNameForMount` | internal/storage/validate.go | `Validate*(v) error` | EVERY arg that reaches a root shell-out | The security boundary; strict whitelists (no by-* symlinks, no dm, no traversal); `systemdEscapePath` computed in-process | | `ValidateNetworkMountSpec` | internal/storage/netmount.go | `ValidateNetworkMountSpec(spec) error` | NAS mount input boundary | Same discipline as validate.go; SMB requires a creds ref; mountpoint confined under `NetworkMountRoot` | diff --git a/configs/felhom-internal-place b/configs/felhom-internal-place index f4a7389..bd85248 100755 --- a/configs/felhom-internal-place +++ b/configs/felhom-internal-place @@ -55,15 +55,19 @@ pool_gib=$(gib_floor "$pool_b") # Every thin volume promised in the pool (its virtual size) — except the place itself and vzdump's short-lived # snapshots (snap_*_vzdump), which exist only during a whole-box copy and are what the reserve is for. -promised_b=0; have_lv=false +# FAIL CLOSED: a failed or empty listing must never read as „nothing promised" (that would size the place too big). +listing=$(lvs --noheadings --nosuffix --units b -o lv_name,pool_lv,lv_size "$VG" 2>/dev/null) || die "cannot list the volumes of $VG" +pool_seen=false; promised_b=0; have_lv=false while read -r name pl size; do [[ -n "${name:-}" ]] || continue + [[ "$name" == "$POOL" ]] && pool_seen=true [[ "$pl" == "$POOL" ]] || continue if [[ "$name" == "$LV_NAME" ]]; then have_lv=true; continue; fi [[ "$name" =~ ^snap_.*_vzdump$ ]] && continue [[ "$size" =~ ^[0-9]+$ ]] || die "cannot read the size of $VG/$name" promised_b=$(( promised_b + size )) -done < <(lvs --noheadings --nosuffix --units b -o lv_name,pool_lv,lv_size "$VG" 2>/dev/null) +done <<< "$listing" +$pool_seen || die "the listing of $VG does not contain the pool $POOL — refusing to guess what is promised" promised_gib=$(( (promised_b + 1073741823) / 1073741824 )) # rounded UP: never promise a partial GiB twice reserve=$(( pool_gib / 10 )); (( reserve < 16 )) && reserve=16; (( reserve > 48 )) && reserve=48 @@ -79,6 +83,17 @@ is_block() { [[ "$(stat -L -c %F "$1" 2>/dev/null)" == "block special file" ]]; fs_uuid() { blkid -p -s UUID -o value "$DEV" 2>/dev/null; } fs_type() { blkid -p -s TYPE -o value "$DEV" 2>/dev/null; } fs_label() { blkid -p -s LABEL -o value "$DEV" 2>/dev/null; } +# has_signature: FAIL CLOSED. blkid -p exits 0 when it found something and 2 when it found NOTHING; any other result (an +# error, an ambivalent probe, blkid missing) must never read as „empty" — that is the only gate in front of mkfs. +has_signature() { + local rc=0 + blkid -p -o value "$DEV" >/dev/null 2>&1 || rc=$? + case $rc in + 0) return 0 ;; + 2) return 1 ;; + *) die "cannot probe $DEV for a signature (blkid exit $rc) — refusing" ;; + esac +} lv_gib() { gib_floor "$(lvs --noheadings --nosuffix --units b -o lv_size "$VG/$LV_NAME" 2>/dev/null | tr -d ' ')"; } if $have_lv; then @@ -89,12 +104,12 @@ if $have_lv; then echo "existing uuid=$(fs_uuid) gib=$(lv_gib)"; exit 0 fi # A felhom-belso with NO signature: a crash between lvcreate and mkfs. Nothing can be on it — format it. - [[ -z "$(blkid -p -o value "$DEV" 2>/dev/null)" ]] || die "$DEV carries a signature blkid cannot name — left alone" + has_signature && die "$DEV carries a signature blkid cannot name — left alone" else (( internal >= MIN_GIB )) || { echo "skipped only $internal GiB free for it (pool $pool_gib, reserve $reserve, promised $promised_gib, planned $PLANNED; needs $MIN_GIB)"; exit 0; } lvcreate -q -y -V "${internal}G" -T "$VG/$POOL" -n "$LV_NAME" >/dev/null || die "lvcreate failed" is_block "$DEV" || die "$DEV did not appear after lvcreate" - [[ -z "$(blkid -p -o value "$DEV" 2>/dev/null)" ]] || die "the new $DEV already carries a signature — left alone" + has_signature && die "the new $DEV already carries a signature — left alone" made=created fi mkfs.ext4 -q -L "$LABEL" "$DEV" >/dev/null || die "mkfs.ext4 failed on $DEV" diff --git a/configs/felhom-mkfs-guarded.sh b/configs/felhom-mkfs-guarded.sh index cf8003b..41ca698 100644 --- a/configs/felhom-mkfs-guarded.sh +++ b/configs/felhom-mkfs-guarded.sh @@ -58,31 +58,23 @@ fi # 3) mounted OUTSIDE Felhom's own drive area = a live foreign filesystem → catastrophic. Mounts under # /mnt/felhom-drives are our own drives (the agent detaches before a re-init) → allowed. -# Agent v0.158.0 (felhom.eu architecture/16 §6, partitions as places): a SIBLING partition that is Felhom's own -# enrolled drive is mounted TWICE — raw at /mnt/ and under /mnt/felhom-drives (R-220). That raw mount is -# allowed only when the same device is ALSO mounted under /mnt/felhom-drives, a state only Felhom's enrolment -# produces. The TARGET itself mounted anywhere outside /mnt/felhom-drives, or any mount elsewhere (/srv, /media, -# /mnt/a/b), still REFUSES. Read from the kernel's mount table, every node of the disk (partitions and children). +# Agent v0.158.0: read from the kernel's mount table for EVERY node of the disk (lsblk's MOUNTPOINT shows one mount +# per node; a bind elsewhere hid behind it), and a disk whose nodes cannot be listed REFUSES (it passed before). +# Deliberately NO exemption for a sibling that is Felhom's own enrolled drive (raw /mnt/ + /mnt/felhom-drives): +# the only corroboration available here is a mount the agent itself can make, so trusting it would let a compromised +# agent unlock a host owner's disk. Consequence (felhom.eu `16` §13): the free part next to an in-use Felhom drive +# cannot be formatted; the dashboard does not offer it. PROC_MOUNTS=/proc/mounts node_mounts() { awk -v d="$1" '$1 == d { print $2 }' "$PROC_MOUNTS"; } disk_nodes="$(lsblk -nrpo NAME "$whole" 2>/dev/null || true)" [[ -n "$disk_nodes" ]] || die "cannot list the nodes of $whole — refusing" while read -r node; do [[ -z "$node" ]] && continue - own=false - while read -r mp; do - case "$mp" in /mnt/felhom-drives/*) own=true ;; esac - done < <(node_mounts "$node") while read -r mp; do [[ -z "$mp" ]] && continue case "$mp" in /mnt/felhom-drives|/mnt/felhom-drives/*) : ;; - *) - if [[ "$node" != "$dev" && "$own" == true && "$mp" =~ ^/mnt/[A-Za-z0-9_][A-Za-z0-9_.-]*$ ]]; then - : # a sibling that is Felhom's own enrolled drive (raw mount + managed mount) - else - die "device (or a partition) is mounted at $mp ($node)" - fi ;; + *) die "device (or a partition) is mounted at $mp ($node)" ;; esac done < <(node_mounts "$node") done <<< "$disk_nodes" diff --git a/configs/test_felhom_internal_place.py b/configs/test_felhom_internal_place.py index ad1aee2..31d6a1b 100644 --- a/configs/test_felhom_internal_place.py +++ b/configs/test_felhom_internal_place.py @@ -14,12 +14,16 @@ GIB = 1 << 30 STUB_LVS = r'''#!/usr/bin/env python3 import json, os, sys st = json.load(open(os.environ["IP_STATE"])) +if st.get("lvs_fail_listing") and "lv_name,pool_lv,lv_size" in sys.argv: sys.exit(5) args = [a for a in sys.argv[1:] if not a.startswith("-") and a not in ("b",)] fields = sys.argv[sys.argv.index("-o") + 1].split(",") target = args[-1] +listing = target == st["vg"] for lv in st["lvs"]: full = st["vg"] + "/" + lv["name"] - if target == st["vg"] or target == full: + if listing and st.get("hide_pool_in_listing") and lv.get("pool", "") == "": + continue # a listing that silently lost the pool row (the pool's own size stays readable) + if listing or target == full: row = {"lv_name": lv["name"], "pool_lv": lv.get("pool", ""), "lv_size": str(lv["size"])} print(" " + " ".join(row[f] for f in fields)) ''' @@ -35,15 +39,18 @@ if st.get("dirty_new"): st["fs"]["felhom-belso"] = {"TYPE": "", "RAW": "junk"} json.dump(st, open(p, "w")) ''' STUB_BLKID = r'''#!/usr/bin/env python3 +# like blkid -p: exit 0 when something is found, 2 when nothing is; st["blkid_rc"] forces an error exit import json, os, sys st = json.load(open(os.environ["IP_STATE"])) +if st.get("blkid_rc"): sys.exit(st["blkid_rc"]) fs = st["fs"].get(os.path.basename(sys.argv[-1]), {}) if "-s" in sys.argv: v = fs.get(sys.argv[sys.argv.index("-s") + 1], "") if v: print(v) -else: - vals = [fs.get(k, "") for k in ("TYPE", "LABEL", "UUID", "RAW") if fs.get(k)] - if vals: print("\n".join(vals)) + sys.exit(0 if v else 2) +vals = [fs.get(k, "") for k in ("TYPE", "LABEL", "UUID", "RAW") if fs.get(k)] +if vals: print("\n".join(vals)) +sys.exit(0 if vals else 2) ''' STUB_MKFS = r'''#!/usr/bin/env python3 import json, os, sys @@ -213,5 +220,36 @@ class Create(unittest.TestCase): b.close() +class FailsClosed(unittest.TestCase): + """A read that fails must never read as „empty" or „nothing promised". Positive control: Create (same setups).""" + + def test_a_failed_volume_listing_creates_nothing(self): + b = Box(349, lvs_fail_listing=True) + try: + rc, out, err = b.run("create", "132") + self.assertEqual(rc, 1, out) + self.assertEqual(b.calls(), []) + finally: + b.close() + + def test_a_listing_without_the_pool_creates_nothing(self): + b = Box(349, [thin("vm-9201-disk-1", 250)], hide_pool_in_listing=True) + try: + self.assertEqual(b.run("create", "132")[0], 1) + self.assertEqual(b.calls(), []) + finally: + b.close() + + def test_a_blkid_error_never_reads_as_empty(self): + for setup in ({"lvs": [thin("felhom-belso", 183)], "fs": {}}, {}): + b = Box(349, **({"lvs": setup["lvs"], "fs": setup["fs"]} if setup else {}), blkid_rc=4) + try: + rc, out, err = b.run("create", "0" if setup else "132") + self.assertEqual(rc, 1, (setup, out)) + self.assertNotIn("mkfs", " ".join(b.calls()), setup) + finally: + b.close() + + if __name__ == "__main__": unittest.main() diff --git a/configs/test_felhom_mkfs_guarded.py b/configs/test_felhom_mkfs_guarded.py index d60566c..13e8acc 100644 --- a/configs/test_felhom_mkfs_guarded.py +++ b/configs/test_felhom_mkfs_guarded.py @@ -1,9 +1,10 @@ #!/usr/bin/env python3 """Tests for configs/felhom-mkfs-guarded.sh — the ONLY mkfs path the agent's sudoers permits. -First tests of this wrapper (agent v0.158.0), written with its change for partitions as places (felhom.eu -architecture/16 §6): a SIBLING partition that is Felhom's own enrolled drive (mounted raw at /mnt/ AND under -/mnt/felhom-drives) no longer blocks formatting the empty partition next to it; every other mount still refuses. +First tests of this wrapper (agent v0.158.0), written for partitions as places (felhom.eu architecture/16 §6). Every +mount of every node of the disk outside /mnt/felhom-drives refuses — INCLUDING a sibling that is Felhom's own drive +(its raw /mnt/ mount): the agent can make that corroborating mount itself, so the root wrapper must not trust +it (security review of 912f7c1). An unlistable disk refuses. HOW IT RUNS ANYWHERE: the wrapper is copied with five literal host paths replaced (each replacement is asserted, so a drifted wrapper fails here instead of silently testing nothing): /proc/mounts → a fake mount table, the block-device @@ -110,8 +111,12 @@ class Partitions(unittest.TestCase): finally: d.close() - def test_the_empty_part_next_to_felhoms_own_drive_is_formatted(self): - self.check(OWN_SDB1) + def test_a_free_part_on_an_unmounted_disk_is_formatted(self): + self.check([]) # positive control for every refusal below + + def test_the_free_part_next_to_felhoms_own_drive_is_refused(self): + # The agent could make the „corroborating" managed mount itself: the root wrapper does not trust it. + self.check(OWN_SDB1, want_ok=False) def test_a_sibling_mounted_raw_only_is_refused(self): self.check([("/dev/sdb1", "/mnt/hdd_1")], want_ok=False) @@ -131,9 +136,9 @@ class Partitions(unittest.TestCase): def test_the_catastrophic_cases_still_refuse(self): self.check([("/dev/sdb1", "/")], want_ok=False) # system disk - self.check(OWN_SDB1, nodes=disk_sdb(sdb1_fstype="LVM2_member"), want_ok=False) # a member signature - self.check(OWN_SDB1, pvs="/dev/sdb1", want_ok=False) # an LVM PV (pvs) - self.check(OWN_SDB1, ro="1", want_ok=False) # read-only + self.check([], nodes=disk_sdb(sdb1_fstype="LVM2_member"), want_ok=False) # a member signature + self.check([], pvs="/dev/sdb1", want_ok=False) # an LVM PV (pvs) + self.check([], ro="1", want_ok=False) # read-only if __name__ == "__main__": diff --git a/internal/storage/partitions.go b/internal/storage/partitions.go index b95fe82..c7c83de 100644 --- a/internal/storage/partitions.go +++ b/internal/storage/partitions.go @@ -13,7 +13,8 @@ import ( // A disk that passed the WHOLE-disk unclaimed filter (classifyClaim: not the system disk, no LVM/ZFS/RAID/LUKS/swap // member on any node, no foreign mount on any node) also lists its partitions, each with its own state: // -// empty no filesystem, no children → „format this part" (self-serve: the agent's probe says blank) +// empty no filesystem, no children → „format this part" (self-serve: the agent's probe says blank) — +// only while NO part of the disk is mounted (Formattable) // filesystem ext4/xfs (mountable) or another fs → „use this part" when mountable; „format this part" only through // the customer-confirmed data-bearing path (the agent's own probe) // in_use already Felhom's own drive → shown, never offered (it is a registered drive already) @@ -32,6 +33,10 @@ type CandidatePartition struct { Mountable bool `json:"mountable"` DurableID string `json:"durable_id,omitempty"` // uuid: — the attach key ("" when empty) MountedAt string `json:"mounted_at,omitempty"` // in_use: Felhom's own mount + // Formattable is false when ANY node of the disk is mounted (another part in use): the root mkfs wrapper refuses + // every mount outside /mnt/felhom-drives on the disk and deliberately does not trust a sibling that claims to be + // Felhom's own (a compromised agent could fake that). Such a part can still be USED (attach erases nothing). + Formattable bool `json:"formattable"` } const ( @@ -55,6 +60,19 @@ func candidatePartitions(lsblkJSON []byte, wholeName string, sizeOf func(string) if d.Name != wholeName { continue } + diskMounted := d.MountPoint != "" + var walk func(n lsblkDev) + walk = func(n lsblkDev) { + if n.MountPoint != "" { + diskMounted = true + } + for _, c := range n.Children { + walk(c) + } + } + for _, c := range d.Children { + walk(c) + } for _, p := range d.Children { if len(p.Children) > 0 || memberFSTypes[p.FSType] { continue // LVM/mapper on it, or a member: not a place (the disk filter refuses these anyway) @@ -74,6 +92,7 @@ func candidatePartitions(lsblkJSON []byte, wholeName string, sizeOf func(string) c.DurableID = "uuid:" + u } } + c.Formattable = c.State != PartInUse && !diskMounted out = append(out, c) } } diff --git a/internal/storage/partitions_test.go b/internal/storage/partitions_test.go index 4c6672f..81cf728 100644 --- a/internal/storage/partitions_test.go +++ b/internal/storage/partitions_test.go @@ -71,3 +71,21 @@ func TestPartitions_UncorroboratedMountIsNotOffered(t *testing.T) { } } } + +// The root wrapper refuses any format on a disk with a mounted node (and trusts no „it is Felhom's own" claim), so a +// part next to a mounted one is never offered for formatting — only for use. Positive control: an unmounted disk. +func TestPartitions_FormattableOnlyWhenNothingOnTheDiskIsMounted(t *testing.T) { + m := listParts(map[string]bool{"/mnt/films": true}) + for dev, p := range m { + if p.Formattable { + t.Fatalf("%s: a part of a disk with a mounted part must not be formattable: %+v", dev, p) + } + } + free := `{"blockdevices":[{"name":"sdc","fstype":null,"mountpoint":null,"children":[ + {"name":"sdc1","fstype":null,"mountpoint":null},{"name":"sdc2","fstype":"ext4","mountpoint":null}]}]}` + got := candidatePartitions([]byte(free), "sdc", func(string) int64 { return 1 << 30 }, func(string) int { return 1 }, + func(string) (string, bool) { return "u", true }, nil) + if len(got) != 2 || !got[0].Formattable || !got[1].Formattable { + t.Fatalf("parts of an unmounted disk are formattable, got %+v", got) + } +}