# 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. `test/fwtest.rc` is a check for every bug that has shipped here. 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. 2. **`%M` was never installed**, so `fmtrules` emitted `ipmask=%M%` and every ctl edit on a rule set containing `ip=` failed. 3. **`parserules` built the new list in the globals** with no lock, so relays walked an empty then half-built list during any reload. 4. **A 64KB frame on a 32KB proc stack** in `etherwriteip`. 5. **`putback`/`notehandler` were dead code.** `atexit` matches on the registering pid and `_exits` never runs the handlers. 6. **Expired flows kept matching**: `flowlook` refreshed `last` without checking the timeout, and reaping only happens on insert. 7. **The pkt interface claimed a 4096 MTU** from a 1514-byte card. 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. 11. **`delete 0` and `delete foo`** appended a rule reading ``. 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. ## 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. 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. ## Still open ### 1. Nothing puts the card back Taking a card is destructive and is not undone. The pkt interface is `unbindonclose`, so when `fw` stops the interface and the address both go, leaving the card bound to nothing and the machine with no network. `fw` cannot restart unaided either: the address it would read off the card is the address that just vanished. The cleanup that claimed to handle this was dead code and is gone. The 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. ## 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. - **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. - **The gateway with two real machines**, and a real second NIC carrying real traffic. ## Deliberately not doing **NAT**, rate limiting, fragment reassembly, ICMP type matching, deep IPv6. Scope, not difficulty.