diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ac2aa9..d3e6428 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ Design: `felhom.eu/documentation/architecture/03-host-agent.md` §3.1 (new). Mea WireGuard config and the OOB sshd config + felhom-op key reach their root-read places only through it: fixed source, fixed destination, CONTENT checked against what the agent's renderers write (no `[Service]`, `Where=` only `/mnt/` or `/mnt/felhom-drives/` and equal to the unit name, no `bind`/`suid`; a network share must carry - `nosuid,nodev`; no `dhcp-script=`; no `PostUp=`; the sshd config only the one template with its Port). Its 31 tests + `nosuid,nodev`; no `dhcp-script=`; no `PostUp=`; the sshd config only the one template with its Port). Its 30 tests (`configs/test_felhom_priv_apply.py`) + Go contract tests feeding each renderer's real output (`internal/privapplytest`). Pre-flight: every live file on both demo boxes reads OK. - **NFS/SMB options gain `nosuid,nodev`** (a set-uid file on a server outside the box never acts on the host). diff --git a/configs/felhom-os-apply b/configs/felhom-os-apply index 6da4a7b..4606ff6 100755 --- a/configs/felhom-os-apply +++ b/configs/felhom-os-apply @@ -113,6 +113,9 @@ CRASH_GUARD_STATE = "/var/lib/felhom-crash-guard/state.json" SELFUPDATE_OP = "agent_update" SELFUPDATE_DIR = "/var/lib/felhom-agent/selfupdate" SELFUPDATE_WRAPPER = "/usr/local/sbin/felhom-selfupdate-guarded" +# The verified bytes are written HERE (a root-owned directory the agent cannot write) and only this copy reaches the A/B wrapper — never the agent's file. +SELFUPDATE_ROOT_DIR = "/var/lib/felhom-os-apply/agent-update" +SELFUPDATE_MAX_BYTES = 256 * 1024 * 1024 BUNDLE_FORMAT = 1 BUNDLE_OP = "agent_config_update" BUNDLE_RECORD = "/etc/felhom/config-bundle.json" # what the box runs (0644 root; the non-root agent reports it) @@ -289,6 +292,30 @@ class Runner: except Exception: pass + def read_staged_once(self, path, owner_uid, limit): + """R-861: read a file the AGENT staged ONCE, as root, safely: O_NOFOLLOW (the last component may not be a + symlink), fstat on the opened fd (a regular file owned by owner_uid), at most `limit` bytes. The caller hashes + and uses exactly these bytes — never the path again (the agent owns the directory and could swap the file).""" + fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW | os.O_CLOEXEC) + try: + st = os.fstat(fd) + if not stat.S_ISREG(st.st_mode) or st.st_uid != owner_uid: + raise Refused("R19", f"{path} is not a regular file owned by {AGENT_USER}") + if st.st_size > limit: + raise Refused("R19", f"{path} is larger than {limit} bytes") + chunks, n = [], 0 + while True: + b = os.read(fd, 1 << 20) + if not b: + break + chunks.append(b) + n += len(b) + if n > limit: + raise Refused("R19", f"{path} grew past {limit} bytes while it was read") + return b"".join(chunks) + finally: + os.close(fd) + # ---------- host files, for the config bundle (R-840). Tests replace these with an in-memory tree. ---------- def read_bytes(self, path): with open(path, "rb") as f: @@ -547,17 +574,26 @@ class Apply: staged = os.path.join(SELFUPDATE_DIR, "felhom-agent-" + ver) if plan.get("staged") != staged: raise Refused("R19", f"the staged binary must be {staged}, got {plan.get('staged')!r}") + # ONE read, then never the agent's path again: hash exactly these bytes and hand the A/B wrapper a ROOT-OWNED + # copy of them. Hashing the agent's file and then letting the wrapper copy it by path was a race — the agent owns + # that directory and could swap the file between the check and the copy (found by review 2026-10-05). try: - st = self.r.stat(staged) + data = self.r.read_staged_once(staged, self.r.agent_uid(), SELFUPDATE_MAX_BYTES) except OSError as e: - raise Refused("R19", f"cannot stat the staged binary: {e}") - if not stat.S_ISREG(st.st_mode) or st.st_uid != self.r.agent_uid(): - raise Refused("R19", "the staged binary is not a regular file owned by the agent") - got = sha256_hex(self.r.read_bytes(staged)) + raise Refused("R19", f"cannot read the staged binary: {e}") + got = sha256_hex(data) if got != sha: raise Refused("R19", f"the staged binary's sha256 {got[:16]}… is not the signed {sha[:16]}…") - self.r.log(f"os-apply: AGENT-UPDATE signed by the operator: version={ver} sha={sha[:16]} — handing to the A/B wrapper") - rc, out, err = self.r.host([SELFUPDATE_WRAPPER, "apply", staged, sha], 120) + root_copy = os.path.join(SELFUPDATE_ROOT_DIR, "felhom-agent-" + ver) + self.r.put_file(root_copy, data, 0o755) + self.r.log(f"os-apply: AGENT-UPDATE signed by the operator: version={ver} sha={sha[:16]} — handing the root copy to the A/B wrapper") + try: + rc, out, err = self.r.host([SELFUPDATE_WRAPPER, "apply", root_copy, sha], 120) + finally: + try: + self.r.remove(root_copy) + except OSError: + pass self.report["agent_update"] = {"version": ver, "sha256": sha, "wrapper_rc": rc, "wrapper": (out + err).strip()[-300:]} if rc != 0: diff --git a/configs/felhom-priv-apply b/configs/felhom-priv-apply index 3fcdb86..7183b08 100755 --- a/configs/felhom-priv-apply +++ b/configs/felhom-priv-apply @@ -55,7 +55,6 @@ NET_TYPES = {"nfs", "nfs4", "cifs"} # Options that turn a device mount into something else, or let set-uid/device files act on the host. FORBIDDEN_OPTS = {"bind", "rbind", "move", "rmove", "remount", "suid", "dev", "user", "users", "owner", "group", "x-mount.mkdir", "helper"} -UNIT_TOKEN_RE = re.compile(r"^[A-Za-z0-9@_.\\:-]+$") DESC_RE = re.compile(r"^[^\x00-\x1f\x7f]{0,200}$") WG_KEY_RE = re.compile(r"^[A-Za-z0-9+/]{42}[AEIMQUYcgkosw480]=$") KEY_LINE_RE = re.compile(r"^(ssh-ed25519|ssh-rsa|ecdsa-sha2-nistp(256|384|521)|sk-ssh-ed25519@openssh\.com) " @@ -164,6 +163,10 @@ def parse_ini(text, what): sections, cur = {}, None for n, raw in enumerate(text.split("\n"), 1): line = raw.strip() + if line.endswith("\\"): + # systemd joins a line ending in a backslash with the next one; this parser does not. Refused, so the two + # can never read the same bytes differently (review 2026-10-05). + raise Refused("U2", f"{what}: line {n} ends with a backslash (a continuation)") if not line or line.startswith("#") or line.startswith(";"): continue m = re.match(r"^\[([A-Za-z]+)\]$", line) @@ -190,7 +193,10 @@ def check_unit(name, text): kind = "automount" if name.endswith(".automount") else "mount" s = parse_ini(text, name) body = "Automount" if kind == "automount" else "Mount" - allowed = {"Unit": {"Description", "After", "Before", "Wants", "Requires"}, + # [Unit] holds ONLY what the renderers write: Description, and After=local-fs-pre.target on a local mount. A + # Wants=/Requires=/Before= naming any unit would start it with the mount (Wants=reboot.target — found by review + # 2026-10-05), so none of them is accepted. + allowed = {"Unit": {"Description", "After"}, body: {"Where", "TimeoutIdleSec"} if kind == "automount" else {"What", "Where", "Type", "Options"}, "Install": {"WantedBy"}} for sec, keys in s.items(): @@ -202,9 +208,8 @@ def check_unit(name, text): u = s.get("Unit", {}) if not DESC_RE.match(u.get("Description", "")): raise Refused("U2", f"{name}: Description has control characters") - for k in ("After", "Before", "Wants", "Requires"): - if k in u and not all(UNIT_TOKEN_RE.match(t) for t in u[k].split()): - raise Refused("U2", f"{name}: {k}= names something that is not a unit") + if "After" in u and u["After"] != "local-fs-pre.target": + raise Refused("U2", f"{name}: After= may only be local-fs-pre.target") inst = s.get("Install", {}) if inst and inst.get("WantedBy") != "multi-user.target": raise Refused("U2", f"{name}: WantedBy must be multi-user.target") diff --git a/configs/felhom-selfupdate-guarded b/configs/felhom-selfupdate-guarded index c41583d..a6b0703 100644 --- a/configs/felhom-selfupdate-guarded +++ b/configs/felhom-selfupdate-guarded @@ -24,6 +24,11 @@ set -u BIN=/usr/local/bin/felhom-agent PREV=$BIN.prev STAGING=/var/lib/felhom-agent/selfupdate +# R-861 (agent v0.146.1): `apply` takes ONLY the root-owned copy felhom-os-apply writes after it has verified the +# operator's signature and hashed exactly those bytes (mode agent_update). The agent cannot call `apply` any more (it +# left the sudoers), and the agent's own staging dir is no longer accepted: a file in a directory the agent owns can be +# swapped between this script's sha check and its copy. +ROOT_STAGING=/var/lib/felhom-os-apply/agent-update PENDING=$STAGING/pending.json UNIT=felhom-agent.service @@ -42,11 +47,14 @@ apply) log "refusing apply: usage: apply " exit 2 fi - # Root-side path confinement: the staged binary MUST live in the agent's staging dir. + # Root-side path confinement: the staged binary MUST be felhom-os-apply's root-owned copy (R-861). case "$staged" in - "$STAGING"/*) ;; - *) log "refusing apply: staged path outside $STAGING: $staged"; exit 1 ;; + "$ROOT_STAGING"/*) ;; + *) log "refusing apply: staged path outside $ROOT_STAGING: $staged"; exit 1 ;; esac + if [ -L "$staged" ] || [ "$(stat -c %u "$staged" 2>/dev/null)" != "0" ]; then + log "refusing apply: $staged is a symlink or not root-owned"; exit 1 + fi case "$staged" in *..*) log "refusing apply: staged path contains '..'"; exit 1 ;; esac diff --git a/configs/test_felhom_config_bundle.py b/configs/test_felhom_config_bundle.py index d19c2c1..4ac6ac9 100644 --- a/configs/test_felhom_config_bundle.py +++ b/configs/test_felhom_config_bundle.py @@ -94,6 +94,14 @@ class Box: raise OSError("no such file") return self.files[p] + def read_staged_once(self, p, owner_uid, limit): + if p not in self.files: + raise OSError("no such file") + if self.uids[p] != owner_uid: + raise osapply.Refused("R19", f"{p} is not a regular file owned by felhom-agent") + self.staged_reads = getattr(self, "staged_reads", 0) + 1 + return self.files[p] + def stat(self, p): if p not in self.files: raise OSError("no such file") @@ -577,10 +585,23 @@ class AgentUpdate(unittest.TestCase): box = update_box(update_job(sha)) rc, rep = run(box) self.assertEqual(rc, 0, rep) - self.assertEqual(wrapper_calls(box), [[osapply.SELFUPDATE_WRAPPER, "apply", STAGED, sha]]) + root_copy = osapply.SELFUPDATE_ROOT_DIR + "/felhom-agent-0.146.0" + # the wrapper gets the ROOT-OWNED copy of the bytes that were hashed — never the agent's path (review 2026-10-05) + self.assertEqual(wrapper_calls(box), [[osapply.SELFUPDATE_WRAPPER, "apply", root_copy, sha]]) + self.assertIn(root_copy, box.writes) + self.assertNotIn(root_copy, box.files, "the root copy is removed after the flip") + self.assertEqual(box.staged_reads, 1, "the agent's file is read exactly once") self.assertIn("u1", box.nonces) self.assertEqual(rep["agent_update"]["version"], "0.146.0") + def test_a_staged_file_the_agent_does_not_own_is_refused(self): + sha = hashlib.sha256(NEW_BIN).hexdigest() + box = update_box(update_job(sha)) + box.uids[STAGED] = 0 + rc, rep = run(box) + self.assertEqual((rc, rep["refused"]["code"]), (2, "R19"), rep) + self.assertEqual(wrapper_calls(box), []) + def test_a_bad_signature_never_reaches_the_wrapper(self): sha = hashlib.sha256(NEW_BIN).hexdigest() box = update_box(update_job(sha)) @@ -630,5 +651,21 @@ class AgentUpdate(unittest.TestCase): self.assertNotIn("u1", box.nonces) + +class SelfupdateWrapperConfinement(unittest.TestCase): + """R-861 (v0.146.1): the A/B wrapper takes only felhom-os-apply's root-owned copy, never the agent's staging dir + (a file there can be swapped between the wrapper's sha check and its copy). The path check runs before anything + is touched, so the real script can be run here unprivileged. + RED-PROOF: point ROOT_STAGING back at /var/lib/felhom-agent/selfupdate → this fails (the path is accepted and the + script goes on to `staged file missing`).""" + + def test_the_agents_staging_dir_is_refused(self): + import subprocess + sha = "0" * 64 + p = subprocess.run(["sh", str(HERE / "felhom-selfupdate-guarded"), "apply", + "/var/lib/felhom-agent/selfupdate/felhom-agent-0.146.1", sha], capture_output=True, text=True) + self.assertEqual(p.returncode, 1, p.stderr) + self.assertIn("outside /var/lib/felhom-os-apply/agent-update", p.stderr) + if __name__ == "__main__": unittest.main() diff --git a/configs/test_felhom_priv_apply.py b/configs/test_felhom_priv_apply.py index 2a2a9e4..b00b9e6 100644 --- a/configs/test_felhom_priv_apply.py +++ b/configs/test_felhom_priv_apply.py @@ -204,6 +204,17 @@ class Refuses(unittest.TestCase): def test_U2_service_section(self): self.refused(*self.unit(LOCAL_UNIT + "\n[Service]\nExecStart=/bin/sh -c id\n"), "U2") + def test_U2_wants_starts_another_unit(self): # review 2026-10-05: Wants=reboot.target would reboot the host + for extra in ("Wants=reboot.target", "Requires=felhom-agent-rollback.service", "Before=pve-guests.service"): + self.refused(*self.unit(LOCAL_UNIT.replace("After=local-fs-pre.target", "After=local-fs-pre.target\n" + extra)), "U2") + self.refused(*self.unit(LOCAL_UNIT.replace("After=local-fs-pre.target", "After=poweroff.target")), "U2") + + def test_U2_continuation_line(self): + t = LOCAL_UNIT.replace("Description=Felhom storage mount 91d2dc2d-2d28-4929-9bdd-3e11fa2f41ae", + "Description=Felhom storage mount \\") + self.refused(*self.unit(t), "U2") + self.refused(*self.unit(LOCAL_UNIT.replace("# Managed by felhom-agent", "# comment \\\n# Managed by felhom-agent")), "U2") + def test_U2_unknown_key(self): self.refused(*self.unit(LOCAL_UNIT.replace("Type=ext4", "Type=ext4\nDirectoryMode=0777")), "U2") diff --git a/internal/escrow/identity.go b/internal/escrow/identity.go index d50946a..93e4b61 100644 --- a/internal/escrow/identity.go +++ b/internal/escrow/identity.go @@ -184,15 +184,35 @@ func UnwrapIdentityBundle(ctx context.Context, blob []byte, recoveryCode string) } // readStagedNoFollow reads a file the AGENT staged, for the escrow ceremony that runs as ROOT (FELHOM_ESCROW). R-861 -// (agent v0.146.0): both files live in the agent's own directory, so a compromised agent could put a SYMLINK there -// (to any root-only file) and the root ceremony would seal that file into the blob and hand the agent R — a root file -// read. So: no symlink (O_NOFOLLOW), a regular file, at most 4 KiB. A missing file keeps its os.IsNotExist meaning. -// Pinned by TestAttach_RefusesASymlink. +// (agent v0.146.0/0.146.1): both files live in the agent's own directory, so a compromised agent could put a SYMLINK +// there — at the file OR at any directory on the way (review 2026-10-05) — to a root-only file, and the root ceremony +// would seal that file into the blob and hand the agent R. So the path is walked from "/" one component at a time with +// openat(O_NOFOLLOW): no symlink anywhere, the last a regular file of at most 4 KiB. Once a directory is open, renaming +// it does not redirect the walk. A missing file keeps its os.IsNotExist meaning. Pinned by TestAttach_RefusesASymlink*. func readStagedNoFollow(path string) ([]byte, error) { - f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW, 0) + if !filepath.IsAbs(path) { + return nil, fmt.Errorf("%s is not an absolute path", path) + } + clean := filepath.Clean(path) + parts := strings.Split(strings.TrimPrefix(clean, "/"), "/") + dirfd, err := syscall.Open("/", syscall.O_RDONLY|syscall.O_DIRECTORY|syscall.O_CLOEXEC, 0) if err != nil { return nil, err } + for i, part := range parts { + last := i == len(parts)-1 + flags := syscall.O_RDONLY | syscall.O_NOFOLLOW | syscall.O_CLOEXEC + if !last { + flags |= syscall.O_DIRECTORY + } + fd, err := syscall.Openat(dirfd, part, flags, 0) + syscall.Close(dirfd) + if err != nil { + return nil, &os.PathError{Op: "open", Path: clean, Err: err} + } + dirfd = fd + } + f := os.NewFile(uintptr(dirfd), clean) defer f.Close() fi, err := f.Stat() if err != nil { diff --git a/internal/escrow/r861_staged_read_test.go b/internal/escrow/r861_staged_read_test.go index 8cf6604..07364ef 100644 --- a/internal/escrow/r861_staged_read_test.go +++ b/internal/escrow/r861_staged_read_test.go @@ -36,3 +36,28 @@ func TestAttach_RefusesASymlink(t *testing.T) { t.Fatalf("control: a missing file must stay a clean no-attach: %v %v", ok, err) } } + +// Review 2026-10-05: a symlinked DIRECTORY on the way must stop the read too (O_NOFOLLOW alone guards only the last +// component). RED-PROOF: open the full path with O_NOFOLLOW only → this fails. +func TestAttach_RefusesASymlinkedDirectory(t *testing.T) { + d := t.TempDir() + secretDir := filepath.Join(d, "root-only-dir") + _ = os.Mkdir(secretDir, 0o700) + _ = os.WriteFile(filepath.Join(secretDir, "private.key"), []byte("AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=\n"), 0o600) + agentDir := filepath.Join(d, "agent") + _ = os.Mkdir(agentDir, 0o700) + if err := os.Symlink(secretDir, filepath.Join(agentDir, "wg")); err != nil { + t.Fatal(err) + } + var b IdentityBundle + if ok, err := AttachWGKey(&b, filepath.Join(agentDir, "wg", "private.key")); err == nil || ok || b.WGPrivateKey != "" { + t.Fatalf("a key behind a symlinked directory was read: ok=%v err=%v", ok, err) + } + // control: the same key under a REAL directory is read + _ = os.Remove(filepath.Join(agentDir, "wg")) + _ = os.Mkdir(filepath.Join(agentDir, "wg"), 0o700) + _ = os.WriteFile(filepath.Join(agentDir, "wg", "private.key"), []byte("AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=\n"), 0o600) + if ok, err := AttachWGKey(&b, filepath.Join(agentDir, "wg", "private.key")); err != nil || !ok { + t.Fatalf("control: a key under a real directory was not read: %v %v", ok, err) + } +}