fw: fix a bypass, a broken round-trip, and eleven others from review

The serious one is that the ctl filter was a blacklist.  checkctl looked
at connect and announce and passed everything else, but udpctl takes
"headers", and udpcreate gives a conversation a live write queue at clone
time:

	c->wq = qbypass(udpkick, c);

So three writes -- clone, "headers", a header-prefixed datagram to data --
sent a packet anywhere, with no connect for a rule to match.  rudp and
icmpv6 have the same verb, gre has raw and forward.  none.ndb did not
mean "no network at all", though the manual said it did.  It is now a
whitelist of control messages that cannot reach the network by
themselves, which is the argument this code already made about ndb
attributes it does not recognise, applied where it was not.

Also blocking: %M was never installed, so fmtrules emitted ipmask=%M% and
every ctl edit on a rule set containing ip= failed, while save wrote a
file reload would reject.  Tests had exercised the ctl path and the ip=
path but never together.

parserules built the new list in the globals with no lock, so for the
length of a reload the relay procs walked a list that was empty and then
half built -- exactly what installrules' comment promised could not
happen.  etherwriteip put a 64KB frame on a 32KB proc stack, the same bug
design.md records learning and fixing in relay().  putback and
notehandler were dead code: atexit matches on the registering pid and
_exits never runs the handlers, which is why the cleanup "did not fire"
rather than being flaky.  Nothing puts the card back, and the docs that
said otherwise are corrected.

