diff options
| author | Calvin Morrison <calvin@pobox.com> | 2026-08-18 23:22:58 -0400 |
|---|---|---|
| committer | Calvin Morrison <calvin@pobox.com> | 2026-08-18 23:22:58 -0400 |
| commit | 1575b83f102060eab99248c8e7660e9f61df1389 (patch) | |
| tree | 26901caeddb9c2133925600a255987e8e0aac6d7 /fw/doc | |
| parent | 8f5fed654d54a9ae9c4eb95425d857e9524b803c (diff) | |
doc: fragments cross, callers are checked, and the open list is shorter
man/fw said a fragmented datagram does not cross. IPv4 ones now do:
the first piece decides and the train inherits. IPv6 fragments still
do not, because the extension header they live in is not walked, and
that is what the paragraph says now.
The altitude paragraph said request filtering cannot stop an inbound
connection before the handshake. Still true, and still worth saying,
but it now refuses the connection to the program and hangs it up, which
is the difference between a rule that is late and one that does
nothing. fwrules(6) gains the other half of that: an in rule naming an
ip cannot decide an announce, and decides the connection instead.
todo.md loses the six items that are now fixed and gains a third round.
One of them is a correction rather than a fix: the interface tables
being read into fixed buffers was reported as losing the default route,
and it was not -- routes come out sorted and 0.0.0.0 sorts first, so it
was always on the first line. Worth removing the limits anyway; not
worth having claimed it broke something.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Diffstat (limited to 'fw/doc')
| -rw-r--r-- | fw/doc/design.md | 5 | ||||
| -rw-r--r-- | fw/doc/todo.md | 191 |
2 files changed, 88 insertions, 108 deletions
diff --git a/fw/doc/design.md b/fw/doc/design.md index c45c86b..c14787c 100644 --- a/fw/doc/design.md +++ b/fw/doc/design.md @@ -179,7 +179,8 @@ filtering is also just correct design — it is why iptables has separate ## What it cannot do It filters connections and packets, not flows over time: no rate limits, -no fragment logic, no ICMP type matching, and IPv6 extension headers are -not walked. Forwarded traffic is invisible to the namespace mode by +no rate limits, no ICMP type matching, and IPv6 extension headers are +not walked — which also means IPv6 fragments do not cross, though IPv4 +ones do, the first piece deciding for the train. Forwarded traffic is invisible to the namespace mode by construction — it never becomes a ctl write, because no local program asked for it. diff --git a/fw/doc/todo.md b/fw/doc/todo.md index bf4bcda..de5eb8e 100644 --- a/fw/doc/todo.md +++ b/fw/doc/todo.md @@ -1,16 +1,14 @@ # fw: open items Most of this came from code review. Where a finding has been fixed it -says so and how it was checked; where it has not, it says what is -actually true. An item never appears in both halves. +says so; where it has not, it says what is actually true. An item never +appears in both halves. -`test/fwtest.rc` is a check for every bug that has shipped here. Run it -twice in a row after touching anything. +`test/fwtest.rc` is a check for every bug that has shipped here — 72 of +them now. Run it twice in a row after touching anything. ## Fixed, first round -Verified as described in the commit; one line each here. - 1. **The ctl filter was a blacklist.** `headers` on a udp conversation passed, and the write queue is live from clone, so three writes sent a datagram anywhere. Now a whitelist of control messages. @@ -27,48 +25,65 @@ Verified as described in the commit; one line each here. 8. **Ports were read out of non-first fragments.** 9. **`/net/ndb` and `/net/log` were writable and `ipifc/*/data` readable** through the filtered `/net`. -10. **Control files were world-writable.** Now 0600/0440, owned by - whoever runs `fw` — owning them as a nonexistent user "fw" locked - out the administrator too. +10. **Control files were world-writable.** 11. **`delete 0` and `delete foo`** appended a rule reading `<nil>`. 12. **A dead relay left half a firewall.** Now `threadexitsall`. -13. **IPv6 unicast under `-e`** was dropped in silence. Still not - implemented, but it says so now. +13. **IPv6 unicast under `-e`** was dropped in silence. ## Fixed, second round 1. **The path filter was still a blacklist, and still had a hole.** - `/net/tcp/trans` and its siblings install kernel address - translations; devip gates them with `iseve()`, which through this - server is `fw`'s identity and not the caller's, so on a machine - where `fw` runs as eve there was no gate. Demonstrated, then - replaced: nothing is served unless it is named. `/net/log` was half - closed the same way — the write was refused when reading it was the - leak — and `ipifc/*/snoop` is the same wire as `data` and was never - mentioned. Sixteen new checks; six of them fail against the old - `netfs.c`. -2. **The suite poisoned its own next run.** Six `fw` processes and - their pkt interfaces survived every run, and the next run's stack - routed out the dead one. It failed deterministically on the second - run and a comment blamed timing, which is worse than the leak: it - told the reader to disbelieve a real result. Now torn down by - unbinding the interfaces, with two checks at the end that count what - is left. -3. **Checks that passed for the wrong reason.** A "refused" result was - returned for any failure at all, so a check for a hole passed on a - kernel that never had the hole. Each is now paired with one asking - whether the thing being refused exists. Invisibility is asked by - stat, not by read: reading `log` or a `data` file blocks, so the - read version would hang on exactly the build that still serves them. -4. **The dead code from item 5 above was not actually deleted** — only - its call sites were, and the comment meant to be moved was copied, - so the file carried the same twelve lines twice. + `/net/*/trans` installs kernel address translations, gated by + `iseve()` — which through this server is `fw`'s identity, not the + caller's. Replaced with a list of what is served. `/net/log` was + half closed the same way, and `ipifc/*/snoop` is the same wire as + `data`. +2. **The suite poisoned its own next run**, and a comment blamed + timing. Six `fw` processes and their interfaces survived every run. +3. **Checks that passed for the wrong reason**: "refused" was returned + for any failure, so a check for a hole passed on a kernel that never + had the hole. +4. **The dead code from item 5 above was not actually deleted.** 5. **Two compiler warnings** had stood since the program was written. - The suite now builds from clean and asks the compiler whether it had - anything to say. -6. **The frame buffer is the caller's**, sized from the same `Maxpkt` - that sizes the read, rather than a 16KB automatic on a 32KB proc - stack. +6. **The frame buffer belongs to the caller**, not to a proc stack. + +## Fixed, third round + +1. **matchrule returned things whose lifetime it did not own** — a + `Rule*` and a static error buffer, both read after it released the + lock, so a rule set installed in between freed one and two procs + deciding at once overwrote the other. Now a `Match` in the caller's + frame. `netfs.c` had its own static buffer with the same problem. +2. **revalidate counted as traffic.** Re-checking live flows after an + edit charged each one to the rule that matched, so `stats` answered + a different question after every edit. +3. **A rule set was 64 kilobytes long.** `fmtrules` clipped, and since + every ctl edit formats the set out and parses it back, editing a set + past the limit truncated the *rules*: 2000 in, 1214 out, and the + next edit failed. `flows` had the same cap. +4. **A fragmented datagram did not cross.** Later fragments matched no + port and every rule set is written in ports: measured, one fragment + passed and two dropped. The first fragment now decides and the train + inherits. +5. **`deny=in ip=...` was a no-op in namespace mode.** An announce + names no peer, so rules naming one were skipped and nothing asked + again. The peer is now checked at listen time, and a refused caller + is hung up rather than handed to the program. +6. **`fwstart` had never worked.** `mntgen /mnt/fw` was run only when + `/mnt/fw` was missing, which is the one case where mount(2) fails, + so the control directories never appeared. It also left its servers + holding the console. +7. **The log named an address without its port** — `getfields` writes + over the separators it splits on. +8. **relay() and permitted() were the same logic twice**, already + drifted apart in their debug output. +9. **The interface tables were read into fixed buffers** and searched + as blobs rather than parsed. Not the defect it was reported as — the + default route sorts first, so it was always found — but the device + name was matched against the whole status text, addresses included. +10. **Promiscuous mode had no filter behind it.** The card must be + promiscuous for multicast, but every neighbour's unicast was then + judged, counted and flow-tracked as if it were ours. ## Still open @@ -85,78 +100,42 @@ answer is not more note handling — it has to be something that outlives `fw`, which means the supervisor, with the address stored where `fw` does not own it. This also blocks `svc` supervision. -### 2. deny=in ip=... silently never matches in namespace mode - -An `announce` sets `anyip`, and `matchrule` skips rules naming an -address, so a peer-address inbound rule is a no-op there while working -at the packet layer. It is a reasoned decision, but by this project's -own standard it should either be enforced at listen time (check -`remote`, hang up) or refused at startup in that mode. - -### 3. A fragmented datagram does not cross - -Later fragments carry no transport header, so they match no port, and -every rule set in `fwrules`(6) is written in ports. The first fragment -crosses and the rest are denied, leaving the receiver holding an -incomplete train until it times out. Reading ports out of them, which -is what happened before, was worse — a crafted fragment whose payload -bytes matched an open flow went through — so this is the right trade, -but it is a hole in what works, not only in what is checked. - -Tying later fragments to the first would mean keying on -(protocol, source, destination, id) and letting the first fragment's -verdict stand for the rest. - -## Worth fixing - -- **`promiscuous` injects neighbours' unicast into the protected - stack.** `ethermux` already delivers frames addressed to us plus - multicast and broadcast; promiscuous only adds other machines' - traffic, into the rules, the stats and the flow table. It is needed - for multicast reception, so keep it and filter on the destination MAC - in `etherin`: accept `ourmac` or `d[0]&1`. -- **Use-after-free of the matched rule.** `matchrule` returns `*rp` - after dropping `rulelock` and callers read `rule->log` outside it. - Copy the flag out under the lock. -- **`learnaddr` truncates the routing table** — `buf[1024]`, `lines[8]`, - against a table that routinely exceeds both, so the default route can - be missed. -- **`fmtrules` truncates silently at 64K**, so a large rule set loses - rules on every ctl edit and on save. Detect and fail. -- **Static error buffers race under `srvrelease`** — `netfs.c` and - `rules.c` both return `static char err[128]` while running multi-proc. - Worst case is a wrong diagnostic, which is the thing namespace mode - exists to produce. -- **`revalidate` inflates hit counts** — it calls `matchrule`, so - `stats` counts rule-change re-checks as decisions about traffic. - -## Smaller - -- `relay()` and `permitted()` are the same decision logic twice with - divergent log text; the gateway path should call `permitted()`. -- `fwstart`'s `mntgen` inherits the caller's fds — the trap CLAUDE.md - documents. -- `fw` daemonizes, so `svc` cannot supervise it. Needs a foreground - mode; `ready=srv:name` fits, since `-s` already posts to `/srv`. - `fwstart` is the wrong shape and should probably go. -- One card per fw, and rules cannot name a card. Wants repeatable `-e` - and an `ifc=` attribute together. -- `Tflush` is not implemented. Only affects namespace mode. ~80 lines, - with a race that cannot be fully closed. -- Positional `delete n` renumbers. +### 2. fw daemonizes, so svc cannot supervise it + +Needs a foreground mode; `ready=srv:name` fits, since `-s` already +posts to `/srv`. `fwstart` is the wrong shape and should probably go — +one service per card is the right one. + +### 3. One card per fw, and rules cannot name a card + +Wants repeatable `-e` and an `ifc=` attribute, together. + +### 4. Tflush is not implemented + +A request `fw` is blocked on cannot be abandoned. Only affects +namespace mode, where waiting for an inbound connection is the one +operation that blocks indefinitely. ~80 lines, with a race that cannot +be fully closed: between the syscall returning and the handler clearing +its entry, a note may land on a worker that has moved on. + +### 5. Positional `delete n` renumbers + +Inherent to positional deletion; iptables has it too. Said out loud +here rather than fixed. ## Never tested -- **Card mode**, by the suite, on purpose: it takes the machine's card - away and a test that can leave you with no network is a test nobody - runs. A unit harness for `ether.c` — point `efd` at a pipe, call - `etherwriteip`, read the frame back — would cover the frame path and - the broadcast mapping below without touching a real card. +- **The wire side of card mode.** The suite can take a spare card — it + does, and everything up to the wire is now covered — but it cannot + make a neighbour send to it. Anything that depends on another machine + on the same segment is unproven: the destination-address filter, ARP + against a real peer, broadcast. - **Broadcast handling.** Written, reviewed, never observed crossing `fw`. The mapping is the standard one and normal traffic is unaffected. - **IPv6 traffic**, in any mode. Under `-e` it cannot work at all: - there is no neighbour discovery, so v6 unicast is dropped. + there is no neighbour discovery, so v6 unicast is dropped. IPv6 + extension headers are not walked, so v6 fragments do not cross. - **The gateway with two real machines**, and a real second NIC carrying real traffic. |
