1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
|
# fw: open items
Most of this came from code review. Where a finding has been fixed it
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 — 77 of
them now. Run it twice in a row after touching anything.
## Fixed, first round
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.
2. **`%M` was never installed**, so `fmtrules` emitted `ipmask=%M%` and
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.
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.**
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.
## Fixed, second round
1. **The path filter was still a blacklist, and still had a hole.**
`/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.
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.
11. **"fw daemonizes, so svc cannot supervise it" was wrong.** `svc`
has had the detaching shape from the start: `ready=srv:` watches
the `/srv` name, and for those services the process exiting "is
normal and never causes a restart". Nothing needed a foreground
mode. What was true underneath it: the control filesystem going
away ended the server proc and left the relays filtering, so the
`/srv` name could be gone while the firewall was still running.
Now any part stopping stops all of them, which is what makes the
name worth watching. `fwstart` is still the wrong shape; a service
file per card is in `lib/svc`.
## 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 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 is what makes `restart=always` a half-measure rather than an
answer: `svc` will start `fw` again, and the new one has no address to
read off a card that no longer has one. It needs `-a`, or the card
configured again first. Supervision works; recovery does not.
### 2. One card per fw, and rules cannot name a card
Wants repeatable `-e` and an `ifc=` attribute, together.
### 3. 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.
### 4. Positional `delete n` renumbers
Inherent to positional deletion; iptables has it too. Said out loud
here rather than fixed.
## Never tested
- **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. 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.
## Deliberately not doing
**NAT**, rate limiting, fragment reassembly, ICMP type matching, deep
IPv6. Scope, not difficulty.
|
