diff options
Diffstat (limited to 'fw/doc/todo.md')
| -rw-r--r-- | fw/doc/todo.md | 241 |
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. |