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
155
156
157
158
159
160
161
162
163
164
165
166
|
# 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.
`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, 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.** 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
### 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 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.
## 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
**NAT**, rate limiting, fragment reassembly, ICMP type matching, deep
IPv6. Scope, not difficulty.
|
