diff options
Diffstat (limited to 'fw/doc/todo.md')
| -rw-r--r-- | fw/doc/todo.md | 191 |
1 files changed, 85 insertions, 106 deletions
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. |