diff options
Diffstat (limited to 'fw')
| -rw-r--r-- | fw/doc/design.md | 5 | ||||
| -rw-r--r-- | fw/doc/todo.md | 191 | ||||
| -rw-r--r-- | fw/man/fw | 22 | ||||
| -rw-r--r-- | fw/man/fwrules | 12 |
4 files changed, 110 insertions, 120 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. @@ -173,8 +173,12 @@ The difference is one of altitude. Packet filtering sees everything, including traffic no local program asked for, but cannot tell one program from another. Request filtering knows exactly who asked, but only ever sees intentions, -so it cannot stop an inbound connection before the handshake and cannot -see traffic that is merely passing through. +and cannot see traffic that is merely passing through. +It cannot stop an inbound connection before the handshake either - the +kernel has answered by the time +.I fw +is asked - but it does refuse the connection to the program and hang it +up, so a rule naming a caller is late rather than decorative. .PP The options are: .TP @@ -474,16 +478,10 @@ IPv6 extension headers are not walked; such packets are matched on their addresses and next-header protocol alone. ICMP has no type or code matching. .PP -A fragmented datagram does not cross. -Only the first fragment carries the transport header, so the rest match -no port, and a rule set written in ports - which is every rule set worth -writing - denies them. -The first fragment arrives and the receiver waits for the others until -it gives up. -Reading ports out of a later fragment, which is what -.I fw -did before, is worse: a fragment whose payload bytes happen to match an -open connection is then let through. +IPv6 fragments are carried in an extension header, which is not walked, +so a fragmented IPv6 datagram does not cross. +IPv4 fragments do: the first one carries the transport header and +decides, and the rest of the datagram inherits what it decided. .PP Taking a card is destructive and is not undone. The interface that replaces it is unbound when diff --git a/fw/man/fwrules b/fw/man/fwrules index 143ac08..2a42fe1 100644 --- a/fw/man/fwrules +++ b/fw/man/fwrules @@ -93,6 +93,12 @@ is matched by and by .B port= never - at that moment nobody has called, so there is no far end. +An +.B in +rule naming an +.B ip +therefore cannot decide an announce; it decides the connection that +arrives, which is asked about separately when there is a caller to name. .TP .BI ip= address the address at the far end, optionally carrying a @@ -158,6 +164,12 @@ ICMP has no ports, so its flows are the two addresses alone, which is enough for a reply to an echo to be recognised, but does not tie an ICMP error to the connection it is about. .PP +A fragmented datagram is a train: only its first piece carries ports, so +that piece is matched against the rules and the rest of the train +inherits the answer. +A train whose first piece was never seen - or was refused - is judged on +its addresses alone. +.PP When the rules change, connections the new rules forbid are dropped rather than left to finish: a block blocks. .SH EXAMPLES |