The rest: expired flows kept matching and refreshing themselves; the pkt
interface claimed a 4096 MTU from a 1514-byte card; ports were read out
of non-first fragments; /net/ndb and /net/log were writable and
ipifc/*/data readable through the filter; control files were
world-writable, and owning them as a user called "fw" locked out the
administrator instead; delete 0 appended a rule reading <nil>; a dead
relay left one direction unfiltered with nothing to notice; and IPv6
unicast under -e was dropped in silence when it is simply not
implemented.

All three modes regression tested after: a namespace refusing headers and
port 22 while allowing 443, a card passing https and then blocking it
live, and the machine's network restored afterwards.

doc/todo.md says which of these were reproduced and which were read.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

1 files changed, 143 insertions, 98 deletions

diff --git a/fw/doc/todo.md b/fw/doc/todo.md
index 97d1d71..e750674 100644
--- a/fw/doc/todo.md
+++ b/fw/doc/todo.md
@@ -1,103 +1,148 @@
 # fw: open items
 
-Ranked. Item 1 is the only thing between this and running it on a
-machine you care about.
-
-## 1. A dead fw takes the network with it
-
-**Severity: blocks use.**
-
-Taking a card is destructive and is not undone. The `pkt` interface that
-replaces it is `unbindonclose`, so when `fw` stops the interface goes and
-the address goes with it. The machine is left with a card bound to
-nothing and no network.
-
-Worse, `fw` cannot restart unaided: the address it would read off the
-card is the address that just vanished. So it exits, and a supervisor
-with `restart=always` would spin.
-
-`fw` tries to put the card back as it exits (`putback`, via `atexit` and
-`threadnotify`). That covers an orderly stop. **It did not fire on a kill
-in testing** and I did not chase it further — a cleanup that works
-sometimes is worse than none, because you would trust it.
-
-The fix is probably not more note handling. Whatever restarts `fw` has to
-be able to configure the card first, which means the address has to
-survive somewhere `fw` does not own. That is a supervisor's job.
-
-This is also what blocks `svc` supervision (item 3), so one fix, two
-payoffs.
-
-## 2. Broadcast handling is written but unwitnessed
-
-Broadcast and multicast are addressed directly rather than resolved:
-`255.255.255.255` and the subnet broadcast to `ff:ff:ff:ff:ff:ff`,
-`224/4` to `01:00:5e:...`, `ff00::/8` to `33:33:...`. Without this a
-DHCP renewal would be ARPed for the gateway and unicast there, and the
-lease would quietly never renew.
-
-The mapping is the standard one and normal traffic is unaffected, but
-**I never managed to get a broadcast to cross `fw` to confirm it.**
-Reviewed, not observed.
-
-Related and unfixed: `fw` reads the address once at startup, so a lease
-that *changes* the address goes unnoticed until restart.
-
-## 3. fw daemonizes, so svc cannot supervise it
-
-The process you exec returns immediately and leaves the server behind.
-`svc` would see an instant exit and, with `restart=always`, spin.
-
-Needs a foreground mode where the process started is the process that
-stays. `svc`'s `ready=srv:name` fits: `fw -s fw.ether0` already posts to
-`/srv`.
-
-`fwstart` is the wrong shape and should probably go. `svc` already does
-dependency ordering and per-service supervision; `fwstart` re-implements
-the loop in rc and then exits, so `svc` would be supervising a process
-that has already gone. One service per card is the right shape.
-
-## 4. One card per fw, and rules cannot name a card
-
-Two cards means two `fw`s with two rule files. Tested and it works, but
-a rule cannot say `ifc=ether0`, so one file cannot express different
-policy for different cards. Wants repeatable `-e` and an `ifc=`
-attribute, and those go together.
-
-## 5. Tflush is not implemented
-
-A request `fw` is blocked on cannot be abandoned, so killing a program
-that is waiting for an inbound connection does not reach `fw`. Only
-affects the namespace mode; nothing in packet mode blocks indefinitely.
-
-Doable: record the worker's pid against the Req, post an `interrupt`
-note on flush, catch it with `threadnotify` so the syscall returns
-`interrupted` rather than killing the proc. Maybe 80 lines. There is a
-race that cannot be fully closed — between the syscall returning and the
-handler clearing its entry, a note may already be in flight and land on
-a worker that has moved on, failing an unrelated request. Rare,
-unreproducible, and the reason to do it deliberately.
-
-## 6. Positional delete renumbers
-
-`delete n` counts lines of `rules`, so numbers shift after each delete
-and `delete 3` twice removes two different rules. Inherent to positional
-deletion — iptables has it too — but it should be said out loud.
-
-## 7. Untested at the edges
-
-- The gateway has only been tested between two synthetic stacks on one
-  machine. `run.sh` has `-gw`/`-lan` for a two-VM test; the client VM
-  was never built.
-- No IPv6 traffic has been pushed through any mode. The code paths
-  exist and parse v6, but nothing has exercised them.
-- No test with a real second NIC carrying real traffic.
+Most of this came from a code review after the first commit. Where a
+finding has been fixed it says so and how it was checked; where it has
+not, it says what is actually true.
+
+## Fixed since the review
+
+1. **The ctl filter was a blacklist.** `headers` on a udp conversation
+   passed straight through, and the write queue is live from clone, so
+   three writes sent a datagram anywhere with no `connect` for a rule to
+   match. Now a whitelist: `bind`, `ttl`, `tos`, `ignoreadvice`,
+   `close`, `hangup`, `keepalive` pass; `connect` and `announce` are
+   rule-checked; everything else is refused by name. Verified — `headers`
+   and gre `raw` are refused, `ttl` still works, and `none.ndb` now means
+   what it says.
+2. **`%M` was never installed**, so `fmtrules` emitted `ipmask=%M%` and
+   every ctl edit on a rule set containing `ip=` failed. Reproduced, then
+   fixed with `fmtinstall('M', eipfmt)`. Verified round-tripping.
+3. **`parserules` built the new list in the globals** with no lock, so
+   relays walked an empty then half-built list during any reload. Now
+   built in locals. Verified: load, reload, a rejected file leaving the
+   old rules, and edits.
+4. **A 64KB frame on a 32KB proc stack** in `etherwriteip` — the same
+   bug already fixed in `relay()` and left here. Now `Maxframe`.
+5. **`putback`/`notehandler` were dead code** — `atexit` matches on the
+   registering pid and `_exits` never runs the handlers. Removed, and
+   every doc that claimed the card was put back is corrected. Nothing
+   puts it back; see item 1 below.
+6. **Expired flows kept matching.** `flowlook` refreshed `last` without
+   checking the timeout, and reaping only happens on insert, so on an
+   idle firewall a dead flow passed traffic forever.
+7. **The pkt interface claimed a 4096 MTU** from a 1514-byte card. Now
+   `mtu 1500`. Verified: `pkt0 maxtu 1500`.
+8. **Ports were read out of non-first fragments**, so fragmented traffic
+   was denied and a crafted fragment could pass. Later fragments now
+   match on addresses and protocol only.
+9. **Four holes in the filtered `/net`**: `/net/ndb` and `/net/log` were
+   writable (mode 0666 — reconfigure name service, or trace every
+   connection on the machine), and `ipifc/*/data` was readable, which on
+   a machine also running `fw -e` is the packet stream itself. All
+   closed, reads still work. Verified.
+10. **Control files were world-writable.** Now 0600/0440 and owned by
+    the user running `fw` — testing caught that owning them as a
+    nonexistent user "fw" locked out the administrator too.
+11. **`delete 0` and `delete foo`** appended a rule reading `<nil>`. Now
+    refused.
+12. **A dead relay left half a firewall** — one direction unfiltered,
+    nothing to notice. Now `threadexitsall`.
+13. **IPv6 unicast under `-e`** was dropped in silence. It is still not
+    implemented — neighbour discovery is missing — but it now says so
+    once instead of pretending.
+
+## 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 has been
+removed. 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.
+
+### 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.
+
+## 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`.
+- **Control files are world-writable.** `ctl` and `rules` at 0666 means
+  any user can rewrite the firewall. 0660 or 0600.
+- **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.
+- **`delete 0` / `delete foo`** falls into the append branch with a nil
+  argument and appends a rule reading `<nil>`.
+- **A dead relay proc leaves a half-firewall** — the proc returns, the
+  others carry on, one direction is permanently dead with nothing to
+  notice. `threadexitsall` is the fail-closed answer.
+- **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()`.
+- The `reclaim` comment is orphaned above `putback`.
+- `putback`'s "the ctl fd must stay open" is wrong —
+  `ethermedium.unbindonclose` is 0.
+- `fswalk1` does not reject names containing `/`.
+- `fwstart`'s `mntgen` inherits the caller's fds — the trap CLAUDE.md
+  documents.
+
+## Still open from before
+
+- **Broadcast handling is written but never observed.** The mapping is
+  standard and normal traffic is unaffected, but no broadcast has been
+  seen crossing `fw`.
+- **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**: the gateway with two real VMs, any IPv6 traffic, a
+  real second NIC carrying traffic.
+
+## Docs that are now wrong
+
+- `design.md`, `todo.md` and `man/fw` all said `fw` "tries to put the
+  card back as it exits, which covers an orderly stop". It covers
+  nothing — see 5.
+- `man/fw` SYNOPSIS shows `-a` as required with `-e`; it is optional.
+- `man/fw` says an empty rule file means no network at all; see 1.
+- `design.md` presents "cs and dns come along for free" as a benefit. It
+  is also why DNS cannot be blocked in namespace mode, which belongs
+  next to it.
 
 ## Deliberately not doing
 
-**NAT.** See design.md. A 9front client imports `/net`; anything else
-behind an IPv4 gateway with one address is the only case that needs it,
-and that case can wait for someone who actually has it.
-
-**Rate limiting, fragment logic, ICMP type matching, deep IPv6.** Scope,
-not difficulty. The useful firewall is the one that ships.
+**NAT**, rate limiting, fragment reassembly, ICMP type matching, deep
+IPv6. Scope, not difficulty.