diff options
| -rw-r--r-- | fw/doc/design.md | 11 | ||||
| -rw-r--r-- | fw/doc/todo.md | 188 | ||||
| -rw-r--r-- | fw/man/fw | 46 | ||||
| -rw-r--r-- | fw/src/fw.c | 10 |
4 files changed, 166 insertions, 89 deletions
diff --git a/fw/doc/design.md b/fw/doc/design.md index 1c78db1..c45c86b 100644 --- a/fw/doc/design.md +++ b/fw/doc/design.md @@ -114,6 +114,17 @@ stays an ordered list and not a lookup. An unrecognised attribute is fatal: silently ignoring `prot=tcp` would leave a rule matching every protocol, and a typo that fails open is not something a firewall may do. +**Named, not filtered.** What the synthetic `/net` contains is a list +of what is served, not a list of what to hide. A list of things to deny +was wrong twice: `trans` installs kernel address translations and devip +gates it with `iseve()`, which through a proxy is *fw's* identity and +not the caller's; and `log`, a trace of every connection on the machine, +had only its write refused when reading it was the leak. Both times the +bug was an omission from a list, which is a kind of bug a whitelist +cannot have. A protocol nobody thought to name is a protocol nobody can +reach — the same direction the rule parser fails in when it meets an +attribute it does not know. + **First match wins, default deny.** Firewall matching is a solved interface. Being different about it would be cost for its own sake. diff --git a/fw/doc/todo.md b/fw/doc/todo.md index e750674..bf4bcda 100644 --- a/fw/doc/todo.md +++ b/fw/doc/todo.md @@ -1,55 +1,74 @@ # fw: open items -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. +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. -## Fixed since the review +`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 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. + 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. Reproduced, then - fixed with `fmtinstall('M', eipfmt)`. Verified round-tripping. + 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. 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. + 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 `<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. + +## 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 @@ -61,12 +80,12 @@ 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. +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. -### deny=in ip=... silently never matches in namespace mode +### 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 @@ -74,6 +93,20 @@ 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 @@ -82,8 +115,6 @@ own standard it should either be enforced at listen time (check 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. @@ -92,11 +123,6 @@ own standard it should either be enforced at listen time (check 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 @@ -108,39 +134,31 @@ own standard it should either be enforced at listen time (check - `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 +- `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, +- 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. +- 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 @@ -126,6 +126,39 @@ A refused write fails, and the diagnostic is what .IR dial (2) reports to the program that tried it. .PP +The +.B /net +it serves is named rather than filtered: protocol directories and their +conversations, plus +.BR cs , +.BR dns , +.BR arp , +.BR bootp , +.BR iproute , +.B ipselftab +and +.BR ndb , +of which only +.B cs +and +.B dns +may be written. +Nothing else is there at all - not a card, not +.BR ipmux , +not +.BR log , +not a protocol's +.B trans +file, and not an interface's +.B data +or +.B snoop +file, each of which is a wire. +A protocol +.I fw +has not been told about is one nobody in the namespace can reach, which +is the safe way for this list to be out of date. +.PP Because the policy lives in a namespace, each program can have its own. Filtering a program's requests is not a boundary by itself - see .B CONTAINMENT @@ -441,7 +474,18 @@ 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 -Taking a card is destructive and is not undone reliably. +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. +.PP +Taking a card is destructive and is not undone. The interface that replaces it is unbound when .I fw stops, and the address goes with it, so a diff --git a/fw/src/fw.c b/fw/src/fw.c index 4dc6d79..70bcc5c 100644 --- a/fw/src/fw.c +++ b/fw/src/fw.c @@ -306,9 +306,13 @@ wireup(Wire *w) if(fprint(w->cfd, "bind pkt") < 0) sysfatal("%s: bind pkt: %r", w->net); /* - * pktmedium claims 4096 with no link header; a card is 1514 with - * 14. Left alone the protected stack emits packets the card - * refuses, and only remote peers capping the MSS hide it. + * pktmedium claims 4096 with no link header, where a card is 1514 + * with 14, so a stack behind one of these thinks it may send 4096 + * bytes. In card mode devether then refuses the frame outright; + * between two stacks the packet only has further to go before an + * ether interface refuses it. Either way 1500 is what the traffic + * will meet in the end, so say so here. Left alone, the only + * thing hiding it is remote peers capping the MSS. */ if(fprint(w->cfd, "mtu 1500") < 0) fprint(2, "fw: %s: cannot set mtu: %r\n", w->net); |