diff --git a/STATUS.md b/STATUS.md index abda52e..4519fe9 100644 --- a/STATUS.md +++ b/STATUS.md @@ -42,7 +42,9 @@ readable by everything, permanently and unlogged. The **escape** (a protected in name outside its object) is refused unconditionally, with no rule lookup and no blessing exemption; **clobber** and **destroy** are ordinary writes and take the subject's verdict, so `audit` objects keep working. An internal rename short-circuits, which is what keeps -atomic-save alive inside a protected dir. All five programs share `find_object`, so entry is +atomic-save alive inside a protected dir. `inode_mkdir` joins them by classifying off the +*parent*, since a to-be-created dentry is negative and matches nothing itself. Every mutation +program shares `find_object`, so entry is a dentry rather than a `struct file`. A rename **over** an existing name is a fourth case, classified off the destination *dentry* rather than its directory: it destroys that inode and `inode_rename` is the only hook the kernel fires for it, so without it `mv anything secret` @@ -89,22 +91,13 @@ Environment, cwd, tty, job control and session are inherited by construction. **M6**, which are the must-haves. Blessing work waits — B1+B2 already make `bless` usable, and the core boundary is what the remaining holes are in. The rule: *soundness before ergonomics.* -### First — bugs, then finish M3 - -- **The self-test probe is a root-writable `/tmp` path** (security bug, small fix). `cordond` - is uid 0 with no `PrivateTmp=`, and `self_test` uses `create_dir_all` (accepts a - pre-existing directory of any owner) plus `std::fs::write` (no `O_NOFOLLOW`), so a local - user who pre-creates `/tmp/cordon-selftest./` holding a symlink gets root to truncate - any root-owned file — `/etc/cordon/policy.toml` included, after which the daemon fails to - compile and **the boundary is off entirely**. It runs before `seed_policy`, so cordon is not - yet enforcing on itself, and `fs.protected_symlinks` does not cover it. Full analysis and - the fix (move the probe under `/run/cordon/`) in - [PLAN-rule-model](docs/PLAN-rule-model.md) *Carried-over notes*. -- **M3 completion** — `inode_mkdir` and `bprm_check_security` (the `execute` action) are the - two hooks still missing; `listdir` ≈ "open dir for read" needs its coarseness documented; - and `renameat2(RENAME_EXCHANGE)` is **believed, not verified** — the escape refusal for the - protected side moving *out* depends on the kernel calling the hook a second time with the - pair swapped. The four mutation hooks that ship (§15.5) already carry the hard part. +### First — finish M3 + +- **M3 completion** — `bprm_check_security` (the `execute` action) is the one hook still + missing, and it is the first to need a **new BTF offset** (`linux_binprm -> file`); every + hook so far is handed a dentry the existing offsets already cover. `listdir` ≈ "open dir for + read" still needs its coarseness documented. `inode_mkdir` now ships, and + `RENAME_EXCHANGE` is **verified** rather than believed (see *Durable kernel facts*). ### Next — the must-haves @@ -210,6 +203,14 @@ Smaller carried-over notes live in [PLAN-rule-model](docs/PLAN-rule-model.md). supported"). And inlining is not merely a linkage workaround: outlining `emit` added a fifth call frame and the v6.12 verifier rejected the whole object with `E2BIG` and a `mark_precise: frame4` storm. `consult` sits exactly at the five-arg ceiling. +- **`RENAME_EXCHANGE` fires `inode_rename` twice** (verified 2026-07-28 in the VM, not + reasoned from the source): `security_inode_rename` calls the hook once with the pair + **swapped** before the ordinary call. Both directions therefore reach the escape check, + which is the only reason an exchange cannot walk a protected inode out of its object. It + matters *only* where the object allows write — otherwise the destroy or clobber check + refuses first and hides whether the second call happened at all. The `rename_exchange` + scenario is built around that case; do not "simplify" it to a denying object, which would + make it pass without testing anything. - At the mutation hooks the **trusted context argument is a dentry**, not a `file`. The verifier range-checks offsets applied to trusted pointers against `.rodata`, so the `dentry_d_*` offsets must stay real in `maybe_poison_offsets` or the failure-path test diff --git a/crates/bpf-common/src/policy_maps.rs b/crates/bpf-common/src/policy_maps.rs index d77a9f2..6da4e0e 100644 --- a/crates/bpf-common/src/policy_maps.rs +++ b/crates/bpf-common/src/policy_maps.rs @@ -173,7 +173,7 @@ bitflags! { /// `action = ["read","write"]` is a single [`CompiledRule`]; the kernel matches /// when the requested action's bit is set (`intersects`). `READ`/`WRITE`/`CREATE` /// are derivable at `file_open` from `f_mode`/`f_flags`; `LINK`/`RENAME`/`UNLINK`/ - /// `RMDIR` fire at the mutation hooks (§15.5). `EXECUTE`/`LISTDIR`/`MKDIR` still + /// `RMDIR`/`MKDIR` fire at the mutation hooks (§15.5). `EXECUTE`/`LISTDIR` still /// compile but don't fire — their hooks haven't landed. /// /// `#[repr(transparent)]` is load-bearing: it's read byte-for-byte out of the diff --git a/crates/bpf/src/main.rs b/crates/bpf/src/main.rs index cda1ed7..6f64659 100644 --- a/crates/bpf/src/main.rs +++ b/crates/bpf/src/main.rs @@ -176,6 +176,12 @@ pub fn inode_rmdir(ctx: LsmContext) -> i32 { clamped(decide_remove(kernel::Dentry::from_arg(&ctx, 1), Action::RMDIR)) } +/// `inode_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)` +#[lsm(hook = "inode_mkdir")] +pub fn inode_mkdir(ctx: LsmContext) -> i32 { + clamped(decide_create(kernel::Dentry::from_arg(&ctx, 1), Action::MKDIR)) +} + /// Settings + the daemon's own-I/O fast path. `None` ⇒ the caller allows: either we can't /// read settings (fail open) or this *is* the daemon (never deadlock on its own I/O). fn live_bank(pid: Pid) -> Option { @@ -272,6 +278,27 @@ fn decide_remove(dentry: kernel::Dentry, op: Action) -> Option { consult(&matched, Action::WRITE, op, pid, bank) } +/// `inode_mkdir`: a new directory name is appearing inside a (possibly protected) parent. +/// +/// The dentry is **negative** — no inode exists yet — so `find_object` on it would miss on +/// the first step of the walk. The object comes from the parent instead, exactly as the +/// clobber arm of [`decide_reparent`] takes `new_dentry.parent()`. Adding a name inside an +/// object is an ordinary create/write on it: the subject's verdict, honouring `audit`. +/// +/// Gated for symmetry with `inode_rmdir`. Removing a directory inside the object is a write; +/// adding one is too, and leaving it ungated lets a subject that may not touch the object +/// grow structure inside it. Note this is *not* the `rm -rf ~/.ssh && mkdir ~/.ssh` case — +/// that one is closed by `inode_rmdir` refusing the removal, since after the object's own +/// directory is gone there is no protected ancestor left for this hook to find. +fn decide_create(dentry: kernel::Dentry, op: Action) -> Option { + let pid = kernel::current_pid(); + let bank = live_bank(pid)?; + let Some(matched) = find_object(dentry.parent()?, bank) else { + return Some(Verdict::Allow); + }; + consult(&matched, Action::CREATE | Action::WRITE, op, pid, bank) +} + /// Refuse an escape, and journal it best-effort. The verdict deliberately does **not** /// depend on being able to log: an unreadable exe or a full ring buffer costs the /// observation, never the enforcement. diff --git a/crates/daemon/src/backend/bpf_lsm.rs b/crates/daemon/src/backend/bpf_lsm.rs index fec28cb..32ab1ed 100644 --- a/crates/daemon/src/backend/bpf_lsm.rs +++ b/crates/daemon/src/backend/bpf_lsm.rs @@ -55,12 +55,13 @@ use tracing::{error, info, warn}; /// `file_open` decides reads/writes/creates; the four mutation hooks make a *directory* /// object sound, since a directory protects its children only by ancestry and a second /// name (hardlink or rename) is an ancestry that avoids it (DESIGN §15.5). -const HOOKS: [&str; 5] = [ +const HOOKS: &[&str] = &[ "file_open", "inode_link", "inode_rename", "inode_unlink", "inode_rmdir", + "inode_mkdir", ]; /// Test-only env var: forces deliberately wrong field offsets so the self-test's @@ -119,7 +120,7 @@ impl Backend for BpfLsmBackend { // Attach (maps still empty ⇒ allow-all, transient), then PROVE they enforce // before trusting them — a wrong-offset silent no-op must abort loudly. - for hook in HOOKS { + for &hook in HOOKS { let program: &mut Lsm = ebpf .program_mut(hook) .ok_or_else(|| { @@ -388,6 +389,9 @@ fn run_self_test(ebpf: &mut Ebpf, probe: &Path) -> Result<()> { // dentry's own lookup refuses — which is the whole point of the case. Last, because if // it is wrongly allowed it consumes `src`. let clobbered = std::fs::rename(&src, probe.join("over")); + // A *negative* dentry inside the object: `inode_mkdir` classifies off the parent, which + // no other probe here exercises (every other one starts from a positive dentry). + let made = std::fs::create_dir(probe.join("sub")); // Clean up the probe entries (the real seed also clears, but don't rely on it). let _ = seed::protected_remove(ebpf, bank, id); @@ -403,6 +407,7 @@ fn run_self_test(ebpf: &mut Ebpf, probe: &Path) -> Result<()> { "rename over a protected name (inode_rename, destroy)", clobbered, )?; + expect_denied("mkdir inside the object (inode_mkdir)", made)?; Ok(()) } diff --git a/docs/DESIGN.md b/docs/DESIGN.md index bd84276..d494a49 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -828,7 +828,7 @@ restricts only linking files the caller does not own, and here the adversary *is So the invariant is not "gate rename" but **a protected inode must not acquire a name outside its object** — which needs `inode_link` *and* `inode_rename`. -**Shipped (2026-07-27).** Both hooks, plus `inode_unlink`/`inode_rmdir`, share one resolver +**Shipped.** Both hooks, plus `inode_unlink`/`inode_rmdir`/`inode_mkdir`, share one resolver over `find_object`. Four cases. `dst` is the destination **directory** (`new_dentry.parent()`); `target` is the destination **dentry itself**, which is a distinct question — see below: @@ -845,6 +845,12 @@ verdict — gated because objects resolve **once**, at `policy apply`, and nothi them: `rm -rf ~/.ssh && mkdir ~/.ssh` would otherwise dangle the `PROTECTED` entry and leave the path unprotected forever. +`inode_mkdir` is the mirror of `rmdir` and the one hook whose dentry is **negative**: there is +no inode yet, so it matches nothing itself and the verdict comes from its **parent**, like the +clobber arm above. Adding a name inside an object is an ordinary `create`/`write` on it. Note +this does *not* cover the `rm -rf` case above — once the object's own directory is gone there +is no protected ancestor left to find, which is exactly why `rmdir` is the hook that closes it. + **Why escape consults no rule.** An escape publishes the inode to *every* subject, permanently, and silently — afterwards `find_object` misses, so nothing is ever logged again. Gating it on the acting subject's read verdict was considered and rejected: `allow diff --git a/docs/PLAN-rule-model.md b/docs/PLAN-rule-model.md index a07b812..d5c4c5d 100644 --- a/docs/PLAN-rule-model.md +++ b/docs/PLAN-rule-model.md @@ -24,12 +24,14 @@ The milestones are sequenced so each is shippable and VM-validated on its own. Numbering is identity, not schedule. The queue: -1. **Known bugs** — the root-writable `/tmp` self-test probe below (*Carried-over notes*) is a - local escalation that can switch the boundary off; it is small and it goes first. +1. ~~**Known bugs**~~ — **done.** The self-test probe ran in a world-writable `TMPDIR` and + reused any directory it found, so a planted symlink got root to truncate the policy and + switch the boundary off. It lives in root-only `/run/cordon` now. 2. **Docs** — keep DESIGN/STATUS/plan in step with what ships, since these files are the resume point. -3. **Finish M3** — `inode_mkdir`, `bprm_check_security`, and verifying `RENAME_EXCHANGE`. The - hard part (the four mutation hooks) already ships; this closes the set. +3. **Finish M3** ← **here** — `inode_mkdir`, `bprm_check_security`, and verifying + `RENAME_EXCHANGE`. The hard part (the four mutation hooks) already ships; this closes the + set. 4. **M5**, then **M6** — the two must-haves. M5 is small and bounded and removes a fail-**open**; M6 is the largest known hole and the longest job, so it goes last of the two and starts with its userspace-only tasks. @@ -109,7 +111,17 @@ currently unsound — it stops enforcing when the daemon dies, and a grant can b ## Milestone 3 — Mutation + exec hooks ← **finish this first (after the bugs)** - **Done (2026-07-27):** `inode_link`, `inode_rename`, `inode_unlink`, `inode_rmdir` — the second-name escape, clobber, and destroy. Design + rationale in DESIGN §15.5. -- Left: `inode_mkdir` and `bprm_check_security` (the `execute` action). +- **Done (2026-07-28): `inode_mkdir`.** `rmdir` was gated and `mkdir` was not, so a subject + that could not remove a directory inside an object could still add one. The new dentry is + **negative**, so `find_object` on it misses on the first step and the verdict comes from the + *parent* — the same shape as the clobber arm of `decide_reparent`. It is an ordinary + create/write on the parent object, so `audit` still observes rather than blocks. This is not + the `rm -rf ~/.ssh && mkdir ~/.ssh` case: that one is closed by `inode_rmdir` refusing the + removal, since once the object's own directory is gone there is no protected ancestor left + for this hook to find. +- Left: `bprm_check_security` (the `execute` action). Needs a new BTF offset + (`linux_binprm -> file`), unlike every hook so far — the mutation hooks all hand over a + dentry the existing offsets already cover. - `listdir` ≈ "open dir for read" (coarse) — note the limitation; finer (`file_permission`) stays opt-in for cost. - **Done:** gate a positive `new_dentry`. `decide_reparent` classified the source and @@ -118,10 +130,17 @@ currently unsound — it stops enforcing when the daemon dies, and a grant can b `rm secret` is refused. `find_object` now runs on the destination dentry first and takes that object's `write` verdict; the daemon self-test and a `bpf-lsm-smoke.sh` scenario both cover it, the latter also pinning that atomic-save still works. DESIGN §15.5. -- Verify `renameat2(RENAME_EXCHANGE)`. The destroy check gives the destination object a - `write` floor, but an exchange re-parents both inodes rather than destroying one, so the - protected side moving *out* still wants the unconditional escape refusal — which depends on - the kernel calling the hook a second time with the pair swapped. Believed, not verified. +- **Done (2026-07-28): `renameat2(RENAME_EXCHANGE)` verified.** An exchange re-parents both + inodes and destroys neither, so the destroy check's `write` floor does not cover it; the + protected side leaving needs the unconditional escape refusal, which requires the kernel to + ask about *that* side. It does: `security_inode_rename` calls the hook a second time with + the pair swapped when `RENAME_EXCHANGE` is set. Now measured rather than believed, by the + `rename_exchange` smoke scenario. The load-bearing case is an object that **allows write** + and the unprotected name passed *first* — then the first call sees only an ordinary + permitted clobber, and the swapped call is the sole thing standing between the secret and a + name outside its object. The scenario preflights the syscall against two unprotected files, + because an unavailable `renameat2` would otherwise read as a deny and pass for the wrong + reason. An exchange wholly inside one object stays allowed (internal shuffle). ## Milestone 4 — Autolearn (the zones layer) ← **not scheduled; its machinery lands as M6 task 6** - Separate `learned.db` (SQLite); `zones` config = where learning may propose rules. @@ -548,25 +567,6 @@ cookie-DB measurement, not on any of the above. ## Carried-over notes (still live) Loose ends left by shipped work, none of them a milestone of their own: -- **The self-test probe is a root-writable `/tmp` path (security bug, small fix).** ← **first - in the queue** — it is the one item here that is a live escalation rather than a rough edge. - `self_test` builds `$TMPDIR/cordon-selftest.` and now seeds a file inside it with - `std::fs::write`, which is `O_WRONLY|O_CREAT|O_TRUNC` with no `O_NOFOLLOW` — and - `create_dir_all` accepts a pre-existing directory of any owner. `cordond` is uid 0 and no - unit sets `PrivateTmp=`, so a local user who pre-creates `/tmp/cordon-selftest./` - (mode 0755, theirs) holding `src -> /etc/cordon/policy.toml` gets root to truncate the - policy on the next start with that pid — after which the daemon fails to compile and the - boundary is off entirely. Any root-owned file works; the pid guess is the only cost, and - pids are sequential. `fs.protected_symlinks` does **not** cover this: `may_follow_link()` - keys on the symlink's *immediate parent* being sticky and world-writable, and here that - parent is the attacker's own plain directory, so the check returns before comparing uids. - (Planting the top-level probe path itself as a symlink *is* blocked — that one sits - directly in sticky `/tmp`.) The write also runs before `seed_policy`, so cordon is not yet - enforcing on itself. Fix: put the probe under `/run/cordon/` (root-only, removes the shared - namespace outright); if it must stay in `TMPDIR`, use `std::fs::create_dir` so a planted - directory fails `EEXIST` and open the seed file `O_NOFOLLOW|O_EXCL` via - `OpenOptionsExt::custom_flags`. `PrivateTmp=yes` on the unit is worth adding regardless, but - as defence in depth — it misses the dev unit and any non-systemd start. - **`MAX_RULES_PER_OBJ = 32`** — raise toward 64 if a verifier log demands it; `cordon policy check` errors when a compiled bucket exceeds it. - **Per-object default deferred.** A protected object with no matching rule falls to the diff --git a/packaging/test/bpf-lsm-smoke.sh b/packaging/test/bpf-lsm-smoke.sh index 2e4edb2..b69f25e 100755 --- a/packaging/test/bpf-lsm-smoke.sh +++ b/packaging/test/bpf-lsm-smoke.sh @@ -1630,12 +1630,108 @@ EOF daemon_stop; return $fail } +# --- scenario: inode_mkdir --------------------------------------------------- +# `rmdir` was gated and `mkdir` was not, so a subject that could not remove a directory +# inside an object could still add one. The new dentry is negative, so the verdict comes +# from the *parent* — which is what this checks, on both polarities, in one policy. +scenario_mkdir() { + echo "### scenario: mkdir inside a protected object takes the parent's verdict" + ws_new + mkdir -p "$VAULT/locked" + write_policy </dev/null || { echo " SKIP: python3 unavailable (needed for renameat2)"; return 0; } + local outside="$TMP/outside"; echo "OUTSIDE" > "$outside" + # Preflight while nothing is enforcing: an unavailable syscall must not read as a deny. + echo A > "$TMP/xa"; echo B > "$TMP/xb" + if ! exchange "$TMP/xa" "$TMP/xb"; then + echo " SKIP: renameat2(RENAME_EXCHANGE) unavailable on this kernel/libc"; return 0 + fi + grep -q B "$TMP/xa" || { echo " FAIL: preflight exchange did not swap the inodes"; return 1; } + # Deliberately permissive: `write` must be *allowed* on the vault, or the destroy and + # clobber checks would refuse the exchange for an unrelated reason and prove nothing. + write_policy <