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
|
# 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.
## 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**, rate limiting, fragment reassembly, ICMP type matching, deep
IPv6. Scope, not difficulty.
|
