diff --git a/CHANGELOG.md b/CHANGELOG.md index a76b871..aa05568 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,136 @@ 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. +- **`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. + +### 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 + 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-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/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/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 a9590d3..bfa67a5 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}; @@ -900,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); } @@ -909,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) { @@ -1043,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); @@ -1224,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 { @@ -1231,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) {} @@ -1266,6 +1294,7 @@ mod tests { dns: StubDns { self_pid: None, host: None, + ..Default::default() }, resolver: StubResolver { pid: Some(4242), @@ -1830,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 { @@ -2036,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() } 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 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. 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"