From e1c841bcc6c9e7482126a00588be74645a421563 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Sun, 6 Sep 2026 12:00:36 +0200 Subject: [PATCH 1/4] feat(bench): a throwaway VM, so the firewall can be measured while armed scripts/bench-latency.sh has been in the repo since 0.4.0 and could not answer the question it exists for. The comparison that matters needs CFC armed - the fail-closed queue rule loaded, the daemon deciding, the fast path granting - and arming that on a development machine takes its network with it if anything is wrong. scripts/vm-bench boots a guest for one run instead. It assembles an initramfs out of this machine's own kernel modules, nftables, iproute2, python3 and the release binaries, boots it under KVM, and runs the bench there against a real daemon. Nothing is downloaded, nothing outside target/vm-bench is written, and the guest exists for one boot. One sudo, once, to read the host's kernel image. The states differ from each other in one thing at a time, so each difference isolates one cost: no firewall, the queue, the queue with a daemon built with a shorter idle beat, the fast path. ALT_DAEMON carries that second daemon into the same image, so a constant can be attributed by changing it and measuring both in one boot rather than by reasoning about a distribution. Two facts are recorded beside every measurement rather than assumed: what `cfc status` says the fast path is, and `id_sequence` from /proc/net/netfilter/nfnetlink_queue - one increment per packet the kernel really handed to userspace. A state calling itself `fast` whose queue saw one packet per connect did not take the fast path, and no latency figure says that on its own. Both directions run everywhere, because `in` never meets the queue and so measures the eBPF hooks alone. Three defects of its own were found by running it, all of the class this repo keeps meeting: a pipeline whose legitimate empty answer killed the build under pipefail (a module this kernel builds in rather than builds), a `/lib` created as a directory before being made a symlink, and `modprobe --show-depends` silently omitting nf_conntrack because Arch ships an install directive for it - which booted a guest with no conntrack and refused `ct state new queue num 0` with a bare ENOENT. --- scripts/vm-bench/README.md | 83 ++++++++++++++++++ scripts/vm-bench/build-guest.sh | 127 ++++++++++++++++++++++++++++ scripts/vm-bench/init | 60 +++++++++++++ scripts/vm-bench/plan.sh | 144 ++++++++++++++++++++++++++++++++ scripts/vm-bench/report.py | 77 +++++++++++++++++ scripts/vm-bench/run.sh | 58 +++++++++++++ 6 files changed, 549 insertions(+) create mode 100644 scripts/vm-bench/README.md create mode 100755 scripts/vm-bench/build-guest.sh create mode 100755 scripts/vm-bench/init create mode 100755 scripts/vm-bench/plan.sh create mode 100755 scripts/vm-bench/report.py create mode 100755 scripts/vm-bench/run.sh diff --git a/scripts/vm-bench/README.md b/scripts/vm-bench/README.md new file mode 100644 index 0000000..b5436ce --- /dev/null +++ b/scripts/vm-bench/README.md @@ -0,0 +1,83 @@ +# Measuring what the firewall costs, on a machine it may arm + +`scripts/bench-latency.sh` measures connect latency over a veth pair. It +answers nothing on its own, because the interesting comparison needs CFC +*armed* - the queue rule loaded, the daemon deciding, the fast path granting - +and arming a fail-closed firewall on a development machine has consequences. + +This directory boots a throwaway VM instead. It assembles an initramfs from the +host's own kernel modules, `nftables`, `iproute2`, `python3` and the release +binaries in `target/`, boots it under KVM, and runs the bench there against a +real daemon. Nothing is downloaded; nothing outside `target/vm-bench` is +written; the guest exists for one boot. + +```sh +cargo build --release -p cfc-daemon -p cfc-cli +cargo xtask build-ebpf +./scripts/vm-bench/run.sh +``` + +One `sudo` is needed, once, to read the host's kernel image. `KERNEL`, `OUT`, +`MEM`, `SMP` and `TIMEOUT` override the defaults. + +## What it measures, and why each state exists + +Every state differs from its neighbour in exactly one thing, so each difference +isolates one cost. + +| state | what is running | what the difference against the previous one buys | +|---|---|---| +| `floor` | nothing: no daemon, no table | the veth link and `connect()` itself | +| `queue-N` | the daemon, the table, a lasting Allow, `fast_allow = false` | the NFQUEUE round trip, at N flows | +| `poll200us-N` | the same, with a daemon built with a shorter `RECV_POLL_INTERVAL` | how much of that round trip is the worker's idle beat | +| `fast-N` | `fast_allow = true`, the client covered by a lasting Allow | the fast path against the queue | + +Both directions run in every state and they answer different questions. `out` +leaves through the host's output chain and meets the queue. `in` is generated +inside the network namespace, whose own output chain carries no colony table, +so it never meets a queue - but its client sits in the root cgroup and still +runs the connect hooks, which makes `in` the cost of the eBPF layer alone. + +Two things are recorded beside every measurement rather than assumed: +`cfc status`'s own account of the fast path, and `id_sequence` from +`/proc/net/netfilter/nfnetlink_queue` - one increment per packet the kernel +actually handed to userspace. A state calling itself `fast` whose queue saw one +packet per connect did not take the fast path, and no latency figure says that +on its own. + +`ALT_DAEMON=/path/to/colony-firewalld` carries a second daemon into the same +image, measured in the same boot under conditions that differ in nothing else. +That is how the `poll200us` row is produced: build one, point at it, and the +constant under test is the only variable. + +## What it found + +Run on 2026-09-06, Linux 7.2.2, KVM, four vCPUs, 3000 flows unless said. + +| state | 300 flows | 3000 flows | +|---|---|---| +| no firewall | 0.0158 ms | 0.0162 ms | +| fast path | 0.0268 ms | 0.0269 ms | +| queue, 200 us idle beat | 0.7703 ms | 2.3646 ms | +| queue, the shipped 5 ms beat | 5.6745 ms | 7.6083 ms | + +Read across, and three things fall out. + +- **The fast path saves 5.6 ms per new flow at 300 flows and 7.6 ms at 3000**, + and costs 0.011 ms over having no firewall at all. Its own cost does not grow + with load, because those flows never reach the daemon. +- **A full `RECV_POLL_INTERVAL` is paid per queued flow, not half of one.** + `crates/cfc-daemon/src/nfqueue.rs` predicts "up to one interval (mean: half + that)", which is right for random arrivals and wrong for a client that + connects in series: every connect lands just after the worker committed to a + fresh idle wait, so it waits the whole beat. Measured by changing the + constant, not by reading the shape of a distribution: 4.90 ms of the 5.67 at + 300 flows, 5.24 ms of the 7.61 at 3000. +- **What is left grows with the number of live sockets** - 0.77 ms at 300 flows + against 2.36 ms at 3000, with the beat removed. That growth is in the daemon's + per-packet work, not the kernel's: the `floor` state moved 0.0003 ms across + the same range. + +The absolute numbers are this VM's, not a bare-metal host's. What transfers is +that every state was measured in the same guest, back to back, with one +variable moving at a time. diff --git a/scripts/vm-bench/build-guest.sh b/scripts/vm-bench/build-guest.sh new file mode 100755 index 0000000..aff85b5 --- /dev/null +++ b/scripts/vm-bench/build-guest.sh @@ -0,0 +1,127 @@ +#!/usr/bin/env bash +# Assemble a minimal initramfs that runs Colony Firewall Control for real: +# this machine's own kernel modules, nftables, iproute2, python3 and the +# release binaries. There is no package manager and no network in the guest, +# so everything the daemon or the bench touches has to be put here. +set -euo pipefail + +REPO="${REPO:?REPO must point at the checkout}" +OUT="${OUT:?OUT must point at the work directory}" +KREL="$(uname -r)" +RFS="$OUT/rootfs" + +rm -rf "$RFS" +# The usr-merge symlinks FIRST. Creating /lib as a directory and turning it +# into a symlink afterwards does not work - ln would drop the link inside it. +mkdir -p "$RFS"/usr/bin "$RFS"/usr/lib +ln -s usr/bin "$RFS/bin"; ln -s usr/bin "$RFS/sbin" +ln -s bin "$RFS/usr/sbin" +ln -s usr/lib "$RFS/lib"; ln -s usr/lib "$RFS/lib64" +mkdir -p "$RFS"/{proc,sys,tmp,dev,run,root,bench,etc} \ + "$RFS/usr/lib/modules/$KREL" "$RFS"/sys/fs/{cgroup,bpf} "$RFS"/sys/kernel/tracing \ + "$RFS"/etc/colony-firewall "$RFS"/var/{lib,log} \ + "$RFS"/var/lib/colony-firewall "$RFS"/var/log/colony-firewall +ln -s ../run "$RFS/var/run" + +copy_file() { # keeps the path, dereferences: every name the loader may ask + local src="$1" dst="$RFS$1" + [ -e "$src" ] || return 1 + [ -e "$dst" ] && return 0 + mkdir -p "$(dirname "$dst")" + cp -aL "$src" "$dst" 2>/dev/null || return 1 +} + +missing="" +copy_bin() { + local name="$1" path real lib rl + path="$(command -v "$name" 2>/dev/null || true)" + [ -n "$path" ] || path="$name" + [ -x "$path" ] || { missing="$missing $name"; return 0; } + real="$(readlink -f "$path")" + copy_file "$real" || true + [ "$real" != "$path" ] && { copy_file "$path" || true; } + for lib in $(ldd "$real" 2>/dev/null | grep -oE '/[^ ]+\.so[^ ]*' || true); do + copy_file "$lib" || true + rl="$(readlink -f "$lib")" + [ "$rl" != "$lib" ] && { copy_file "$rl" || true; } + done + return 0 +} + +echo "==> binaries" +for b in bash mount umount mkdir rmdir rm cp mv ln cat sleep seq paste cut tr sort uniq \ + head tail wc grep sed gawk env readlink dirname basename chmod chown \ + stat date ip ss nft python3 insmod id find xargs touch sync ls \ + uname hostname mktemp tee expr comm sha256sum install printf sleep; do + copy_bin "$b" +done +[ -n "$missing" ] && echo " missing:$missing" +install -m0755 "$REPO/target/release/colony-firewalld" "$RFS/usr/bin/colony-firewalld" +install -m0755 "$REPO/target/release/cfc" "$RFS/usr/bin/cfc" +# Optional: the same daemon built with a different RECV_POLL_INTERVAL. Both in +# one image so the two can be measured in a single boot, under conditions that +# differ in nothing else. +if [ -x "${ALT_DAEMON:-/nonexistent}" ]; then + install -m0755 "$ALT_DAEMON" "$RFS/usr/bin/colony-firewalld-alt" + echo " plus an alternate daemon: $ALT_DAEMON" +fi +copy_bin "$REPO/target/release/colony-firewalld" +copy_bin "$REPO/target/release/cfc" +ln -sf gawk "$RFS/usr/bin/awk" +ln -sf bash "$RFS/usr/bin/sh" + +echo "==> python stdlib" +PYDIR="$(python3 -c 'import sysconfig; print(sysconfig.get_paths()["stdlib"])')" +mkdir -p "$RFS$(dirname "$PYDIR")" +tar -C "$(dirname "$PYDIR")" -cf - \ + --exclude='test' --exclude='tests' --exclude='__pycache__' --exclude='idlelib' \ + --exclude='tkinter' --exclude='lib2to3' --exclude='ensurepip' --exclude='site-packages' \ + --exclude='config-*' --exclude='pydoc_data' --exclude='turtledemo' \ + "$(basename "$PYDIR")" | tar -C "$RFS$(dirname "$PYDIR")" -xf - +# The stdlib's C extensions have their own shared-object dependencies. +for so in "$RFS$PYDIR"/lib-dynload/*.so; do + [ -e "$so" ] || continue + for lib in $(ldd "$so" 2>/dev/null | grep -oE '/[^ ]+\.so[^ ]*' || true); do + copy_file "$lib" || true + done +done +true + +echo "==> kernel modules" +# Named in dependency order and resolved with `modinfo -n`, not with +# `modprobe --show-depends`. Arch ships an install directive for nf_conntrack +# (it runs sysctl afterwards), so --show-depends prints an `install` line +# rather than an `insmod` one for it - the filter dropped it, the guest booted +# without conntrack, and `ct state new queue num 0` was refused with a bare +# ENOENT. modinfo answers about the module file itself and has no such +# indirection. +: > "$OUT/modlist" +for m in nf_defrag_ipv4 nf_defrag_ipv6 nf_conntrack nf_tables nft_ct nft_queue \ + nfnetlink_queue veth x_tables; do + ko="$(modinfo -n "$m" 2>/dev/null || true)" + if [ -n "$ko" ] && copy_file "$ko"; then + echo "$ko" >> "$OUT/modlist" + else + echo " $m: built in or absent, nothing to carry" + fi +done +install -m0644 "$OUT/modlist" "$RFS/modlist" +echo " $(wc -l < "$OUT/modlist") modules" + +echo "==> the daemon's own files" +install -m0644 "$REPO/crates/cfc-ebpf/target/bpfel-unknown-none/release/cfc-ebpf.o" "$RFS/cfc-ebpf.o" +install -m0644 "$REPO/systemd/nftables-snippet.conf" "$RFS/etc/colony-firewall/nftables-snippet.conf" +install -m0755 "$REPO/scripts/bench-latency.sh" "$RFS/bench/bench-latency.sh" +install -m0755 "$OUT/plan.sh" "$RFS/bench/plan.sh" +install -m0755 "$OUT/init" "$RFS/init" + +printf 'root:x:0:0:root:/root:/bin/bash\n' > "$RFS/etc/passwd" +printf 'root:x:0:\ncolony-firewall:x:970:\n' > "$RFS/etc/group" +printf 'passwd: files\ngroup: files\nhosts: files\n' > "$RFS/etc/nsswitch.conf" +printf '127.0.0.1 localhost\n' > "$RFS/etc/hosts" +printf 'guest\n' > "$RFS/etc/hostname" +cp -aL /etc/protocols /etc/services "$RFS/etc/" 2>/dev/null || true + +echo "==> pack" +( cd "$RFS" && find . | cpio -o -H newc --owner=0:0 --quiet | gzip -1 ) > "$OUT/rootfs.cpio.gz" +du -sh "$RFS" "$OUT/rootfs.cpio.gz" | awk '{print " "$1"\t"$2}' diff --git a/scripts/vm-bench/init b/scripts/vm-bench/init new file mode 100755 index 0000000..66672f2 --- /dev/null +++ b/scripts/vm-bench/init @@ -0,0 +1,60 @@ +#!/bin/bash +# PID 1. Nothing else will mount anything, so everything the daemon and the +# bench touch is mounted here. +# +# /dev first, then take the console: a cpio rootfs has no device nodes, so +# until devtmpfs is up this script has no stdout at all and a failure before +# that line is invisible. +mount -t devtmpfs devtmpfs /dev +exec > /dev/console 2>&1 + +mount -t proc proc /proc +mount -t sysfs sysfs /sys +mount -t tmpfs tmpfs /tmp +mount -t tmpfs tmpfs /run +mount -t cgroup2 cgroup2 /sys/fs/cgroup +# bpffs, unlike the CI guests: with it the exec/exit links pin, which is what +# lets the fast path run with its full sixty-second deadline rather than the +# shortened one. The measurement should see the feature as a host sees it. +mount -t bpf bpf /sys/fs/bpf +mount -t tracefs tracefs /sys/kernel/tracing 2>/dev/null + +mkdir -p /run/colony-firewall /run/netns /var/lib/colony-firewall /var/log/colony-firewall +export PATH=/usr/bin:/usr/sbin:/bin:/sbin +export HOME=/root + +echo "guest kernel: $(uname -r)" + +# Two passes: the dependency order in the list is per-module, so a module +# whose dependency comes later in the list fails the first time and takes on +# the second. Silent by design - "already loaded" is the common failure. +for pass in 1 2 3; do + while read -r ko; do + err="$(insmod "$ko" 2>&1)" + # Third pass only: by then a real failure is not an ordering problem, + # and "already loaded" is the expected answer for everything that took. + if [ "$pass" = 3 ] && [ -n "$err" ] && [[ "$err" != *"File exists"* ]]; then + echo " module $(basename "$ko"): $err" + fi + done < /modlist +done +echo "modules loaded: $(awk '{printf "%s ", $1}' /proc/modules)" + +ip link set lo up + +# How many samples per direction, from the kernel command line, so a smoke run +# and a real run are the same guest booted differently. +# The plan's knobs come down the kernel command line, so a quick check and a +# real run are the same guest booted differently. +cmdline() { sed -n "s/.*$1=\([^ ]*\).*/\1/p" /proc/cmdline; } +DRAIN_SECS="$(cmdline cfc_drain)" +SWEEP="$(cmdline cfc_sweep | tr ',' ' ')" +export DRAIN_SECS SWEEP +echo "sweep: ${SWEEP:-default} drain: ${DRAIN_SECS:-default}s" + +/bench/plan.sh +echo "CFC_DONE=$?" + +# Exiting is the shutdown: panic=1 turns a dead init into a reboot, and +# qemu's -no-reboot turns that reboot into an exit. +exit 0 diff --git a/scripts/vm-bench/plan.sh b/scripts/vm-bench/plan.sh new file mode 100755 index 0000000..58917e8 --- /dev/null +++ b/scripts/vm-bench/plan.sh @@ -0,0 +1,144 @@ +#!/bin/bash +# Why does a queued flow cost what it costs, and does that cost depend on load? +# +# The first full run said 17.8 ms per queued flow at 3000 flows and 5.5 ms at +# 40, with the two rounds 15.0 and 20.7 ms apart - and the state that does +# strictly MORE work (`armed`: the fast path live but the rule ineligible) came +# out faster than the one that does less. None of that is a per-packet +# constant. Two candidate explanations, and this run separates them: +# +# 1. a fixed cost per flow, dominated by RECV_POLL_INTERVAL (5 ms), the beat +# the NFQUEUE worker idles on. Testable by changing the constant: the same +# daemon built with 200 us is in this image as colony-firewalld-alt. +# 2. a cost that grows with how many sockets are around, because attribution +# falls back to reading /proc/net/tcp, whose length is the socket table. +# Testable by sweeping the number of flows with everything else fixed. +# +# So: the same state at 100, 300, 1000 and 3000 flows, with a drain between +# every state so each starts from a comparable socket table, and the socket and +# conntrack counts recorded next to each measurement rather than assumed. +set -uo pipefail + +SOCK=/run/colony-firewall/cfc.sock +CFG=/etc/colony-firewall/daemon.toml +LOG=/var/log/colony-firewall/daemon.log +SNIPPET=/etc/colony-firewall/nftables-snippet.conf +PIN=/sys/fs/bpf/colony-firewall +DPID="" +PY="$(readlink -f "$(command -v python3)")" + +say() { echo "== $*"; } +ctx() { echo "CTX $*"; } +qseq() { awk '$1=="0"{print $8; exit}' /proc/net/netfilter/nfnetlink_queue 2>/dev/null || echo 0; } +sockets() { echo $(( $(wc -l < /proc/net/tcp) - 1 )); } +ctcount() { cat /proc/sys/net/netfilter/nf_conntrack_count 2>/dev/null || echo '?'; } + +# TIME_WAIT is 60 s and nothing shortens it. A state that starts on the +# previous state's leftovers is measuring the previous state too. +drain() { + local before; before="$(sockets)" + sleep "${DRAIN_SECS:-70}" + ctx "drain sockets $before -> $(sockets), conntrack $(ctcount)" +} + +write_cfg() { + cat > "$CFG" < /tmp/rules.json < "$LOG" + RUST_LOG="${2:-}" "$1" --config "$CFG" --socket "$SOCK" >> "$LOG" 2>&1 & + DPID=$! + for _ in $(seq 1 150); do [ -S "$SOCK" ] && return 0; sleep 0.1; done + say "!! socket never appeared for $1"; tail -20 "$LOG"; return 1 +} + +stop_daemon() { + [ -n "$DPID" ] && { kill "$DPID" 2>/dev/null; wait "$DPID" 2>/dev/null; } + DPID="" + nft delete table inet colony_firewall 2>/dev/null + rm -rf "$PIN" +} + +probe_layer() { + say "what the in-kernel layer comes up as here" + write_cfg true + start_daemon /usr/bin/colony-firewalld info || return 1 + nft -f "$SNIPPET"; write_rules + cfc --socket "$SOCK" rules import --replace /tmp/rules.json >/dev/null 2>&1 + sleep 4 + for k in ring0 enforcement degrade fast_path exec_tracking exit_tracking dns_capture ppid_from_btf; do + v="$(grep -oE "$k=[A-Za-z_-]+" "$LOG" | tail -1)" + [ -n "$v" ] && ctx "layer $v" + done + ctx "layer $(cfc --socket "$SOCK" status --json | python3 -c 'import json,sys; d=json.load(sys.stdin); print("status_fast_allow=%s status_enforcing=%s" % (d["fast_allow"], d["enforcing"]))')" + stop_daemon +} + +measure() { # $1 label $2 n $3 mode(none|queue|fast) $4 binary + local label="$1" n="$2" mode="$3" bin="${4:-/usr/bin/colony-firewalld}" q0 q1 + say "state: $label n=$n mode=$mode daemon=$(basename "$bin")" + if [ "$mode" != none ]; then + [ -x "$bin" ] || { echo "SKIP $label: $bin is not in this image"; return 0; } + if [ "$mode" = fast ]; then write_cfg true; else write_cfg false; fi + start_daemon "$bin" || { echo "FAIL $label"; return 1; } + nft -f "$SNIPPET" || { echo "FAIL $label nft"; stop_daemon; return 1; } + write_rules + cfc --socket "$SOCK" rules import --replace /tmp/rules.json >/dev/null 2>&1 + sleep 4 + ctx "$label fast_allow=$(cfc --socket "$SOCK" status --json 2>/dev/null | python3 -c 'import json,sys; print(json.load(sys.stdin).get("fast_allow","?"))' 2>/dev/null || echo unreachable)" + fi + ctx "$label before sockets=$(sockets) conntrack=$(ctcount)" + q0="$(qseq)" + /bench/bench-latency.sh -n "$n" -w 20 -t 5 -l "$label" --json 2>/dev/null \ + | while read -r line; do echo "RESULT $line"; done + q1="$(qseq)" + ctx "$label after sockets=$(sockets) conntrack=$(ctcount) queued_packets=$(( q1 - q0 ))" + [ "$mode" != none ] && stop_daemon + return 0 +} + +say "the binary the rules name: $PY" +say "conntrack max: $(cat /proc/sys/net/netfilter/nf_conntrack_max 2>/dev/null || echo '?')" +probe_layer + +# The sweep: one variable at a time, a drain before each. SWEEP overrides the +# flow counts (a short list is how one checks the harness itself without +# waiting for the real thing). +SWEEP="${SWEEP:-100 300 1000 3000}" +SMALL="${SWEEP%% *}"; LARGE="${SWEEP##* }" + +drain; measure "floor-$SMALL" "$SMALL" none +for n in $SWEEP; do + drain; measure "queue-$n" "$n" queue /usr/bin/colony-firewalld +done +for n in "$SMALL" "$LARGE"; do + drain; measure "poll200us-$n" "$n" queue /usr/bin/colony-firewalld-alt +done +for n in "$SMALL" "$LARGE"; do + drain; measure "fast-$n" "$n" fast /usr/bin/colony-firewalld +done +[ "$SMALL" != "$LARGE" ] && { drain; measure "floor-$LARGE" "$LARGE" none; } +say "done" diff --git a/scripts/vm-bench/report.py b/scripts/vm-bench/report.py new file mode 100755 index 0000000..65a7729 --- /dev/null +++ b/scripts/vm-bench/report.py @@ -0,0 +1,77 @@ +#!/usr/bin/env python3 +"""Turn a guest console log into the table the measurement is for. + +Reads the `RESULT` lines (one JSON object per state and direction) and the +`CTX` lines (what was true beside each measurement) that `plan.sh` prints. +""" +import collections +import json +import sys + + +def main(path): + rows, ctx = [], collections.defaultdict(list) + for line in open(path, errors="replace"): + line = line.replace("\r", "").strip() + if line.startswith("RESULT "): + try: + rows.append(json.loads(line[7:])) + except json.JSONDecodeError: + pass + elif line.startswith("CTX "): + head, _, rest = line[4:].partition(" ") + if rest: + ctx[head].append(rest) + + print(f"{'state':<16} {'dir':<4} {'flows':>6} {'p50 ms':>9} {'p90 ms':>9} " + f"{'p99 ms':>9} {'max ms':>9}") + print("-" * 72) + out = {} + for r in rows: + if not r.get("ms"): + print(f"{r['label']:<16} {r['direction']:<4} {r['ok']:>6} " + f"no connect succeeded: {r.get('failed')}") + continue + m = r["ms"] + if r["direction"] == "out": + out[r["label"]] = m["p50"] + print(f"{r['label']:<16} {r['direction']:<4} {r['ok']:>6} {m['p50']:>9.4f} " + f"{m['p90']:>9.4f} {m['p99']:>9.4f} {m['max']:>9.4f}") + + if ctx: + print("\nwhat was true beside each state") + for k in sorted(ctx): + print(f" {k:<16} " + " | ".join(ctx[k])) + + def pair(a, b, what): + if a in out and b in out: + print(f" {what:<50} {out[a]:>8.4f} vs {out[b]:>8.4f} " + f"{out[a] - out[b]:+8.4f} ms") + + # The flow counts are whatever the run used (SWEEP overrides them), so the + # comparisons are derived from the labels present rather than named here - + # a hard-coded "queue-3000" prints nothing at all on a shorter sweep, and + # silence reads like "no difference" instead of "not measured". + def counts(prefix): + return sorted(int(k.rsplit("-", 1)[1]) for k in out + if k.rsplit("-", 1)[0] == prefix and k.rsplit("-", 1)[1].isdigit()) + + q, f, fl, po = (counts(x) for x in ("queue", "fast", "floor", "poll200us")) + print("\nreadings (p50 of the `out` direction, the one that meets the queue)") + if len(q) > 1: + pair(f"queue-{q[-1]}", f"queue-{q[0]}", + f"queue: {q[-1]} flows against {q[0]}") + for n in po: + pair(f"poll200us-{n}", f"queue-{n}", + f"{n} flows: a 200us idle beat against the 5ms one") + for n in f: + pair(f"fast-{n}", f"queue-{n}", f"{n} flows: the fast path against the queue") + for n in sorted(set(f) & set(fl)): + pair(f"fast-{n}", f"floor-{n}", f"{n} flows: what the fast path costs over nothing") + if len(fl) > 1: + pair(f"floor-{fl[-1]}", f"floor-{fl[0]}", + f"the floor itself, {fl[-1]} flows against {fl[0]}") + + +if __name__ == "__main__": + main(sys.argv[1] if len(sys.argv) > 1 else "guest.log") diff --git a/scripts/vm-bench/run.sh b/scripts/vm-bench/run.sh new file mode 100755 index 0000000..d57b491 --- /dev/null +++ b/scripts/vm-bench/run.sh @@ -0,0 +1,58 @@ +#!/usr/bin/env bash +# Build the guest and boot it. The guest's own output is the verdict. +# +# sudo -v && ./scripts/vm-bench/run.sh +# +# Needs qemu-system-x86_64, KVM, and one `sudo` to read the host kernel image +# (it is copied once into the work directory and then owned by you). Everything +# else is assembled from this machine's own binaries; nothing is downloaded. +# +# Knobs, all optional: +# KERNEL the kernel image to boot (default: this machine's) +# OUT work directory (default: target/vm-bench) +# SWEEP flow counts to measure, e.g. "100 300 1000 3000" (the default) +# DRAIN_SECS seconds between states, so each starts on a comparable socket +# table (default 70, which is one TIME_WAIT) +# ALT_DAEMON a second colony-firewalld to carry into the image, measured +# beside the first. Used to attribute a cost to one constant: +# build it, point at it, and the plan runs both. +set -euo pipefail + +REPO="$(cd "$(dirname "$0")/../.." && pwd)" +OUT="${OUT:-$REPO/target/vm-bench}" +KERNEL="${KERNEL:-/boot/vmlinuz-$(uname -r)}" +[ -e "$KERNEL" ] || KERNEL=/boot/vmlinuz-linux +mkdir -p "$OUT" +export REPO OUT + +for f in build-guest.sh init plan.sh; do + install -m0755 "$REPO/scripts/vm-bench/$f" "$OUT/$f" +done + +if [ ! -r "$OUT/vmlinuz" ]; then + echo "==> kernel ($KERNEL)" + # Arch ships /boot/vmlinuz-linux mode 0600 root:root; one sudo, once. + sudo install -m0644 -o "$(id -u)" -g "$(id -g)" "$KERNEL" "$OUT/vmlinuz" +fi + +"$OUT/build-guest.sh" + +echo "==> boot" +START=$(date +%s) +RC=0 +# The guest panics on purpose when its init exits, and -no-reboot turns that +# into a qemu exit: the marker below is the verdict, not this exit code. +timeout "${TIMEOUT:-2700}" qemu-system-x86_64 \ + -enable-kvm -cpu host -m "${MEM:-4G}" -smp "${SMP:-4}" \ + -kernel "$OUT/vmlinuz" -initrd "$OUT/rootfs.cpio.gz" \ + -append "rdinit=/init console=ttyS0 panic=1${SWEEP:+ cfc_sweep=${SWEEP// /,}}${DRAIN_SECS:+ cfc_drain=$DRAIN_SECS}" \ + -nographic -no-reboot > "$OUT/guest.log" 2>&1 || RC=$? +echo "==> qemu exit ${RC} (informational), $(( $(date +%s) - START ))s wall clock" + +if ! grep -q 'CFC_DONE=' "$OUT/guest.log"; then + echo "the guest never reached the end of the plan; see $OUT/guest.log" >&2 + exit 1 +fi +python3 "$REPO/scripts/vm-bench/report.py" "$OUT/guest.log" | tee "$OUT/report.txt" +echo +echo "full guest console: $OUT/guest.log" From 623725a8bac602799961391c8df94e06feb638a8 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Sun, 6 Sep 2026 12:00:36 +0200 Subject: [PATCH 2/4] perf: what the fast path is worth, measured, and two claims it disproved TODO 1a has said since the fast path landed that the number justifying it was not measured. It is now. Per new outbound TCP flow, median, Linux 7.2.2 under KVM: 300 flows 3000 flows no firewall 0.0158 ms 0.0162 ms the fast path 0.0268 ms 0.0269 ms the NFQUEUE queue 5.6745 ms 7.6083 ms The fast path saves 5.6 ms per new flow at 300 flows and 7.6 ms at 3000, and costs 0.011 ms over having no firewall at all. Its cost does not grow with load, because those flows never reach the daemon; the queue's does. The 0.28 ms this file used to quote came from a different bench and understated the case by two orders of magnitude. Attributing that cost rather than only recording it found two statements in this repo that the measurement contradicts, and both are corrected here rather than left standing beside the number that disproves them. nfqueue.rs predicted "up to one RECV_POLL_INTERVAL (mean: half that)". Half is right for arrivals independent of the beat and wrong for a client connecting in series: each connect lands just after the worker observed an empty queue and committed to a fresh wait, so it pays close to a whole interval every time. Proved the way this file's own memory demands - build the same daemon with the constant at 200 us, run both in one guest - and not from the shape of a distribution, which is how this path has been misread before: 4.90 ms of the 5.67 at 300 flows, 5.24 of the 7.61 at 3000. docs/ARCHITECTURE.md still described the design nfqueue.rs replaced: a worker blocking in `recv` with "no polling, no added latency" whenever no prompt was outstanding. That has not been true since the blocking recv became the ninety-second hang on every daemon stop. What is left open, in TODO 1a: the remaining queued cost grows with the number of live sockets - 0.77 ms at 300 flows against 2.36 at 3000, with the beat removed - and that growth is the daemon's own per-packet work, not the kernel's, because the floor moved 0.0003 ms across the same range. Attribution is the suspect and is not yet proven. --- CHANGELOG.md | 33 +++++++++++++++++++++ TODO.md | 49 +++++++++++++++++++++++++++----- crates/cfc-daemon/src/nfqueue.rs | 25 +++++++++++----- docs/ARCHITECTURE.md | 22 ++++++++++---- 4 files changed, 110 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a76b871..10e03f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,39 @@ and [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added + +- **`scripts/vm-bench`**, which measures what the firewall costs on a machine + it is allowed to arm. It assembles an initramfs from this host's own kernel + modules, nftables, iproute2, python3 and the release binaries, boots it under + KVM, and runs `scripts/bench-latency.sh` there against a real daemon - queue + rule loaded, rules imported, fast path granting. Nothing is downloaded and + nothing outside `target/vm-bench` is written. Each state differs from its + neighbour in one thing, and two facts are recorded beside every measurement + rather than assumed: what `cfc status` says the fast path is, and how many + packets the kernel actually handed to userspace. + +### Fixed + +- **`docs/ARCHITECTURE.md` described a design that had been replaced.** It + said the NFQUEUE worker blocks in `recv` while no prompt is outstanding - + "no polling, no added latency" - which is the design `nfqueue.rs` replaced, + and the opposite of what the shipped daemon does. +- **`nfqueue.rs` predicted half the idle beat per queued flow; it is a whole + one.** "Mean: half that" holds for arrivals independent of the beat, not for + a client connecting in series, where every connect lands just after the + worker committed to a fresh wait. Measured by building the same daemon with + `RECV_POLL_INTERVAL` at 200 us and running both in one guest. + +### Measured + +- **What the fast path is worth**, per new outbound TCP flow, median, on Linux + 7.2.2 under KVM: 0.0269 ms against 7.6083 ms through the queue at 3000 + flows, and 0.0268 against 5.6745 at 300. It costs 0.011 ms over having no + firewall at all, and unlike the queue its cost does not grow with load, + because those flows never reach the daemon. `TODO.md` 1a carries the whole + table and what is still unattributed. + ## [0.4.0] - 2026-09-05 ### Added diff --git a/TODO.md b/TODO.md index 3955d22..df3f084 100644 --- a/TODO.md +++ b/TODO.md @@ -101,13 +101,48 @@ grants written with no liveness guard where the deny side had carried one since it was written; and the feature being **inert for its own motivating case** - nothing granted a process that was already running, so every restart silently switched the fast path off for every long-lived program while -`cfc status` said `live`. What is not done: the latency win is not yet -*measured* on the veth bench - the number that justifies the feature is still -the pre-feature 0.28 ms per new flow - and 1b below is untouched. The bench -that produces that number is `scripts/bench-latency.sh`: a veth pair into a -network namespace, both directions, run once per state (client under a -lasting Allow rule, under a flow-scoped one, table absent) on a VM where CFC -may be armed. +`cfc status` said `live`. + +**Measured, 2026-09-06.** The number that justifies the feature exists now: +`scripts/vm-bench` boots a throwaway guest from this machine's own kernel and +binaries and runs `scripts/bench-latency.sh` there against a real daemon, so +CFC can be armed without arming it on anybody's workstation. On Linux 7.2.2 +under KVM, per new outbound TCP flow, median: + +| | 300 flows | 3000 flows | +|---|---|---| +| no firewall at all | 0.0158 ms | 0.0162 ms | +| the fast path | 0.0268 ms | 0.0269 ms | +| the NFQUEUE round trip | 5.6745 ms | 7.6083 ms | + +So the fast path saves **5.6 ms per new flow at 300 flows and 7.6 ms at +3000**, and costs 0.011 ms over having no firewall. Its cost does not grow +with load, because those flows never reach the daemon; the queue's does. The +0.28 ms this file quoted before was measured on a different bench and is not +comparable - and it understated the case by two orders of magnitude. + +Two findings came out of attributing that cost rather than just recording it, +both now written where they were wrong: + +- **A whole `RECV_POLL_INTERVAL` is paid per queued flow, not half of one.** + `nfqueue.rs` predicted "mean: half that", which holds for arrivals + independent of the beat and not for a client connecting in series: each + connect lands just after the worker committed to a fresh idle wait. Proved + by building the same daemon with the constant at 200 us and measuring both + in one guest - 4.90 ms of the 5.67 at 300 flows, 5.24 ms of the 7.61 at + 3000. Not by reading a distribution's shape, which is how this path has been + misread before. +- **`docs/ARCHITECTURE.md` still described the blocking-recv design** that + `nfqueue.rs` replaced, claiming "no polling, no added latency" for the + common case. Corrected. + +What is left here: the remaining queued cost grows with the number of live +sockets (0.77 ms at 300 flows against 2.36 ms at 3000, with the beat removed) +and that growth is in the daemon's own per-packet work - the floor moved +0.0003 ms across the same range. Attribution is the obvious suspect and is not +yet proven; the cheap next experiment is the same sweep with `[ebpf] enabled` +off, which forces the `/proc` walk and should separate the socket-cookie path +from the fallback. 1b below is still untouched. **1b. Rules that depend on a destination still cannot be precomputed.** `process_wide_action` deliberately answers `None` for them, which is correct and diff --git a/crates/cfc-daemon/src/nfqueue.rs b/crates/cfc-daemon/src/nfqueue.rs index a9590d3..a1a4b3c 100644 --- a/crates/cfc-daemon/src/nfqueue.rs +++ b/crates/cfc-daemon/src/nfqueue.rs @@ -42,13 +42,24 @@ //! that this replaces. //! //! The price is that while the worker is idle the first packet of an -//! intercepted flow can wait up to one [`RECV_POLL_INTERVAL`] (mean: half -//! that) in the kernel queue, and that the idle worker wakes at that -//! cadence. It is the same cadence the loop already paid whenever a prompt -//! was outstanding, and single-digit milliseconds on connection setup is a -//! far better trade than a minute and a half on every restart. If `nfq` -//! ever exposes the netlink fd, move the idle wait to a `poll()` on it: -//! that buys back the zero added latency *and* keeps the bounded stop. +//! intercepted flow waits in the kernel queue for the rest of the current +//! beat, and that the idle worker wakes at that cadence. +//! +//! "Mean: half of one interval" is what this comment used to claim, and it +//! is only true of arrivals that are independent of the beat. A client that +//! connects in series is not: each connect lands just after the worker +//! observed an empty queue and committed to a fresh wait, so it pays close +//! to a whole interval, every time. Measured in `scripts/vm-bench` by +//! building this file with the constant at 200 us and running both daemons +//! in one guest - 4.90 ms of 5.67 at 300 flows, 5.24 ms of 7.61 at 3000 - +//! rather than inferred from the shape of a distribution, which is how this +//! path has been misread before. +//! +//! It remains a far better trade than a minute and a half on every restart, +//! and the fast path takes the whole round trip away for a process a lasting +//! rule allows. If `nfq` ever exposes the netlink fd, move the idle wait to +//! a `poll()` on it: that buys back the added latency *and* keeps the +//! bounded stop. use crate::config::NfqConfig; use crate::decision::{Decision, Engine}; diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 45ee4db..343be1c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -103,11 +103,23 @@ The worker keeps two maps that are created and destroyed together: Verdicts arrive asynchronously on a separate channel and are applied out of order, so a slow prompt only delays its own flow. -**Recv mode follows the outstanding count.** With no prompt outstanding the -worker blocks in `recv`, which is both the common case and the cheapest one: -no polling, no added latency. The moment a prompt is outstanding it switches -the queue socket to non-blocking and interleaves `recv` with 5ms waits on the -verdict channel. It switches back once the last prompt resolves. +**The queue socket is non-blocking for the worker's whole life**, and every +idle turn of the loop waits up to `RECV_POLL_INTERVAL` (5 ms) on the verdict +channel. This paragraph used to describe the opposite - a blocking `recv` with +"no polling, no added latency" whenever no prompt was outstanding - which was +true of an earlier design and had not been true for some time. A thread parked +in a blocking `recv` cannot be woken to see a stop flag, and that was ninety +seconds of hang on every daemon stop; `crates/cfc-daemon/src/nfqueue.rs` has +the full argument. + +The price is real and is now measured rather than estimated: a queued flow +pays a whole idle beat, about 5 ms, because a client connecting in series +lands just after the worker committed to a fresh wait. `scripts/vm-bench` +attributes it - 4.90 ms of 5.67 at 300 flows, 5.24 ms of 7.61 at 3000, by +building the same daemon with the constant at 200 us and measuring both in one +boot. The fast path below removes the round trip entirely for a process a +lasting rule allows, which is what makes that cost bearable rather than +something to redesign around today. **Prompt deduplication** is keyed on `(exe-or-pid, dst_ip, dst_port, protocol)`. Source address and port are deliberately excluded, so a SYN From d11359e77beafbe4805fea3723d7f2c9c06ac417 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Sun, 6 Sep 2026 14:21:41 +0200 Subject: [PATCH 3/4] fix(security): eight defects an adversarial audit found, and the three my own reading did An audit of the cores - the control socket, the packet parser and the refusal path, the rule engine, DNS and provenance - returned findings I then verified against the code myself, one at a time, before writing a line. Two of them were the same shape and it is the shape that matters: a rule the user wrote was not the rule being applied. **The fast path could grant what the packet path denies.** The two deciders read different uids - exec-time on one side, live on the other - so for a program that drops privileges a uid-scoped deny above a broad allow was answered both ways, and the grant is process-wide. The grant side now stands aside wherever a uid-scoped rule could reach the program, which is exactly what the deny side already did. Picking a uid instead would have changed what every existing rule means. **A rule that could not be decided was walked past.** A deny scoped to a digest the daemon cannot compute handed the flow to a lower allow. `lookup` is three-valued now and stops there. The connection half is tested first so a rule its own destination excludes still abstains for nothing - the trap in the obvious version of this fix. **A hostname nobody confirmed could admit traffic.** The kernel gate for an observed DNS answer is "source port 53" and nothing else: no transaction id, no resolver address, no question-to-answer check. Such a name may now refuse but not admit. One direction, deliberately: refusing it both ways is the obvious change and would have quietly disarmed every `deny --dst-host` on a machine running the DNS observer. The rest, each verified before and after: ICMP refusals forged with a multicast source for mDNS and DHCP traffic; the same refusals unbudgeted off the machine, which is a reflector; four raw sockets receiving a copy of every TCP segment on the host for the daemon's life; `enforcing` latched true after one packet, including after the ruleset was removed under it; an unbounded `exe_path` from the wire; an unbounded event offset; a hostname cache whose bound one insert path did not honour. One performance change rides with them because it is the same file as a security one: attribution opened and closed a netlink socket per queued packet, measured at 0.28 ms of every queued flow. It is now one socket per thread - which is only safe because each request carries its own sequence number and the reply is checked against it. Every request used to carry seq 1, so a late answer to a timed-out request was indistinguishable from the next one's: a wrong attribution, not a slow one. The socket is discarded on anything but a cleanly-sequenced answer. CHANGELOG has the full account. Every fix carries the test that fails without it, including one that attaches the send-only filter to a UDP socket and proves the kernel really drops - the raw-socket version needs privileges the test suite does not have. Two mistakes of my own, caught by verifying rather than by assuming, are worth recording because both were the fix being wrong rather than the finding: refusing an unconfirmed hostname in both directions (it would have disarmed every deny by name), and reporting an absent nftables table verbatim (the shipped units load it *after* the daemon, so that called every boot unprotected for its first minute). --- CHANGELOG.md | 89 ++++++ crates/cfc-core/src/connection.rs | 29 ++ crates/cfc-core/src/rule.rs | 387 ++++++++++++++++++++++++-- crates/cfc-daemon/src/decision.rs | 135 ++++++++- crates/cfc-daemon/src/dns.rs | 97 ++++++- crates/cfc-daemon/src/ebpf.rs | 9 + crates/cfc-daemon/src/ebpf/enforce.rs | 8 + crates/cfc-daemon/src/ebpf/nft_set.rs | 22 ++ crates/cfc-daemon/src/ipc.rs | 113 +++++++- crates/cfc-daemon/src/main.rs | 42 +++ crates/cfc-daemon/src/nfqueue.rs | 92 +++++- crates/cfc-daemon/src/reject.rs | 342 ++++++++++++++++++++++- crates/cfc-daemon/src/sock_diag.rs | 185 ++++++++++-- crates/cfc-daemon/src/stats.rs | 54 +++- 14 files changed, 1533 insertions(+), 71 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10e03f2..b1f7a66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,95 @@ and [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Security + +- **The fast path could grant what the packet path denies.** The two deciders + did not read the same uid: the packet path takes the uid from the kernel's + exec record, the grant path resolves it from `/proc` at the time it decides. + For a program that drops privileges after `exec` - `named`, `postfix`, a + browser entering its sandbox - those differ, so `deny --exe X --uid 0` above + `allow --exe X` was answered "deny" by one and "allow" by the other. A grant + is process-wide and destination-blind, so the deny was not slower, it never + applied. The grant side now abstains wherever a uid-scoped rule could reach + the program, exactly as the deny side already did; those flows take the + queue, where the uid has one answer. Hosts with no uid-scoped rule are + unaffected. +- **A rule that could not be decided was walked past.** `matches_process` + collapses "cannot say" into "does not match", so a `deny` scoped to + `exe_sha256` over a binary the daemon cannot hash - over 64 MiB, unreadable, + or a process whose image is already gone - handed the flow to a + lower-precedence `allow`. The deny listed, ranked first and never fired. + `RuleSet::lookup` is now three-valued: a rule that is *about* this flow but + undecidable stops the walk, and the default applies instead of a rule its + author wrote it to override. The connection half is tested first, so a rule + its own destination excludes still abstains for nothing. +- **Hostnames observed on the wire could admit traffic.** Nothing correlates + an observed DNS response to a query this host sent - the kernel gate is + `source port == 53` and no more - so any peer answering from that port could + assert any name for its own address and inherit whatever a `dst_host` rule + allows it. Such a name may now refuse but may not admit; a reverse lookup, + which is forward-confirmed, still does both. Deny rules written against a + name keep working exactly as before, which is why the check runs one way + only. +- **ICMP refusals were emitted for multicast and broadcast destinations, + sourced from the group address.** Inbound, `dst_ip` is this machine - and + for mDNS, SSDP, LLMNR or a DHCP offer it is the group the datagram was sent + to. Refusing those put a martian source on the wire, one packet per packet + received, at every neighbour that speaks multicast: an RFC 1122 and RFC 4443 + violation, a way to enumerate CFC hosts on a segment, and an answer to the + DHCP server that breaks the lease. Both endpoints must now be ordinary + unicast addresses before anything is forged. +- **Refusals that leave the machine are budgeted.** The destination came + straight from the packet with no limit of any kind, so a spoofed source + address turned the daemon into an unthrottled ICMP reflector - `nft reject` + cannot be used that way because the kernel rate-limits `icmp_send`, and a + raw socket with `IP_HDRINCL` is governed by nothing. Twenty per second off + the machine, with a burst of twenty. Refusals to a local application are not + budgeted: throttling those would restore the timeout the feature replaces. +- **`cfc status` reported `enforcing` forever once it had seen one packet.** + The counter only goes up, so after the first connection the answer was "yes" + for the daemon's life - including after `nft flush ruleset` or a firewall + reload took the table away and the machine stopped being filtered. A packet + counter cannot tell "nothing is filtered" from "nothing is happening", so + the daemon now asks nftables directly, once a minute, and says what it + found. An absent table inside the startup grace period is not alarming: the + shipped units start the daemon before the one that loads the table. +- **An `exe_path` from the wire was unbounded.** A 4 MiB path passed every + gate and then drove one `canonicalize(2)` per component - millions of + syscalls - on the sixteen-slot blocking pool the prompt router also uses. + Rules now refuse a path longer than `PATH_MAX`, which no process could match + anyway. The rejection deliberately does not echo the path back. +- **`ListEvents` skipped an unbounded number of rows.** `limit` was clamped + and `offset` was not, so a read-only peer could make sqlite step and discard + the whole event table per call, holding the global connection mutex. + +### Fixed + +- **Four raw sockets received a copy of every TCP segment and every ICMP + packet on the machine, for the daemon's whole life.** They exist only to + send refusals and are never read, but `IP_HDRINCL` governs sends alone: the + kernel clones every matching packet into a buffer that could only fill and + drop. They now carry a one-instruction filter that returns zero, the + standard way to say send-only. Opened unconditionally at start, so this was + a tax on every host running CFC, whether or not any rule ever rejected. +- **The hostname cache had no upper bound on one of its two insert paths.** A + completed reverse lookup re-created its own key even when the reservation + had already been evicted, so the map ratcheted upward by one per orphaned + completion on any workload touching more distinct destinations than it + holds. A completion with nothing to update is now dropped. + +### Performance + +- **One netlink socket per thread instead of one per queued packet.** + Attribution opened, configured and closed an `AF_NETLINK` socket for every + packet the queue handed up, on the single datapath thread. Measured on + `scripts/vm-bench`: 0.28 ms of every queued flow at 3000 flows. Reuse is + only safe because each request now carries its own sequence number and the + reply is checked against it - every request used to carry `seq = 1`, so a + late answer to a timed-out request would have been indistinguishable from + the next one's, which is a wrong attribution rather than a slow one. The + socket is discarded on anything but a cleanly-sequenced answer. + ### Added - **`scripts/vm-bench`**, which measures what the firewall costs on a machine diff --git a/crates/cfc-core/src/connection.rs b/crates/cfc-core/src/connection.rs index 3d72c0c..6d565a7 100644 --- a/crates/cfc-core/src/connection.rs +++ b/crates/cfc-core/src/connection.rs @@ -28,6 +28,24 @@ pub struct Connection { pub dst_ip: IpAddr, pub dst_port: u16, pub dst_host: Option, + /// Whether [`Self::dst_host`] was confirmed to belong to [`Self::dst_ip`], + /// rather than merely asserted by something on the wire. + /// + /// The daemon learns names two ways and they are not equally + /// trustworthy. A reverse lookup is forward-confirmed - the name must + /// resolve back to this address, so claiming one means controlling that + /// name's forward zone. A name lifted out of an observed DNS response is + /// whatever the packet said: nothing correlates such a response to a + /// query this host sent, so any peer that answers from source port 53 can + /// assert any name for any address. + /// + /// What that costs an unconfirmed name is the power to *admit*, and only + /// that: see [`crate::Rule::permits_on_an_unverified_name`]. It may still + /// refuse, so a `deny --dst-host` keeps working exactly as before, and it + /// is still attached to the flow either way - it is what the live feed, + /// the log and the prompt show, and it is right nearly always. + #[serde(default)] + pub dst_host_verified: bool, pub pid: Option, pub uid: Option, } @@ -51,6 +69,7 @@ impl Connection { dst_ip, dst_port, dst_host: None, + dst_host_verified: false, pid: None, uid: None, } @@ -64,8 +83,18 @@ impl Connection { self } + /// Attaches a name that has *not* been confirmed against the address. + /// See [`Self::dst_host_verified`]. pub fn with_host(mut self, host: impl Into) -> Self { self.dst_host = Some(host.into()); + self.dst_host_verified = false; + self + } + + /// Attaches a name and says whether it was confirmed against the address. + pub fn with_host_verified(mut self, host: impl Into, verified: bool) -> Self { + self.dst_host = Some(host.into()); + self.dst_host_verified = verified; self } } diff --git a/crates/cfc-core/src/rule.rs b/crates/cfc-core/src/rule.rs index 0317947..2bf611f 100644 --- a/crates/cfc-core/src/rule.rs +++ b/crates/cfc-core/src/rule.rs @@ -70,6 +70,13 @@ pub struct RuleScope { pub protocol: Option, } +/// Longest `exe_path` a rule may carry. +/// +/// `PATH_MAX` on Linux is 4096 including the terminator, so nothing longer can +/// name a file that exists. Rules are matched by exact string equality against +/// `/proc//exe`, so a longer one is unmatchable by construction. +pub const MAX_EXE_PATH_LEN: usize = 4096; + /// Largest executable either side will hash for an `exe_sha256` predicate. /// /// One constant, two enforcers, and they must agree: the daemon refuses to @@ -241,6 +248,29 @@ impl RuleScope { exe.display() )); } + // A path no filesystem can hold is a path no process can be running, + // so such a rule can never fire - the same test as the two above, for + // a value that arrives over the wire. + // + // The bound is here rather than at the wire because this is the gate + // both writers already run. What it stops is not a bad rule: it is the + // work of *rejecting* one. `exe_path` is a bare proto string, capped + // only by the 4 MiB decode limit, and resolution walks one + // `canonicalize(2)` per path component from the leaf upward - about + // two million syscalls for a 4 MiB path, on a blocking pool of + // sixteen that the prompt router also depends on. + // + // The message deliberately does not print the path. Every other arm + // here formats it into a string that becomes a gRPC status and a log + // line, which for this input is the denial-of-service repeated on the + // way out. + if exe.as_os_str().len() > MAX_EXE_PATH_LEN { + return Err(format!( + "exe path is {} bytes; the kernel cannot hold a path longer \ + than {MAX_EXE_PATH_LEN}, so no process could ever match it", + exe.as_os_str().len() + )); + } Ok(()) } @@ -376,9 +406,21 @@ impl RuleScope { } pub fn matches(&self, conn: &crate::Connection, proc: &crate::Process) -> bool { - if !self.matches_process(proc) { - return false; - } + self.matches_process(proc) && self.matches_connection(conn) + } + + /// The connection half of [`Self::matches`], on its own. + /// + /// The mirror of [`Self::matches_process`], and split out for the mirror + /// reason: a caller that has a connection needs to ask "could this rule + /// be about this flow at all?" *before* asking whether the process half + /// can be decided. Without that order, a rule undecidable for the process + /// would stop the search for every flow, including the ones its own + /// destination predicates exclude - `deny --exe X --sha256 H + /// --dst-port 25` would abstain on a connection to port 80, which it can + /// never be about. [`Self::matches`] is defined in terms of both halves, + /// so none of the three can drift. + pub fn matches_connection(&self, conn: &crate::Connection) -> bool { // First, because it is the cheapest and the most likely to exclude: // an inbound rule must never fire on outbound traffic or the reverse. // An unset direction means **outbound**, not "both". @@ -480,6 +522,29 @@ impl Rule { /// True when this rule should no longer match at `now_unix_ms`. /// /// Only `Duration::Seconds(n)` expires here: the rule stops matching once + /// Whether this rule would *admit* a flow on the strength of a hostname + /// nothing confirmed. + /// + /// The asymmetry is the point, and getting it backwards is a hole either + /// way. Names reach the daemon two ways. A reverse lookup is + /// forward-confirmed, so claiming one means controlling that name's + /// forward zone. A name lifted out of an observed DNS response is + /// whatever the packet said: nothing ties such a response to a query this + /// host sent, so any peer that answers from source port 53 can assert any + /// name for any address. + /// + /// So an unconfirmed name may still *refuse* - an attacker gains nothing + /// by naming themselves something the user has denied, and honouring it + /// keeps every existing deny rule working exactly as before. It may not + /// *permit*: otherwise an attacker wears a name the user trusts and + /// inherits its allowance. Refusing the name in both directions is the + /// obvious change and it would have quietly disarmed every + /// `deny --dst-host` on the machine, which is the failure this codebase + /// minds most. + pub fn permits_on_an_unverified_name(&self, conn: &crate::Connection) -> bool { + self.action == Action::Allow && self.scope.dst_host.is_some() && !conn.dst_host_verified + } + /// `created_at + n` seconds have elapsed. `Always` and `UntilRestart` /// never expire at lookup time (`UntilRestart` rules are purged from /// storage at daemon startup instead). `Once` also returns false: real @@ -549,16 +614,78 @@ impl RuleSet { /// `now_unix_ms` is the current wall-clock time; rules whose /// `Duration::Seconds(..)` window has elapsed are skipped (see /// [`Rule::is_expired`]). + /// A rule that is about this flow but cannot be decided stops the walk, + /// rather than being skipped as if it did not match. See [`Match`]. pub fn lookup( &self, conn: &crate::Connection, proc: &crate::Process, now_unix_ms: i64, - ) -> Option<&Rule> { - self.rules + ) -> Match<'_> { + for rule in self + .rules .iter() .filter(|r| r.enabled && !r.is_expired(now_unix_ms)) - .find(|r| r.scope.matches(conn, proc)) + { + // The connection half first, so a rule's own destination + // predicates can exclude it before its process half is ever + // questioned. Without that order an undecidable rule would abstain + // for flows it can never be about. + if !rule.scope.matches_connection(conn) { + continue; + } + if rule.scope.undecidable_for(proc) { + return Match::Undecidable(rule); + } + if rule.scope.matches_process(proc) { + if rule.permits_on_an_unverified_name(conn) { + // A name nobody confirmed may refuse traffic, never admit + // it. Skip this rule and keep walking: something below it + // may still answer, and anything below an allow is at + // least as restrictive. + continue; + } + return Match::Rule(rule); + } + } + Match::None + } +} + +/// What [`RuleSet::lookup`] found. +/// +/// Three outcomes, not two, and the third is the whole point. Precedence is +/// ordered, so a rule that cannot be decided must not be walked past: the +/// rules beneath it are the ones its author wrote it to override. +/// +/// The case in the field is a `deny` scoped to `exe_sha256` over a binary the +/// daemon cannot hash - over 64 MiB, unreadable, or a process whose image is +/// already gone. `matches_process` collapses "cannot say" into "does not +/// match", so the walk continued and a lower-precedence `allow --exe X` won. +/// The deny listed, ranked first, and never fired - indistinguishable from +/// working. `RuleScope::undecidable_for` was written for exactly this hazard +/// and its own documentation says so, but it was only ever consulted on the +/// fast path, where the mirror case (an abstaining *allow* handing the flow to +/// a lower *deny*) had been noticed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Match<'a> { + /// This rule answered. + Rule(&'a Rule), + /// This rule is about this flow and outranks everything below it, but the + /// daemon cannot tell whether it matches. Nobody answers; the caller must + /// treat it as "no rule decided" and fall back to asking. + Undecidable(&'a Rule), + /// No rule is about this flow. + None, +} + +impl<'a> Match<'a> { + /// The rule that answered, if one did. An abstention is not an answer. + pub fn rule(self) -> Option<&'a Rule> { + match self { + Self::Rule(r) => Some(r), + _ => None, + } } } @@ -595,12 +722,201 @@ mod tests { chrono::Utc::now().timestamp_millis() } + /// The rule set the defect needs: a hash-scoped deny ranking above a + /// plain allow for the same program. + fn deny_by_hash_over_allow(dst_port: Option) -> RuleSet { + let mut hashed = RuleScope::any(); + hashed.exe_path = Some(PathBuf::from("/usr/bin/curl")); + hashed.exe_sha256 = Some("aa".repeat(32)); + hashed.dst_port = dst_port; + let mut plain = RuleScope::any(); + plain.exe_path = Some(PathBuf::from("/usr/bin/curl")); + let mut set = RuleSet { + rules: vec![ + Rule::new("deny-that-binary".to_string(), Action::Deny, hashed), + Rule::new("allow-curl".to_string(), Action::Allow, plain), + ], + }; + set.sort_deterministic(); + set + } + + #[test] + fn a_deny_that_cannot_be_decided_does_not_hand_the_flow_to_an_allow() { + let set = deny_by_hash_over_allow(None); + let conn = mk_conn(); + + // With the digest in hand the deny answers, as it always did. + let mut hashed = mk_proc("/usr/bin/curl"); + hashed.sha256 = Some("aa".repeat(32)); + assert_eq!( + set.lookup(&conn, &hashed, now()).rule().map(|r| r.action), + Some(Action::Deny) + ); + + // Without it - a binary over the hashing cap, an unreadable image, a + // process whose image is already gone - the deny cannot be decided. + // It must stop the walk, not be skipped: the allow beneath it is the + // rule its author wrote the deny to override. + let unhashed = mk_proc("/usr/bin/curl"); + assert_eq!(unhashed.sha256, None); + match set.lookup(&conn, &unhashed, now()) { + Match::Undecidable(r) => assert_eq!(r.name, "deny-that-binary"), + other => panic!("expected an abstention, got {other:?}"), + } + assert!( + set.lookup(&conn, &unhashed, now()).rule().is_none(), + "an abstention is not an answer" + ); + } + + #[test] + fn an_undecidable_rule_only_abstains_for_flows_it_could_be_about() { + // The trap in the fix: keying the abstention on the process alone + // would let `deny --exe X --sha256 H --dst-port 25` stop the walk for + // a connection to port 443, which that rule can never be about. + let set = deny_by_hash_over_allow(Some(25)); + let unhashed = mk_proc("/usr/bin/curl"); + + // Port 443: the deny's own destination predicate excludes it, so the + // allow answers normally. + let conn = mk_conn(); + assert_eq!(conn.dst_port, 443); + assert_eq!( + set.lookup(&conn, &unhashed, now()).rule().map(|r| r.action), + Some(Action::Allow), + "a rule excluded by its own destination must not abstain" + ); + + // Port 25: now it is about this flow, and it abstains. + let mut smtp = mk_conn(); + smtp.dst_port = 25; + assert!(matches!( + set.lookup(&smtp, &unhashed, now()), + Match::Undecidable(_) + )); + } + + #[test] + fn matches_is_still_exactly_its_two_halves() { + // The split must not have changed what `matches` means. + let conn = mk_conn(); + let proc = mk_proc("/usr/bin/curl"); + for scope in [ + RuleScope::any(), + { + let mut s = RuleScope::any(); + s.exe_path = Some(PathBuf::from("/usr/bin/curl")); + s + }, + { + let mut s = RuleScope::any(); + s.dst_port = Some(443); + s + }, + { + let mut s = RuleScope::any(); + s.dst_port = Some(80); + s + }, + { + let mut s = RuleScope::any(); + s.exe_path = Some(PathBuf::from("/usr/bin/wget")); + s + }, + { + let mut s = RuleScope::any(); + s.direction = Some(Direction::Inbound); + s + }, + ] { + assert_eq!( + scope.matches(&conn, &proc), + scope.matches_process(&proc) && scope.matches_connection(&conn), + "the halves must recompose into the whole for {scope:?}" + ); + } + } + + #[test] + fn an_unverified_name_admits_nothing_and_still_refuses() { + let mut named = RuleScope::any(); + named.dst_host = Some("example.org".to_string()); + + let mut conn = mk_conn(); + conn.dst_host = Some("example.org".to_string()); + conn.dst_host_verified = false; + + let allow = Rule::new("a".to_string(), Action::Allow, named.clone()); + let deny = Rule::new("d".to_string(), Action::Deny, named.clone()); + assert!( + allow.permits_on_an_unverified_name(&conn), + "an allow keyed on a name nothing confirmed must stand aside" + ); + assert!( + !deny.permits_on_an_unverified_name(&conn), + "a deny is not admitting anything, so it still applies" + ); + + // Confirmed, and the allow is back in play. + conn.dst_host_verified = true; + assert!(!allow.permits_on_an_unverified_name(&conn)); + + // A rule with no name predicate is untouched either way. + let plain = Rule::new("p".to_string(), Action::Allow, RuleScope::any()); + conn.dst_host_verified = false; + assert!(!plain.permits_on_an_unverified_name(&conn)); + } + + #[test] + fn an_unverified_name_lets_a_lower_rule_answer() { + // The walk must keep going past the abstaining allow, not stop. + // + // The allow has to genuinely outrank the deny or this proves nothing: + // at equal specificity `action_rank` puts Deny first, so the deny + // would win either way and the interesting assertion would pass for + // the wrong reason. Two predicates against one. + let mut named = RuleScope::any(); + named.dst_host = Some("example.org".to_string()); + named.dst_port = Some(443); + let mut everything = RuleScope::any(); + everything.dst_port = Some(443); + let mut set = RuleSet { + rules: vec![ + Rule::new("allow-by-name".to_string(), Action::Allow, named), + Rule::new("deny-that-port".to_string(), Action::Deny, everything), + ], + }; + set.sort_deterministic(); + assert_eq!( + set.rules[0].name, "allow-by-name", + "the allow must sort first for this test to mean anything" + ); + + let proc = mk_proc("/usr/bin/curl"); + let mut conn = mk_conn(); + conn.dst_host = Some("example.org".to_string()); + + conn.dst_host_verified = false; + assert_eq!( + set.lookup(&conn, &proc, now()).rule().map(|r| r.action), + Some(Action::Deny), + "with the name unconfirmed the rule below answers" + ); + conn.dst_host_verified = true; + assert_eq!( + set.lookup(&conn, &proc, now()).rule().map(|r| r.action), + Some(Action::Allow), + "with it confirmed the more specific allow wins again" + ); + } + #[test] fn empty_set_returns_none() { let set = RuleSet::default(); let conn = mk_conn(); let proc = mk_proc("/usr/bin/curl"); - assert!(set.lookup(&conn, &proc, now()).is_none()); + assert!(set.lookup(&conn, &proc, now()).rule().is_none()); } #[test] @@ -616,9 +932,11 @@ mod tests { assert!(set .lookup(&conn, &mk_proc("/usr/bin/curl"), now()) + .rule() .is_some()); assert!(set .lookup(&conn, &mk_proc("/usr/bin/wget"), now()) + .rule() .is_none()); } @@ -632,9 +950,9 @@ mod tests { let proc = mk_proc("/usr/bin/curl"); let mut conn = mk_conn(); - assert!(set.lookup(&conn, &proc, now()).is_some()); + assert!(set.lookup(&conn, &proc, now()).rule().is_some()); conn.dst_port = 80; - assert!(set.lookup(&conn, &proc, now()).is_none()); + assert!(set.lookup(&conn, &proc, now()).rule().is_none()); } #[test] @@ -648,10 +966,10 @@ mod tests { let mut conn = mk_conn(); conn.dst_ip = IpAddr::V4(Ipv4Addr::new(10, 1, 2, 3)); - assert!(set.lookup(&conn, &proc, now()).is_some()); + assert!(set.lookup(&conn, &proc, now()).rule().is_some()); conn.dst_ip = IpAddr::V4(Ipv4Addr::new(8, 8, 8, 8)); - assert!(set.lookup(&conn, &proc, now()).is_none()); + assert!(set.lookup(&conn, &proc, now()).rule().is_none()); } #[test] @@ -667,21 +985,22 @@ mod tests { // All three match -> hit. let mut conn = mk_conn(); - assert!(set.lookup(&conn, &proc, now()).is_some()); + assert!(set.lookup(&conn, &proc, now()).rule().is_some()); // Wrong port -> miss. conn.dst_port = 80; - assert!(set.lookup(&conn, &proc, now()).is_none()); + assert!(set.lookup(&conn, &proc, now()).rule().is_none()); // Wrong proto -> miss. conn.dst_port = 443; conn.protocol = Protocol::Udp; - assert!(set.lookup(&conn, &proc, now()).is_none()); + assert!(set.lookup(&conn, &proc, now()).rule().is_none()); // Wrong exe -> miss. conn.protocol = Protocol::Tcp; assert!(set .lookup(&conn, &mk_proc("/usr/bin/python"), now()) + .rule() .is_none()); } @@ -695,7 +1014,7 @@ mod tests { let set = RuleSet { rules: vec![rule] }; let conn = mk_conn(); let proc = mk_proc("/usr/bin/curl"); - assert!(set.lookup(&conn, &proc, now()).is_none()); + assert!(set.lookup(&conn, &proc, now()).rule().is_none()); } /// Was `first_matching_rule_wins`, which codified whatever Vec order the @@ -723,7 +1042,10 @@ mod tests { let conn = mk_conn(); let proc = mk_proc("/usr/bin/curl"); - let hit = set.lookup(&conn, &proc, now()).expect("should match"); + let hit = set + .lookup(&conn, &proc, now()) + .rule() + .expect("should match"); assert_eq!(hit.action, Action::Deny); assert_eq!(hit.name, "deny-curl"); } @@ -748,7 +1070,10 @@ mod tests { let conn = mk_conn(); let proc = mk_proc("/usr/bin/curl"); - let hit = set.lookup(&conn, &proc, now()).expect("should match"); + let hit = set + .lookup(&conn, &proc, now()) + .rule() + .expect("should match"); assert_eq!(hit.name, "allow-curl-443"); assert_eq!(hit.action, Action::Allow); } @@ -776,8 +1101,14 @@ mod tests { }; reverse.sort_deterministic(); - let hit_fwd = forward.lookup(&conn, &proc, at).expect("should match"); - let hit_rev = reverse.lookup(&conn, &proc, at).expect("should match"); + let hit_fwd = forward + .lookup(&conn, &proc, at) + .rule() + .expect("should match"); + let hit_rev = reverse + .lookup(&conn, &proc, at) + .rule() + .expect("should match"); assert_eq!(hit_fwd.id, hit_rev.id); assert_eq!(hit_fwd.name, "deny-curl"); } @@ -799,6 +1130,7 @@ mod tests { let hit = set .lookup(&mk_conn(), &mk_proc("/usr/bin/curl"), now()) + .rule() .expect("should match"); assert_eq!(hit.name, "old-allow"); } @@ -812,11 +1144,14 @@ mod tests { let set = RuleSet { rules: vec![rule] }; let conn = mk_conn(); - assert!(set.lookup(&conn, &mk_proc("/anything"), now()).is_some()); + assert!(set + .lookup(&conn, &mk_proc("/anything"), now()) + .rule() + .is_some()); let mut other = mk_proc("/anything"); other.uid = Some(2000); - assert!(set.lookup(&conn, &other, now()).is_none()); + assert!(set.lookup(&conn, &other, now()).rule().is_none()); } #[test] @@ -831,7 +1166,7 @@ mod tests { let conn = mk_conn(); let unknown = Process::unknown(0); assert_eq!(unknown.uid, None); - assert!(set.lookup(&conn, &unknown, now()).is_none()); + assert!(set.lookup(&conn, &unknown, now()).rule().is_none()); } #[test] @@ -850,11 +1185,11 @@ mod tests { let expiry = created + 3600 * 1000; // Still inside the window -> matches. - assert!(set.lookup(&conn, &proc, created + 1).is_some()); - assert!(set.lookup(&conn, &proc, expiry - 1).is_some()); + assert!(set.lookup(&conn, &proc, created + 1).rule().is_some()); + assert!(set.lookup(&conn, &proc, expiry - 1).rule().is_some()); // At and after expiry -> skipped. - assert!(set.lookup(&conn, &proc, expiry).is_none()); - assert!(set.lookup(&conn, &proc, expiry + 1).is_none()); + assert!(set.lookup(&conn, &proc, expiry).rule().is_none()); + assert!(set.lookup(&conn, &proc, expiry + 1).rule().is_none()); } #[test] diff --git a/crates/cfc-daemon/src/decision.rs b/crates/cfc-daemon/src/decision.rs index 03d0523..0be212e 100644 --- a/crates/cfc-daemon/src/decision.rs +++ b/crates/cfc-daemon/src/decision.rs @@ -157,9 +157,25 @@ impl Engine { let now_unix_ms = chrono::Utc::now().timestamp_millis(); let rule_match = { let rules = self.inner.rules.read(); - rules - .lookup(conn, proc, now_unix_ms) - .map(|r| (r.id, r.action)) + match rules.lookup(conn, proc, now_unix_ms) { + cfc_core::rule::Match::Rule(r) => Some((r.id, r.action)), + // A rule that is about this flow could not be decided. It + // outranks everything below it, so nothing below it may answer + // in its place: ask instead. For the case this exists for - a + // hash-scoped deny over a binary that cannot be hashed - the + // alternative was falling through to a lower allow, which is + // the deny silently not existing. + cfc_core::rule::Match::Undecidable(r) => { + tracing::debug!( + rule = %r.name, + exe = %proc.exe.display(), + "a rule that outranks the rest cannot be decided for this process; \ + no rule answers and the default applies" + ); + None + } + cfc_core::rule::Match::None => None, + } }; if let Some((rule_id, action)) = rule_match { *self.inner.hits.lock().entry(rule_id).or_insert(0) += 1; @@ -412,6 +428,42 @@ impl Engine { ) } + /// Whether any live rule scoped to a uid could apply to `exe`. + /// + /// The mirror of [`Self::compilable_exe_paths`], for the grant map rather + /// than the deny map, and it exists for the same reason turned around. + /// + /// The two deciders do not read the same uid. The packet path takes the + /// uid from the kernel's exec record when it has one - the uid at + /// `execve` - and does not read `/proc//status` at all. The grant + /// path resolves through `/proc` and gets the uid the process holds + /// *now*. For a program that drops privileges after exec - `named`, + /// `postfix`, a browser entering its sandbox - those are different, so + /// `deny --exe X --uid 0` above `allow --exe X` can be answered "deny" by + /// the packet path and "allow" by the grant path. A grant is process-wide + /// and destination-blind, so that disagreement is not a slower answer, it + /// is the deny never being applied at all. + /// + /// Rather than decide which uid is the right one - a semantic change to + /// what every existing uid-scoped rule means - the fast path simply + /// abstains wherever a uid could matter. Those flows take the queue, + /// where the uid question has one answer and it is the packet path's. + /// Hosts with no uid-scoped rule, which is nearly all of them, pay + /// nothing: the walk stops at the first predicate. + pub fn uid_scoped_may_apply(&self, exe: &std::path::Path) -> bool { + let now_unix_ms = chrono::Utc::now().timestamp_millis(); + self.inner + .rules + .read() + .rules + .iter() + .filter(|r| r.enabled && !r.is_expired(now_unix_ms)) + .filter(|r| r.scope.uid.is_some()) + // A uid-scoped rule that names no executable can apply to any + // program, exactly as in `compilable_exe_paths`. + .any(|r| r.scope.exe_path.as_deref().is_none_or(|p| p == exe)) + } + /// What an inbound flow gets when no rule matches. /// /// Separate from `no_ui_action` because it answers a different question. @@ -605,6 +657,83 @@ mod tests { Engine::new(RuleSet { rules }, shared(dp_deny())) } + // --- the uid guard on the grant path -------------------------------- + + #[test] + fn a_uid_scoped_rule_takes_its_program_off_the_fast_path() { + let exe = std::path::Path::new("/usr/sbin/named"); + let other = std::path::Path::new("/usr/bin/curl"); + + // The shape that made the fast path grant what the packet path denies: + // the program execs as root and drops to its own uid, so the grant + // path reads 53 and the packet path reads 0, and only one of them + // sees the deny. + let mut denied_as_root = RuleScope::any(); + denied_as_root.exe_path = Some(PathBuf::from(exe)); + denied_as_root.uid = Some(0); + let mut allowed = RuleScope::any(); + allowed.exe_path = Some(PathBuf::from(exe)); + let engine = engine_with(vec![ + Rule::new("deny-as-root".to_string(), Action::Deny, denied_as_root), + Rule::new("allow".to_string(), Action::Allow, allowed), + ]); + assert!( + engine.uid_scoped_may_apply(exe), + "a uid-scoped rule names this program, so the fast path must stand aside" + ); + assert!( + !engine.uid_scoped_may_apply(other), + "a program no uid-scoped rule names is unaffected" + ); + + // A rule set with no uid predicate at all costs nothing: this is the + // common case and it must not be taken off the fast path. + let mut plain = RuleScope::any(); + plain.exe_path = Some(PathBuf::from(exe)); + let engine = engine_with(vec![Rule::new("a".to_string(), Action::Allow, plain)]); + assert!(!engine.uid_scoped_may_apply(exe)); + } + + #[test] + fn a_uid_rule_naming_no_program_takes_everything_off_the_fast_path() { + // It could apply to anything, and nothing here can tell whether it + // would - the same reasoning `compilable_exe_paths` uses to return + // `None` rather than a list. + let mut any_program = RuleScope::any(); + any_program.uid = Some(1000); + let engine = engine_with(vec![Rule::new( + "per-user".to_string(), + Action::Deny, + any_program, + )]); + for exe in ["/usr/bin/curl", "/usr/sbin/named", "/opt/whatever"] { + assert!( + engine.uid_scoped_may_apply(std::path::Path::new(exe)), + "{exe} must not be granted while a uid rule can reach any program" + ); + } + } + + #[test] + fn a_disabled_or_expired_uid_rule_does_not_hold_the_fast_path_back() { + let exe = std::path::Path::new("/usr/sbin/named"); + let mut scope = RuleScope::any(); + scope.exe_path = Some(PathBuf::from(exe)); + scope.uid = Some(0); + + let mut disabled = Rule::new("off".to_string(), Action::Deny, scope.clone()); + disabled.enabled = false; + assert!(!engine_with(vec![disabled]).uid_scoped_may_apply(exe)); + + let mut expired = Rule::new("gone".to_string(), Action::Deny, scope); + expired.duration = cfc_core::Duration::Seconds(1); + expired.created_at = chrono::Utc::now() - chrono::Duration::hours(1); + assert!( + !engine_with(vec![expired]).uid_scoped_may_apply(exe), + "an expired rule constrains nothing" + ); + } + // --- process_wide_action ------------------------------------------- // // This is what the `cgroup/connect4|6` programs are steered by, and it diff --git a/crates/cfc-daemon/src/dns.rs b/crates/cfc-daemon/src/dns.rs index 432d5f6..bf80eeb 100644 --- a/crates/cfc-daemon/src/dns.rs +++ b/crates/cfc-daemon/src/dns.rs @@ -171,6 +171,25 @@ impl DnsCache { /// The trust level backing the currently cached name for `ip`, if any. /// Diagnostics and tests; the packet path does not care. + /// The cached name for `ip` and whether it was confirmed against that + /// address, in one read. + /// + /// Confirmed means a reverse lookup that passed forward confirmation: the + /// name resolved back to this address, so asserting it takes control of + /// that name's forward zone. A name lifted out of an observed response is + /// not confirmed - nothing ties such a response to a query this host sent + /// - and only decorates the flow. See `Connection::dst_host_verified`. + pub fn cached_named(&self, ip: IpAddr) -> Option<(String, bool)> { + let now = Instant::now(); + let cache = self.inner.cache.read(); + let entry = cache.get(&ip)?; + if !entry.is_fresh(now) { + return None; + } + let name = entry.hostname.clone()?; + Some((name, entry.trust == Trust::Ptr)) + } + pub fn cached_trust(&self, ip: IpAddr) -> Option { let cache = self.inner.cache.read(); let entry = cache.get(&ip)?; @@ -293,12 +312,29 @@ impl DnsCache { /// live observed one, and it must not be bypassable. fn record_ptr_result(inner: &Inner, ip: IpAddr, hostname: Option, now: Instant) { let mut cache = inner.cache.write(); + // Only ever *update* a key `enqueue_lookup` reserved; never create one. + // + // This is the bound, and it is a branch rather than a call to + // `evict_if_full`. Every insert site but this one is paired with a + // reservation, so this one adding an eviction would be a third O(n) scan + // under the write lock for no gain - and it would still not bound the map, + // because eviction removes exactly one entry per insert while a completing + // lookup whose placeholder was already evicted adds one. The map ratcheted + // upward by one per orphaned completion, without limit, on any workload + // touching more distinct destinations than the cache holds. + // + // A completion whose placeholder is gone has nothing to say: the entry it + // would describe was evicted precisely because nothing had asked for it + // recently. Dropping the answer costs one hostname, and the next packet to + // that address enqueues a fresh lookup. + let Some(existing) = cache.get(&ip) else { + tracing::trace!(%ip, "PTR result arrived after its reservation was evicted; dropping"); + return; + }; // An answer observed on the wire while this lookup was in flight is // better than the result we just got; do not clobber it. - if let Some(existing) = cache.get(&ip) { - if !existing.supersedes(Trust::Ptr, now) { - return; - } + if !existing.supersedes(Trust::Ptr, now) { + return; } let ttl = Duration::from_secs(if hostname.is_some() { CACHE_TTL_SECS @@ -418,10 +454,63 @@ mod tests { /// Files a PTR result exactly as the completed lookup task would, without /// needing a resolver. + /// + /// Including the reservation, because that is half of what the real path + /// does: `enqueue_lookup` always inserts an `in_flight` placeholder before + /// it spawns, and `record_ptr_result` now only ever *updates* a key that + /// placeholder created. A helper that skipped the reservation would test a + /// sequence that cannot happen. fn insert_ptr(cache: &DnsCache, addr: IpAddr, name: Option<&str>, now: Instant) { + reserve(cache, addr, now); record_ptr_result(&cache.inner, addr, name.map(str::to_string), now); } + /// The `in_flight` placeholder `enqueue_lookup` reserves before spawning. + /// + /// Non-clobbering, exactly as the real one is: `enqueue_lookup` returns + /// early when an entry is already there, so a reservation can never throw + /// away an observation. A helper that overwrote would have made every + /// trust-ordering test below pass for the wrong reason. + fn reserve(cache: &DnsCache, addr: IpAddr, now: Instant) { + let mut c = cache.inner.cache.write(); + if c.contains_key(&addr) { + return; + } + c.insert( + addr, + Entry { + hostname: None, + inserted: now, + in_flight: true, + trust: Trust::Ptr, + ttl: Duration::from_secs(NEGATIVE_TTL_SECS), + }, + ); + } + + #[test] + fn a_ptr_result_whose_reservation_was_evicted_is_dropped() { + // The bound. `evict_if_full` removes one entry per insert, so a + // completing lookup that re-created its own evicted key added one back + // - the map ratcheted upward for as long as new destinations kept + // arriving. A completion with nothing to update has nothing to say. + let cache = DnsCache::new(); + let now = Instant::now(); + let addr = ip("198.51.100.7"); + record_ptr_result(&cache.inner, addr, Some("orphan.example".to_string()), now); + assert_eq!( + cache.lookup_at(addr, now), + None, + "a result nobody reserved must not create an entry" + ); + assert_eq!(cache.cached_trust(addr), None); + + // With the reservation in place it lands, as it always did. + reserve(&cache, addr, now); + record_ptr_result(&cache.inner, addr, Some("real.example".to_string()), now); + assert_eq!(cache.lookup_at(addr, now).as_deref(), Some("real.example")); + } + #[test] fn an_observed_answer_is_returned_and_marked_observed() { let cache = DnsCache::new(); diff --git a/crates/cfc-daemon/src/ebpf.rs b/crates/cfc-daemon/src/ebpf.rs index 8d768fe..f94e450 100644 --- a/crates/cfc-daemon/src/ebpf.rs +++ b/crates/cfc-daemon/src/ebpf.rs @@ -744,6 +744,15 @@ pub struct Runtime { _attached: Option, } +/// Whether the nftables table that feeds NFQUEUE is loaded. +/// +/// Available in every build, feature or not: it asks `nft`, not the kernel's +/// BPF machinery, and the question it answers - "is anything actually being +/// filtered?" - is not about the eBPF layer at all. +pub fn nft_table_loaded() -> anyhow::Result { + nft_set::table_loaded() +} + /// Flushes a previous daemon's fast-allow mark out of the nftables set, for /// the starts where [`start`] never reaches the loader's own flush: the layer /// switched off in the config, or a build without it. The set outlives diff --git a/crates/cfc-daemon/src/ebpf/enforce.rs b/crates/cfc-daemon/src/ebpf/enforce.rs index a832312..76056cf 100644 --- a/crates/cfc-daemon/src/ebpf/enforce.rs +++ b/crates/cfc-daemon/src/ebpf/enforce.rs @@ -345,6 +345,14 @@ impl VerdictSink { /// The grant decision for one process: the shared rule for every writer. fn grant_for(&self, proc: &Process) -> Grant { + // Abstain wherever a uid-scoped rule could apply. This decider and the + // packet path read different uids for a process that dropped + // privileges, and a grant is process-wide - see + // `Engine::uid_scoped_may_apply`, which explains why the answer is to + // step aside rather than to pick a uid. + if self.engine.uid_scoped_may_apply(&proc.exe) { + return Grant::No; + } match self.engine.process_wide_verdict(proc) { Some(v) if v.fast_allow_eligible() => Grant::Yes(v.rule_id), _ => Grant::No, diff --git a/crates/cfc-daemon/src/ebpf/nft_set.rs b/crates/cfc-daemon/src/ebpf/nft_set.rs index f3c2f09..cd94012 100644 --- a/crates/cfc-daemon/src/ebpf/nft_set.rs +++ b/crates/cfc-daemon/src/ebpf/nft_set.rs @@ -225,6 +225,28 @@ pub(super) fn holds(mark: u32) -> anyhow::Result { } } +/// Whether `table inet colony_firewall` is loaded at all. +/// +/// This is the question `cfc status`'s `enforcing` is really asking. Without +/// the table nothing reaches NFQUEUE, so nothing is filtered - and that state +/// is invisible from inside the daemon, which simply sees no packets. An idle +/// machine also sees no packets, which is why the packet counter alone cannot +/// tell the two apart and this probe exists. +/// +/// A missing table answers `false`, not an error; anything else - nft absent, +/// the transaction lock held, a permission failure - is an error, because +/// "could not ask" and "asked and it is gone" must not read the same. The +/// caller keeps its previous answer on an error rather than claiming the +/// firewall vanished because a fork failed. +pub(super) fn table_loaded() -> anyhow::Result { + let op = Op::ListTable; + match run(op) { + Ok(()) => Ok(true), + Err(failed) if failed.is_no_such_object() => Ok(false), + Err(failed) => Err(failed.into_error(op)), + } +} + /// Flushes `set fast_allow`, so that no value (this daemon's or a previous /// one's) is accepted by the ruleset. /// diff --git a/crates/cfc-daemon/src/ipc.rs b/crates/cfc-daemon/src/ipc.rs index 3569f07..4026868 100644 --- a/crates/cfc-daemon/src/ipc.rs +++ b/crates/cfc-daemon/src/ipc.rs @@ -133,6 +133,15 @@ const ENFORCING_GRACE_SECS: u64 = 60; const DEFAULT_EVENT_PAGE: u32 = 100; const MAX_EVENT_PAGE: u32 = 1000; +/// Largest `offset` a `ListEvents` request may skip to. +/// +/// `limit` was clamped and `offset` was not, and sqlite pays for a skipped row +/// much as it pays for a returned one: `OFFSET n` steps and discards n rows, +/// applying the `instr(exe, ?)` filter to each, with no index to help. The +/// event table is capped at `[events] max_rows`, so any offset past this can +/// only ever return nothing - clamping it removes no reachable page. +const MAX_EVENT_OFFSET: u32 = 1_000_000; + /// Depth of the datapath -> event-writer queue. The writer batches, so this /// only needs to absorb a burst, never sustained throughput. const EVENT_QUEUE_DEPTH: usize = 4096; @@ -704,7 +713,12 @@ impl Firewall for FirewallService { no_ui_action: convert::action_to_pb(policy.no_ui_action) as i32, prompt_timeout_secs: policy.prompt_timeout_secs, skipped_rules: self.store.skipped_rules() as u64, - enforcing: enforcing_heuristic(self.dry_run, connections_seen, uptime_seconds), + enforcing: enforcing_heuristic( + self.dry_run, + connections_seen, + uptime_seconds, + self.stats.nft_table(), + ), enforcement: crate::ebpf::enforcement_level() .map_or("starting", |l| l.as_str()) .to_string(), @@ -831,11 +845,35 @@ fn resolve_pause_secs(duration_secs: u32, default_secs: u64) -> u64 { /// nothing is being filtered and saying otherwise would be a lie. Outside /// dry-run, seeing no packet at all after the grace period almost always /// means the nftables/iptables rule that feeds NFQUEUE is not loaded. -fn enforcing_heuristic(dry_run: bool, packets_seen: u64, uptime_secs: u64) -> bool { +fn enforcing_heuristic( + dry_run: bool, + packets_seen: u64, + uptime_secs: u64, + table: crate::stats::TablePresence, +) -> bool { + use crate::stats::TablePresence; if dry_run { return false; } - packets_seen > 0 || uptime_secs <= ENFORCING_GRACE_SECS + let starting = uptime_secs <= ENFORCING_GRACE_SECS; + match table { + // Evidence, not a guess, so it decides. The packet counter below + // never decreases, so on its own it could only ever answer "yes, once + // upon a time" - exactly wrong in the case that matters, a ruleset + // removed under a running daemon. + TablePresence::Present => true, + // Also evidence, but only once the machine has had time to load it. + // The shipped unit ordering starts this daemon *first* and + // `colony-firewall-nft.service` after it, so an absent table is the + // expected state for the first moments of every boot - and saying + // "not enforcing" then would be a false alarm on every start, in the + // field people are told to read. + TablePresence::Absent => starting, + // The probe has not run yet, or could not run at all - no nft binary, + // no permission, the transaction lock held. Fall back to what the + // daemon can see for itself. + TablePresence::Unknown => packets_seen > 0 || starting, + } } /// Maps a `ListEvents` request onto the storage query. Rejects an @@ -856,7 +894,7 @@ fn event_query_from_pb(req: &ListEventsRequest) -> Result<(u32, u32, EventFilter action, since_ts_unix_ms: (req.since_unix_ms > 0).then_some(req.since_unix_ms), }; - Ok((limit, req.offset, filter)) + Ok((limit, req.offset.min(MAX_EVENT_OFFSET), filter)) } // --------------------------------------------------------------------------- @@ -1136,6 +1174,7 @@ pub async fn spawn( #[cfg(test)] mod tests { use super::*; + use crate::stats::TablePresence; // -- authorization ------------------------------------------------------ @@ -1223,20 +1262,76 @@ mod tests { // -- enforcing heuristic ------------------------------------------------ + #[test] + fn a_removed_ruleset_stops_reading_as_enforcing() { + // The defect this replaces: `packets_seen` only ever goes up, so once + // one packet had been seen the answer was "yes" for the life of the + // daemon - including after the table was flushed out from under it and + // nothing was being filtered at all. + assert!( + !enforcing_heuristic(false, 1_000_000, 100_000, TablePresence::Absent), + "a machine whose table is gone is not enforcing, however many \ + packets it saw before that" + ); + // And the other way: a table that is loaded settles the question on a + // machine so idle it has never seen a packet. + assert!( + enforcing_heuristic(false, 0, 100_000, TablePresence::Present), + "an idle machine with the table loaded is enforcing" + ); + // --dry-run still overrides everything: nothing is bound to the queue, + // so a loaded table filters nothing of ours. + assert!(!enforcing_heuristic(true, 0, 5, TablePresence::Present)); + } + + #[test] + fn an_absent_table_is_not_alarming_while_the_machine_is_still_starting() { + // The shipped units start this daemon before the one that loads the + // table, and the probe fires as soon as it is spawned - so on every + // single boot the first answer is "absent". Reporting that verbatim + // told the user their firewall was off for the first minute of every + // start, which is a false alarm in the one field they are pointed at. + assert!( + enforcing_heuristic(false, 0, 1, TablePresence::Absent), + "an absent table inside the grace period is a machine still coming up" + ); + assert!( + !enforcing_heuristic(false, 0, ENFORCING_GRACE_SECS + 1, TablePresence::Absent), + "past it, absent means absent" + ); + // And the grace period does not extend to a table that is there. + assert!(enforcing_heuristic(false, 0, 1, TablePresence::Present)); + } + #[test] fn enforcing_is_false_only_after_a_silent_grace_period() { // Fresh start, nothing seen yet: assume healthy. - assert!(enforcing_heuristic(false, 0, 5)); + assert!(enforcing_heuristic(false, 0, 5, TablePresence::Unknown)); // Still nothing after the grace period: the nft rule is missing. - assert!(!enforcing_heuristic(false, 0, ENFORCING_GRACE_SECS + 1)); + assert!(!enforcing_heuristic( + false, + 0, + ENFORCING_GRACE_SECS + 1, + TablePresence::Unknown + )); // Any traffic at all proves we are in the path. - assert!(enforcing_heuristic(false, 1, 100_000)); + assert!(enforcing_heuristic( + false, + 1, + 100_000, + TablePresence::Unknown + )); } #[test] fn dry_run_never_claims_to_be_enforcing() { - assert!(!enforcing_heuristic(true, 0, 5)); - assert!(!enforcing_heuristic(true, 999, 100_000)); + assert!(!enforcing_heuristic(true, 0, 5, TablePresence::Unknown)); + assert!(!enforcing_heuristic( + true, + 999, + 100_000, + TablePresence::Unknown + )); } // -- event query mapping ------------------------------------------------ diff --git a/crates/cfc-daemon/src/main.rs b/crates/cfc-daemon/src/main.rs index d0082af..38c5448 100644 --- a/crates/cfc-daemon/src/main.rs +++ b/crates/cfc-daemon/src/main.rs @@ -39,6 +39,12 @@ const RUNTIME_SHUTDOWN_GRACE: Duration = Duration::from_secs(5); /// See the warmer task in `run`. Not a hot path: the check is one `stat` of /// the package database directory, and a rebuild only happens when its mtime /// moved. +/// How often to ask nftables whether the table that feeds NFQUEUE is loaded. +/// +/// One minute, matching the fast-allow set check: both are a fork and an exec, +/// and both bound how long `cfc status` may be stale by the same amount. +const NFT_PRESENCE_INTERVAL: std::time::Duration = std::time::Duration::from_secs(60); + const PROVENANCE_WARM_INTERVAL: std::time::Duration = std::time::Duration::from_secs(120); #[derive(Debug, Parser)] @@ -270,6 +276,42 @@ async fn run() -> anyhow::Result<()> { } }); + // Is the ruleset that feeds us still loaded? + // + // `cfc status`'s `enforcing` used to be "have we ever seen a packet", and + // a counter that only goes up: after the first packet it said yes for the + // daemon's whole life, including after `nft flush ruleset` or a firewall + // reload took the table away and the machine stopped being filtered. The + // README points people at that field, and at `cfc --json status | jq + // .enforcing` for scripts, so the one indicator failed in the direction of + // false confidence. + // + // A packet counter cannot tell "nothing is filtered" from "nothing is + // happening" - an idle laptop looks identical to an unprotected one. So + // ask nftables instead. Once a minute, on the blocking pool because it is + // a fork and an exec, which is the same cadence and the same reasoning as + // the fast-allow set check that already runs there. An error leaves the + // previous answer standing: "could not ask" must never render as "the + // firewall is gone". + { + let stats = stats.clone(); + tokio::spawn(async move { + let mut tick = tokio::time::interval(NFT_PRESENCE_INTERVAL); + loop { + tick.tick().await; + match tokio::task::spawn_blocking(ebpf::nft_table_loaded).await { + Ok(Ok(present)) => stats.set_nft_table(if present { + stats::TablePresence::Present + } else { + stats::TablePresence::Absent + }), + Ok(Err(e)) => tracing::debug!("could not ask nftables for its table: {e:#}"), + Err(e) => tracing::debug!("the nftables probe did not run: {e}"), + } + } + }); + } + // Watchdog heartbeat. sd_notify::notify() no-ops without // $NOTIFY_SOCKET, so this runs harmlessly outside systemd too. The // worker stamps `last_activity` once per loop iteration and no diff --git a/crates/cfc-daemon/src/nfqueue.rs b/crates/cfc-daemon/src/nfqueue.rs index a1a4b3c..bfa67a5 100644 --- a/crates/cfc-daemon/src/nfqueue.rs +++ b/crates/cfc-daemon/src/nfqueue.rs @@ -911,7 +911,10 @@ impl ProcessResolver for ProcfsResolver { /// production. trait HostCache { fn is_self(&self, pid: u32) -> bool; - fn cached_host(&self, ip: IpAddr) -> Option; + /// The name for this address, and whether it was confirmed to belong to + /// it. Both, because the packet path needs the name for the record and the + /// confirmation for the decision, and asking twice would race. + fn cached_host(&self, ip: IpAddr) -> Option<(String, bool)>; fn enqueue(&self, ip: IpAddr); } @@ -920,8 +923,8 @@ impl HostCache for DnsCache { DnsCache::is_self(self, pid) } - fn cached_host(&self, ip: IpAddr) -> Option { - self.lookup_cached(ip) + fn cached_host(&self, ip: IpAddr) -> Option<(String, bool)> { + self.cached_named(ip) } fn enqueue(&self, ip: IpAddr) { @@ -1054,8 +1057,8 @@ fn handle_packet(payload: &[u8], meta: &PacketMeta, deps: &PipelineDeps) -> Pack } // Attach cached hostname if any, kick off a fresh lookup for next time. - if let Some(host) = deps.dns.cached_host(conn.dst_ip) { - conn = conn.with_host(host); + if let Some((host, verified)) = deps.dns.cached_host(conn.dst_ip) { + conn = conn.with_host_verified(host, verified); } deps.dns.enqueue(conn.dst_ip); @@ -1235,6 +1238,20 @@ mod tests { struct StubDns { self_pid: Option, host: Option, + /// Whether the stubbed name is confirmed against the address. `true` + /// by default so a test that only cares about the name keeps meaning + /// what it meant; a test about the trust distinction sets it. + host_verified: bool, + } + + impl Default for StubDns { + fn default() -> Self { + Self { + self_pid: None, + host: None, + host_verified: true, + } + } } impl HostCache for StubDns { @@ -1242,8 +1259,8 @@ mod tests { self.self_pid == Some(pid) } - fn cached_host(&self, _ip: IpAddr) -> Option { - self.host.clone() + fn cached_host(&self, _ip: IpAddr) -> Option<(String, bool)> { + self.host.clone().map(|h| (h, self.host_verified)) } fn enqueue(&self, _ip: IpAddr) {} @@ -1277,6 +1294,7 @@ mod tests { dns: StubDns { self_pid: None, host: None, + ..Default::default() }, resolver: StubResolver { pid: Some(4242), @@ -1841,6 +1859,65 @@ mod tests { } } + #[test] + fn an_unverified_name_may_refuse_but_may_not_admit() { + // A name the daemon lifted off the wire decorates the flow and is + // shown to the user. It may still refuse traffic - an attacker gains + // nothing by calling themselves something the user denied - but it + // must not admit any, because anything answering from source port 53 + // can assert any name for its own address. + let named = |action| { + let mut scope = cfc_core::RuleScope::any(); + scope.dst_host = Some("example.org".to_string()); + Rule::new("by-name".to_string(), action, scope) + }; + + // Allow + unverified: the rule does not answer, so the default does. + let mut env = TestEnv::new(vec![named(Action::Allow)], dp_deny()); + env.dns.host = Some("example.org".into()); + env.dns.host_verified = false; + match env.handle(&tcp_packet(443), &NO_META) { + PacketOutcome::Prompt { connection, .. } => { + assert_eq!( + connection.dst_host.as_deref(), + Some("example.org"), + "the name is still attached, for the log and the live feed" + ); + assert!(!connection.dst_host_verified); + } + other => panic!("an unverified name must not admit, got {other:?}"), + } + + // Deny + unverified: it answers, exactly as it did before. Refusing + // the name in both directions would have disarmed every deny rule + // written against a name on a host running the DNS observer. + let mut env = TestEnv::new(vec![named(Action::Deny)], dp_allow()); + env.dns.host = Some("example.org".into()); + env.dns.host_verified = false; + match env.handle(&tcp_packet(443), &NO_META) { + PacketOutcome::Deliver { verdict, .. } => { + assert_eq!(verdict.action, Action::Deny, "a deny by name still fires"); + } + other => panic!("an unverified name must still refuse, got {other:?}"), + } + + // Allow + confirmed against the address: the rule answers. + let mut env = TestEnv::new(vec![named(Action::Allow)], dp_deny()); + env.dns.host = Some("example.org".into()); + env.dns.host_verified = true; + match env.handle(&tcp_packet(443), &NO_META) { + PacketOutcome::Deliver { + connection, + verdict, + .. + } => { + assert!(connection.dst_host_verified); + assert_eq!(verdict.action, Action::Allow); + } + other => panic!("a confirmed name should have matched, got {other:?}"), + } + } + // ---- FlowKey dedup ---- fn conn_to(dst_port: u16, src_port: u16) -> Connection { @@ -2047,6 +2124,7 @@ mod tests { dns: Box::new(StubDns { self_pid: None, host: None, + ..Default::default() }), resolver: Box::new(StubResolver { pid: Some(4242), diff --git a/crates/cfc-daemon/src/reject.rs b/crates/cfc-daemon/src/reject.rs index 9bd57bd..825e0f1 100644 --- a/crates/cfc-daemon/src/reject.rs +++ b/crates/cfc-daemon/src/reject.rs @@ -49,10 +49,12 @@ //! lifetime: `Reject` behaves exactly like `Deny`. It never panics and //! never logs per packet above trace level. -use cfc_core::{Connection, Protocol}; +use cfc_core::{Connection, Direction, Protocol}; +use parking_lot::Mutex; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::os::fd::{AsRawFd, FromRawFd, OwnedFd}; use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Instant; use tracing::{debug, trace, warn}; /// IANA protocol numbers we emit. @@ -112,6 +114,9 @@ pub enum RejectOutcome { Unsupported, /// The socket existed but the kernel refused the send. SendFailed, + /// The refusal would have left the machine and the off-box budget was + /// spent. Flow is drop-only, which is what `Deny` would have done. + RateLimited, } /// Raw sockets used to inject refusals, opened once at daemon start. @@ -130,6 +135,8 @@ pub struct Rejecter { /// Latches after the first send failure so a persistently unroutable /// destination logs once at WARN and then only at DEBUG. warned_send_failure: AtomicBool, + /// Budget for refusals that leave the machine. See [`Rejecter::reject`]. + inbound_budget: Budget, } impl Rejecter { @@ -170,6 +177,7 @@ impl Rejecter { icmp4, icmp6, warned_send_failure: AtomicBool::new(false), + inbound_budget: Budget::new(INBOUND_REJECTS_PER_SEC, INBOUND_REJECT_BURST), } } @@ -183,6 +191,7 @@ impl Rejecter { icmp4: None, icmp6: None, warned_send_failure: AtomicBool::new(false), + inbound_budget: Budget::new(INBOUND_REJECTS_PER_SEC, INBOUND_REJECT_BURST), } } @@ -192,6 +201,45 @@ impl Rejecter { /// `original` is the exact packet NFQUEUE handed us, needed for the /// TCP sequence arithmetic and the ICMP quotation. pub fn reject(&self, conn: &Connection, original: &[u8]) -> RejectOutcome { + // Both endpoints have to be ordinary unicast addresses before a single + // byte is forged. The destination because it is where the refusal + // goes; the source because `IP_HDRINCL` means whatever is written + // there is what leaves the machine, and the kernel will not correct + // it. + // + // The case this exists for is not exotic. Inbound, `dst_ip` is *us* - + // and for mDNS, SSDP, LLMNR or a DHCP offer it is the group or the + // broadcast address the datagram was sent to. Refusing those produced + // an ICMP error sourced from `224.0.0.251` or `255.255.255.255`, one + // per packet, at every neighbour that speaks multicast: a martian + // source on the wire, a violation of RFC 1122 3.2.2(a) and RFC 4443 + // 2.4(e)(3), a free way to enumerate CFC hosts on a segment, and - + // because a DHCPOFFER is `ct state new` - an answer to the DHCP + // server that breaks the lease. + if !unicast_addressable(conn.src_ip) || !unicast_addressable(conn.dst_ip) { + trace!( + src = %conn.src_ip, dst = %conn.dst_ip, + "reject: not a unicast pair; dropping only" + ); + return RejectOutcome::Unsupported; + } + + // Off-box refusals are budgeted; on-box ones are not. + // + // Outbound, the refusal goes to the local application that dialled, + // over loopback, and throttling it would turn the immediate refusal + // this feature exists for back into the timeout it replaces. Inbound, + // the refusal goes to whoever sent the packet - an address taken from + // the packet - so an attacker spoofing a victim's source address turns + // this daemon into a reflector. `nft reject` cannot be used that way + // because the kernel's `icmp_send` is governed by + // `net.ipv4.icmp_ratelimit`; a raw socket with `IP_HDRINCL` is not + // governed by anything, so the budget has to be here. + if conn.direction == Direction::Inbound && !self.inbound_budget.take() { + trace!(dst = %conn.src_ip, "reject: inbound refusal budget spent; dropping only"); + return RejectOutcome::RateLimited; + } + match conn.protocol { Protocol::Tcp => self.reject_tcp(conn, original), Protocol::Udp => self.reject_udp(conn, original), @@ -592,6 +640,11 @@ fn open_raw_v4(protocol: libc::c_int) -> std::io::Result { // SAFETY: fd is a freshly created, valid descriptor we own. let fd = unsafe { OwnedFd::from_raw_fd(fd) }; set_flag(&fd, libc::IPPROTO_IP, libc::IP_HDRINCL)?; + // Best effort: a kernel that refuses the filter still sends + // correctly, it just keeps paying for receives nobody reads. + if let Err(e) = drop_all_incoming(&fd) { + debug!("could not make the raw socket send-only: {e}"); + } Ok(fd) } @@ -615,9 +668,133 @@ fn open_raw_v6(protocol: libc::c_int) -> std::io::Result { // SAFETY: fd is a freshly created, valid descriptor we own. let fd = unsafe { OwnedFd::from_raw_fd(fd) }; set_flag(&fd, libc::IPPROTO_IPV6, libc::IPV6_HDRINCL)?; + // Best effort: a kernel that refuses the filter still sends + // correctly, it just keeps paying for receives nobody reads. + if let Err(e) = drop_all_incoming(&fd) { + debug!("could not make the raw socket send-only: {e}"); + } Ok(fd) } +/// Whether an address may appear as either end of a forged refusal. +/// +/// Deliberately conservative: anything that is not a plain unicast address a +/// host could hold and answer for is refused. A subnet-directed broadcast +/// (`192.0.2.255` on a /24) cannot be recognised without knowing the mask and +/// is not covered; the limited broadcast, every multicast group, the +/// unspecified address and IPv4-mapped forms of all of those are. +fn unicast_addressable(ip: IpAddr) -> bool { + match ip { + IpAddr::V4(a) => !(a.is_multicast() || a.is_broadcast() || a.is_unspecified()), + // `to_canonical` so an IPv4-mapped v6 address is judged by its v4 + // rules: `::ffff:224.0.0.251` is multicast, and `Ipv6Addr::is_multicast` + // alone answers false for it. + IpAddr::V6(a) => match a.to_canonical() { + IpAddr::V4(v4) => !(v4.is_multicast() || v4.is_broadcast() || v4.is_unspecified()), + IpAddr::V6(v6) => !(v6.is_multicast() || v6.is_unspecified()), + }, + } +} + +/// A token bucket, monotonic and lock-light. +/// +/// One global bucket rather than one per destination: per-destination state is +/// unbounded and an attacker chooses the destinations. This is the same shape +/// the kernel uses for `icmp_ratelimit`, and it bounds what this daemon can be +/// made to emit no matter how the load is spread. +#[derive(Debug)] +struct Budget { + inner: Mutex, + per_sec: f64, + burst: f64, +} + +#[derive(Debug)] +struct BudgetState { + tokens: f64, + last: Instant, +} + +impl Budget { + fn new(per_sec: u32, burst: u32) -> Self { + Self { + inner: Mutex::new(BudgetState { + tokens: f64::from(burst), + last: Instant::now(), + }), + per_sec: f64::from(per_sec), + burst: f64::from(burst), + } + } + + /// Spends one token if there is one. Refills by elapsed time first, so a + /// quiet period is credited without a timer of its own. + fn take(&self) -> bool { + self.take_at(Instant::now()) + } + + fn take_at(&self, now: Instant) -> bool { + let mut st = self.inner.lock(); + let elapsed = now.saturating_duration_since(st.last).as_secs_f64(); + st.last = now; + st.tokens = (st.tokens + elapsed * self.per_sec).min(self.burst); + if st.tokens >= 1.0 { + st.tokens -= 1.0; + true + } else { + false + } + } +} + +/// Refusals per second this daemon will send *off the machine*, and the burst +/// it will allow. Chosen well above any legitimate inbound load on a desktop +/// or a small server - a refused scan is one packet, not twenty - and far +/// below anything worth relaying through. +const INBOUND_REJECTS_PER_SEC: u32 = 20; +const INBOUND_REJECT_BURST: u32 = 20; + +/// Refuses every packet the kernel would otherwise queue on a raw socket. +/// +/// A `SOCK_RAW` socket bound to a protocol receives a *copy of every packet of +/// that protocol delivered to this host*, whether or not anyone reads it - +/// `IP_HDRINCL` governs sends and nothing else. These four sockets exist only +/// to send, and are never read, so without this every inbound TCP segment and +/// every ICMP packet on the machine paid an `skb_clone` and an enqueue into a +/// buffer that could only ever fill and drop. A one-instruction classic BPF +/// filter that returns 0 is the standard way to say "send-only": the kernel +/// drops the packet before the clone. `SO_RCVBUF` would only shrink the waste, +/// and `shutdown(SHUT_RD)` is not honoured for raw sockets. +fn drop_all_incoming(fd: &OwnedFd) -> std::io::Result<()> { + // BPF_RET | BPF_K with k = 0: "accept 0 bytes of this packet", i.e. drop. + let mut insns = [libc::sock_filter { + code: (libc::BPF_RET | libc::BPF_K) as u16, + jt: 0, + jf: 0, + k: 0, + }]; + let prog = libc::sock_fprog { + len: 1, + filter: insns.as_mut_ptr(), + }; + // SAFETY: fd is valid and owned; `prog` and the instruction array it + // points at both live until setsockopt returns, and the length passed is + // the size of the struct the kernel expects. + let rc = unsafe { + libc::setsockopt( + fd.as_raw_fd(), + libc::SOL_SOCKET, + libc::SO_ATTACH_FILTER, + (&prog as *const libc::sock_fprog).cast(), + std::mem::size_of::() as libc::socklen_t, + ) + }; + if rc < 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) +} + fn set_flag(fd: &OwnedFd, level: libc::c_int, name: libc::c_int) -> std::io::Result<()> { let enable: libc::c_int = 1; // SAFETY: fd is valid and owned; the option value points at a properly @@ -780,6 +957,169 @@ mod tests { Connection::new(protocol, Direction::Outbound, src, APP_PORT, dst, PEER_PORT) } + fn inbound(protocol: Protocol, peer: IpAddr, us: IpAddr) -> Connection { + Connection::new(protocol, Direction::Inbound, peer, PEER_PORT, us, APP_PORT) + } + + // ---- who may appear on a forged refusal ---- + + #[test] + fn only_plain_unicast_addresses_may_carry_a_refusal() { + for good in [ + "192.0.2.10", + "127.0.0.1", + "10.0.0.1", + "2001:db8::1", + "::1", + "::ffff:192.0.2.10", + ] { + assert!( + unicast_addressable(good.parse().unwrap()), + "{good} should be addressable" + ); + } + for bad in [ + // The four shapes that produced a martian source in the field: + // mDNS, SSDP, the limited broadcast a DHCP offer carries, and the + // v6 multicast mDNS uses. + "224.0.0.251", + "239.255.255.250", + "255.255.255.255", + "0.0.0.0", + "ff02::fb", + "::", + // And the mapped form, which `Ipv6Addr::is_multicast` alone + // answers `false` for - the reason this helper canonicalises. + "::ffff:224.0.0.251", + "::ffff:255.255.255.255", + ] { + assert!( + !unicast_addressable(bad.parse().unwrap()), + "{bad} must never carry a refusal" + ); + } + } + + #[test] + fn a_multicast_destined_datagram_is_never_answered() { + let r = Rejecter::disabled(); + // A real datagram, so the ICMP quotation can be built and the guard is + // the only thing that can refuse: with an empty buffer this test would + // pass for the wrong reason. + let udp = ipv4_packet(17, &udp_header(0)); + // An mDNS query arriving at the group address. Before the guard this + // produced an ICMP port-unreachable sourced from 224.0.0.251. + let c = inbound( + Protocol::Udp, + "192.0.2.50".parse().unwrap(), + "224.0.0.251".parse().unwrap(), + ); + assert_eq!(r.reject(&c, &udp), RejectOutcome::Unsupported); + // The same peer to our real address gets past the guard and stops at + // the missing socket, which is the next check along. + let c = inbound( + Protocol::Udp, + "192.0.2.50".parse().unwrap(), + "192.0.2.10".parse().unwrap(), + ); + assert_eq!(r.reject(&c, &udp), RejectOutcome::Unavailable); + } + + #[test] + fn the_send_only_filter_really_drops_what_arrives() { + // `drop_all_incoming` is what stops four raw sockets receiving a copy + // of every TCP segment and every ICMP packet on the machine for the + // daemon's whole life. It cannot be exercised on a raw socket without + // CAP_NET_RAW, but `SO_ATTACH_FILTER` is not raw-specific: a UDP + // socket answers the same question - is this filter well formed, and + // does the kernel really drop on a zero return? + use std::net::UdpSocket; + use std::os::fd::AsFd; + + let listener = UdpSocket::bind("127.0.0.1:0").expect("bind"); + let addr = listener.local_addr().expect("addr"); + listener + .set_read_timeout(Some(std::time::Duration::from_millis(250))) + .expect("timeout"); + let sender = UdpSocket::bind("127.0.0.1:0").expect("bind sender"); + + // Without the filter the datagram arrives: this half is the control, + // and without it a broken filter and a broken test look identical. + sender.send_to(b"before", addr).expect("send"); + let mut buf = [0u8; 16]; + let n = listener + .recv(&mut buf) + .expect("the control datagram arrives"); + assert_eq!(&buf[..n], b"before"); + + // SAFETY-adjacent: `drop_all_incoming` takes a borrowed fd and only + // calls setsockopt on it. + let owned = listener.as_fd().try_clone_to_owned().expect("dup"); + drop_all_incoming(&owned).expect("the kernel accepts the filter"); + + sender.send_to(b"after", addr).expect("send"); + assert!( + listener.recv(&mut buf).is_err(), + "with the filter attached nothing may be delivered" + ); + } + + // ---- the off-box budget ---- + + #[test] + fn the_budget_spends_its_burst_then_refills_with_time() { + let b = Budget::new(10, 3); + let t0 = Instant::now(); + assert!(b.take_at(t0), "first of the burst"); + assert!(b.take_at(t0), "second"); + assert!(b.take_at(t0), "third"); + assert!(!b.take_at(t0), "the burst is spent"); + // 10/s, so 100 ms buys exactly one. + assert!(b.take_at(t0 + std::time::Duration::from_millis(100))); + assert!(!b.take_at(t0 + std::time::Duration::from_millis(100))); + // A long quiet period refills to the burst and no further. + let later = t0 + std::time::Duration::from_secs(60); + for i in 0..3 { + assert!(b.take_at(later), "refilled token {i}"); + } + assert!(!b.take_at(later), "refill is capped at the burst"); + } + + #[test] + fn only_refusals_that_leave_the_machine_are_budgeted() { + let peer: IpAddr = "192.0.2.50".parse().unwrap(); + let us: IpAddr = "192.0.2.10".parse().unwrap(); + let udp = ipv4_packet(17, &udp_header(0)); + + // Outbound refusals go to the local application over loopback and are + // never throttled: throttling them would restore the timeout this + // feature exists to replace. + let r = Rejecter::disabled(); + for i in 0..(INBOUND_REJECT_BURST * 4) { + assert_eq!( + r.reject(&conn(Protocol::Udp, us, peer), &udp), + RejectOutcome::Unavailable, + "outbound refusal {i} must not be budgeted" + ); + } + + // Inbound ones are. The burst goes through (as far as the missing + // socket), then the budget answers instead. + let r = Rejecter::disabled(); + for i in 0..INBOUND_REJECT_BURST { + assert_eq!( + r.reject(&inbound(Protocol::Udp, peer, us), &udp), + RejectOutcome::Unavailable, + "inbound refusal {i} is within the burst" + ); + } + assert_eq!( + r.reject(&inbound(Protocol::Udp, peer, us), &udp), + RejectOutcome::RateLimited, + "past the burst the refusal is dropped, not sent" + ); + } + // ---- checksum helper ---- #[test] diff --git a/crates/cfc-daemon/src/sock_diag.rs b/crates/cfc-daemon/src/sock_diag.rs index c8a2329..aeb10d3 100644 --- a/crates/cfc-daemon/src/sock_diag.rs +++ b/crates/cfc-daemon/src/sock_diag.rs @@ -56,16 +56,14 @@ pub fn query( IpAddr::V6(_) => libc::AF_INET6 as u8, }; - let sock = match DiagSocket::open() { - Ok(s) => s, - Err(e) => { - trace!("sock_diag socket unavailable ({e}); falling back to /proc"); - return None; - } - }; - - let req = build_request(family, proto_num, (src_ip, src_port), (dst_ip, dst_port)); - if let Some(info) = sock.round_trip(&req) { + let req = build_request( + family, + proto_num, + (src_ip, src_port), + (dst_ip, dst_port), + next_seq(), + ); + if let Some(info) = ask(&req) { return Some(info); } @@ -75,18 +73,98 @@ pub fn query( // for unconnected sockets the kernel may still miss, in which case // the /proc scan's zero-remote pass takes over. if protocol == Protocol::Udp { - let req = build_request(family, proto_num, (dst_ip, dst_port), (src_ip, src_port)); - return sock.round_trip(&req); + let req = build_request( + family, + proto_num, + (dst_ip, dst_port), + (src_ip, src_port), + next_seq(), + ); + return ask(&req); } None } +thread_local! { + /// One netlink socket per thread, for the life of the thread. + /// + /// This used to be one `socket(2)` + `setsockopt(2)` + `close(2)` per + /// queued packet, on the single datapath thread. Measured on the veth + /// bench in `scripts/vm-bench`, that churn cost 0.28 ms of every queued + /// flow at 3000 flows - small beside the 5 ms idle beat, and pure waste. + /// + /// Reuse is only safe because the sequence number below is unique per + /// request and checked on the way back. Every request used to carry + /// `seq = 1`, so a late answer to a request that had already timed out + /// would have been indistinguishable from the answer to the next one - + /// a *wrong attribution*, which is far worse than a slow one. + static DIAG_SOCKET: std::cell::RefCell> = + const { std::cell::RefCell::new(None) }; + /// Monotonic per-thread request counter. Starts at 1 because 0 is + /// conventionally "not a reply to anything". + static DIAG_SEQ: std::cell::Cell = const { std::cell::Cell::new(1) }; +} + +/// The next request's sequence number, wrapping past `u32::MAX` back to 1. +fn next_seq() -> u32 { + DIAG_SEQ.with(|c| { + let seq = c.get(); + c.set(seq.checked_add(1).unwrap_or(1)); + seq + }) +} + +/// One request on this thread's socket, opening it if needed. +/// +/// The socket is discarded on anything but a clean, correctly-sequenced +/// answer. A socket that timed out may still have that answer queued behind +/// it, and there is no way to tell how much else is queued with it; throwing +/// the socket away throws the ambiguity away too, at the cost of one +/// `socket(2)` on the next call. +fn ask(req: &[u8; REQ_LEN]) -> Option { + let seq = u32::from_ne_bytes(req[8..12].try_into().ok()?); + DIAG_SOCKET.with(|cell| { + let mut slot = cell.borrow_mut(); + if slot.is_none() { + match DiagSocket::open() { + Ok(s) => *slot = Some(s), + Err(e) => { + trace!("sock_diag socket unavailable ({e}); falling back to /proc"); + return None; + } + } + } + match slot.as_ref().map(|s| s.round_trip(req, seq)) { + Some(Reply::Found(info)) => Some(info), + // A correctly-sequenced "no such socket". The socket is clean, so + // it is kept; the caller falls back to /proc as before. + Some(Reply::NotFound) => None, + _ => { + *slot = None; + None + } + } + }) +} + +/// What one round trip produced, split so the caller knows whether the socket +/// is still trustworthy. +enum Reply { + Found(SockInfo), + /// The kernel answered this request and had nothing to report. + NotFound, + /// Send failed, receive failed, or the answer was to some other request. + /// The socket's state is unknown from here. + Desync, +} + /// Serialize nlmsghdr + inet_diag_req_v2 for an exact (non-dump) query. fn build_request( family: u8, protocol: u8, local: (IpAddr, u16), remote: (IpAddr, u16), + seq: u32, ) -> [u8; REQ_LEN] { let mut buf = [0u8; REQ_LEN]; @@ -94,8 +172,8 @@ fn build_request( buf[0..4].copy_from_slice(&(REQ_LEN as u32).to_ne_bytes()); buf[4..6].copy_from_slice(&SOCK_DIAG_BY_FAMILY.to_ne_bytes()); buf[6..8].copy_from_slice(&(libc::NLM_F_REQUEST as u16).to_ne_bytes()); - buf[8..12].copy_from_slice(&1u32.to_ne_bytes()); // seq - // nlmsg_pid stays 0 (kernel). + buf[8..12].copy_from_slice(&seq.to_ne_bytes()); + // nlmsg_pid stays 0 (kernel). // struct inet_diag_req_v2. buf[16] = family; @@ -124,6 +202,13 @@ fn write_addr(dst: &mut [u8], ip: IpAddr) { } /// Parse the first netlink message of a reply. Exact (non-dump) queries +/// The `nlmsg_seq` of a netlink message, or `None` if there is no header. +fn reply_seq(buf: &[u8]) -> Option { + buf.get(8..12) + .and_then(|b| b.try_into().ok()) + .map(u32::from_ne_bytes) +} + /// answer with a single SOCK_DIAG_BY_FAMILY message or an NLMSG_ERROR. fn parse_response(buf: &[u8]) -> Option { if buf.len() < NLMSG_HDR_LEN { @@ -193,7 +278,7 @@ impl DiagSocket { Ok(sock) } - fn round_trip(&self, req: &[u8]) -> Option { + fn round_trip(&self, req: &[u8], seq: u32) -> Reply { let fd = self.0.as_raw_fd(); // SAFETY: zeroed sockaddr_nl is a valid "to the kernel" address. @@ -217,7 +302,7 @@ impl DiagSocket { "sock_diag send failed ({}); falling back to /proc", std::io::Error::last_os_error() ); - return None; + return Reply::Desync; } let mut buf = [0u8; 8192]; @@ -228,9 +313,20 @@ impl DiagSocket { "sock_diag recv failed ({}); falling back to /proc", std::io::Error::last_os_error() ); - return None; + return Reply::Desync; + } + let buf = &buf[..n as usize]; + // The answer must be to *this* request. Anything else means a previous + // request's answer arrived after its timeout, and this socket cannot + // be trusted to be at a message boundary any more. + if reply_seq(buf) != Some(seq) { + trace!("sock_diag answered a different request; discarding the socket"); + return Reply::Desync; + } + match parse_response(buf) { + Some(info) => Reply::Found(info), + None => Reply::NotFound, } - parse_response(&buf[..n as usize]) } } @@ -239,11 +335,54 @@ mod tests { use super::*; use std::net::{Ipv4Addr, SocketAddr, UdpSocket}; + #[test] + fn each_request_carries_its_own_sequence_number() { + // The whole reason the socket may be reused: two requests must never + // be confusable. Before this, every request carried seq 1. + let a = next_seq(); + let b = next_seq(); + assert_ne!(a, b, "two requests must not share a sequence number"); + assert_eq!(b, a + 1); + + let local = (IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 1); + let remote = (IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)), 2); + let req = build_request( + libc::AF_INET as u8, + libc::IPPROTO_TCP as u8, + local, + remote, + 0xABCD, + ); + assert_eq!( + reply_seq(&req), + Some(0xABCD), + "the seq is where the reply check reads it" + ); + } + + #[test] + fn an_answer_to_another_request_is_not_trusted() { + // A reply whose sequence does not match is the shape a late answer to + // a timed-out request takes. `reply_seq` is what tells them apart, and + // a short buffer has no sequence at all. + let mut reply = [0u8; 16]; + reply[8..12].copy_from_slice(&7u32.to_ne_bytes()); + assert_eq!(reply_seq(&reply), Some(7)); + assert_ne!(reply_seq(&reply), Some(8)); + assert_eq!(reply_seq(&[0u8; 4]), None); + } + #[test] fn request_serialization_layout() { let local = (IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080); let remote = (IpAddr::V4(Ipv4Addr::new(93, 184, 216, 34)), 443); - let req = build_request(libc::AF_INET as u8, libc::IPPROTO_TCP as u8, local, remote); + let req = build_request( + libc::AF_INET as u8, + libc::IPPROTO_TCP as u8, + local, + remote, + 1, + ); // nlmsghdr. assert_eq!(u32::from_ne_bytes(req[0..4].try_into().unwrap()), 72); @@ -276,7 +415,13 @@ mod tests { fn request_serialization_v6_addresses() { let local = (IpAddr::V6("2001:db8::1".parse().unwrap()), 1); let remote = (IpAddr::V6("::1".parse().unwrap()), 2); - let req = build_request(libc::AF_INET6 as u8, libc::IPPROTO_UDP as u8, local, remote); + let req = build_request( + libc::AF_INET6 as u8, + libc::IPPROTO_UDP as u8, + local, + remote, + 1, + ); assert_eq!( &req[28..44], &[0x20, 0x01, 0x0D, 0xB8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1] diff --git a/crates/cfc-daemon/src/stats.rs b/crates/cfc-daemon/src/stats.rs index dc96a3f..833d68f 100644 --- a/crates/cfc-daemon/src/stats.rs +++ b/crates/cfc-daemon/src/stats.rs @@ -1,6 +1,6 @@ //! Runtime counters shared across daemon components. -use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, AtomicU8, Ordering}; use std::sync::Arc; use std::time::Instant; @@ -20,6 +20,45 @@ struct StatsInner { /// auto-unpause timer can be invalidated if the user toggles in the /// meantime. pause_generation: AtomicU64, + /// Last answer from the nftables probe: see [`TablePresence`]. + /// + /// Starts `Unknown` and stays there on any host where `nft` cannot be + /// asked, which is what keeps a failed probe from reading as "the + /// firewall is gone". + nft_table: AtomicU8, +} + +/// What the periodic nftables probe last found. +/// +/// Three states, not two, and the third is the point: "could not ask" has to +/// be distinguishable from "asked, and the table is not there". Reporting the +/// second when the first happened would call a healthy machine unprotected +/// every time a fork failed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TablePresence { + /// Never probed, or the probe could not run. + Unknown, + /// `table inet colony_firewall` is loaded. + Present, + /// It is not. Nothing reaches the queue; nothing is being filtered. + Absent, +} + +impl TablePresence { + fn as_u8(self) -> u8 { + match self { + Self::Unknown => 0, + Self::Present => 1, + Self::Absent => 2, + } + } + fn from_u8(v: u8) -> Self { + match v { + 1 => Self::Present, + 2 => Self::Absent, + _ => Self::Unknown, + } + } } impl Stats { @@ -33,6 +72,7 @@ impl Stats { prompts_pending: AtomicU64::new(0), paused: AtomicBool::new(false), pause_generation: AtomicU64::new(0), + nft_table: AtomicU8::new(TablePresence::Unknown.as_u8()), }), } } @@ -59,6 +99,18 @@ impl Stats { self.inner.prompts_pending.fetch_sub(1, Ordering::Relaxed); } + /// Records what the nftables probe found. + pub fn set_nft_table(&self, presence: TablePresence) { + self.inner + .nft_table + .store(presence.as_u8(), Ordering::Relaxed); + } + + /// The probe's last answer. + pub fn nft_table(&self) -> TablePresence { + TablePresence::from_u8(self.inner.nft_table.load(Ordering::Relaxed)) + } + pub fn uptime_seconds(&self) -> u64 { self.inner.started.elapsed().as_secs() } From aec3bcd92a966858bc10ef0efb4be88e9ddd30b3 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Sun, 6 Sep 2026 15:25:24 +0200 Subject: [PATCH 4/4] docs(hardening): the DNS section described a risk smaller than the one it has The section on observed answers framed a forgery as something an attacker must win against the resolver - "the same attacker who could also forge the forward lookup FCrDNS depends on". That is a race for a transaction id and an ephemeral port, and it is not what the code allows. The kernel gate is `source port == 53` and nothing else: any peer the host sends a UDP datagram to can answer from that port and assert any name for any address, with no spoofing, no guessing, and no involvement from the resolving library at all. Rewritten to say that, and to say what now follows from it: such a name may refuse but not admit. The `dst_host` warning above it gains the same note, because that advice - do not lean on a hostname allow rule - is now enforced rather than only given. Also here, found by reading the whole diff back rather than by a test: the fast path's own event consumer named its flows with `with_host`, which marks a name unverified whatever it is. Those connections never reach the matcher, so nothing was decided on it, but a field that says "nobody confirmed this" about a name that was confirmed is a trap for the next reader. It uses the same seam the packet path does. --- CHANGELOG.md | 8 +++++++ crates/cfc-daemon/src/ebpf/loader.rs | 4 ++-- docs/HARDENING.md | 34 ++++++++++++++++++++++------ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1f7a66..aa05568 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,14 @@ and [Semantic Versioning](https://semver.org/spec/v2.0.0.html). syscalls - on the sixteen-slot blocking pool the prompt router also uses. Rules now refuse a path longer than `PATH_MAX`, which no process could match anyway. The rejection deliberately does not echo the path back. +- **`docs/HARDENING.md` understated the DNS risk it described.** It framed a + forged observed answer as something an attacker must race the resolver for, + "the same attacker who could also forge the forward lookup FCrDNS depends + on". That is not the shape: nothing correlates an observed response to a + query this host sent, so any peer the host sends a datagram to can reply + from source port 53, with no spoofing and no guessing, and the application's + own resolver never sees it. The section now says so, and says what the + daemon does about it. - **`ListEvents` skipped an unbounded number of rows.** `limit` was clamped and `offset` was not, so a read-only peer could make sqlite step and discard the whole event table per call, holding the global connection mutex. diff --git a/crates/cfc-daemon/src/ebpf/loader.rs b/crates/cfc-daemon/src/ebpf/loader.rs index efc7437..afbaceb 100644 --- a/crates/cfc-daemon/src/ebpf/loader.rs +++ b/crates/cfc-daemon/src/ebpf/loader.rs @@ -1582,8 +1582,8 @@ pub(super) fn load_and_attach( dst.ip(), dst.port(), ); - if let Some(host) = dns_hosts.lookup_cached(dst.ip()) { - connection = connection.with_host(host); + if let Some((host, verified)) = dns_hosts.cached_named(dst.ip()) { + connection = connection.with_host_verified(host, verified); } dns_hosts.enqueue_lookup(dst.ip()); let process = crate::process_resolve::resolve(ev.pid); diff --git a/docs/HARDENING.md b/docs/HARDENING.md index d6ffca4..9b699dd 100644 --- a/docs/HARDENING.md +++ b/docs/HARDENING.md @@ -131,6 +131,12 @@ lean on a hostname *allow* rule as your only boundary. For allow rules, pin `exe` + `dst_port` (+ `dst_net` where destinations are stable) instead. +That advice is now enforced rather than only given: a name the daemon +did not confirm against the address may **refuse** traffic but may not +**admit** it. A `deny --dst-host` behaves exactly as it +always did; an `allow --dst-host` stands aside and lets the rules +beneath it answer. + #### Observed answers, with `[ebpf] enabled` Turning the eBPF layer on adds a second, better source. The @@ -147,13 +153,27 @@ was told an address, *before* the connection it explains, by the zone that owns the name. The "hostile server names itself `api.github.com`" problem does not arise, because the destination no longer gets a vote. -What it does not fix: the program reads packets off the wire, before the -resolving library's transaction-id and source-port checks. Anything -arriving from source port 53 that parses as a response is observed, -including a forgery that the resolver will go on to reject - and that is -the same attacker who could also forge the forward lookup FCrDNS -depends on. Observed answers raise the bar; they do not make a hostname -allow rule a boundary. The advice above is unchanged. +What it does not fix, stated more plainly than it was: **nothing ties an +observed response to a query this host sent.** The kernel gate is +`source port == 53` and no more - the transaction id is parsed and never +compared, the sender's address is never checked against a configured +resolver, and the answer's owner name is never compared with the +question. So this is not a forgery race that an attacker must win +against the resolver, as this paragraph used to imply. Any peer the host +sends a UDP datagram to - a game server, a STUN peer, anything - can +reply from source port 53 and assert any name for any address. No +spoofing, no guessing, and the application's own resolver never sees the +packet. + +That is why an observed answer decorates a flow but does not admit it: +it may satisfy a `deny --dst-host` and never an `allow --dst-host`. +They remain the better source for *naming* a flow in the log, the live +feed and a prompt, which is what they are for. + +The full remedy is to check the sender against the resolvers this host +actually uses. That needs the source address in the record the kernel +copies up, and therefore an ABI bump; until then the asymmetry above is +what stands between an observed name and a decision. Answers are cached for the record's own TTL, clamped to 60s..1h.