fw: fix a bypass, a broken round-trip, and eleven others from review

The serious one is that the ctl filter was a blacklist.  checkctl looked
at connect and announce and passed everything else, but udpctl takes
"headers", and udpcreate gives a conversation a live write queue at clone
time:

	c->wq = qbypass(udpkick, c);

So three writes -- clone, "headers", a header-prefixed datagram to data --
sent a packet anywhere, with no connect for a rule to match.  rudp and
icmpv6 have the same verb, gre has raw and forward.  none.ndb did not
mean "no network at all", though the manual said it did.  It is now a
whitelist of control messages that cannot reach the network by
themselves, which is the argument this code already made about ndb
attributes it does not recognise, applied where it was not.

Also blocking: %M was never installed, so fmtrules emitted ipmask=%M% and
every ctl edit on a rule set containing ip= failed, while save wrote a
file reload would reject.  Tests had exercised the ctl path and the ip=
path but never together.

parserules built the new list in the globals with no lock, so for the
length of a reload the relay procs walked a list that was empty and then
half built -- exactly what installrules' comment promised could not
happen.  etherwriteip put a 64KB frame on a 32KB proc stack, the same bug
design.md records learning and fixing in relay().  putback and
notehandler were dead code: atexit matches on the registering pid and
_exits never runs the handlers, which is why the cleanup "did not fire"
rather than being flaky.  Nothing puts the card back, and the docs that
said otherwise are corrected.

The rest: expired flows kept matching and refreshing themselves; the pkt
interface claimed a 4096 MTU from a 1514-byte card; ports were read out
of non-first fragments; /net/ndb and /net/log were writable and
ipifc/*/data readable through the filter; control files were
world-writable, and owning them as a user called "fw" locked out the
administrator instead; delete 0 appended a rule reading <nil>; a dead
relay left one direction unfiltered with nothing to notice; and IPv6
unicast under -e was dropped in silence when it is simply not
implemented.

All three modes regression tested after: a namespace refusing headers and
port 22 while allowing 443, a card passing https and then blocking it
live, and the machine's network restored afterwards.

doc/todo.md says which of these were reproduced and which were read.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

7 files changed, 327 insertions, 149 deletions

diff --git a/fw/doc/design.md b/fw/doc/design.md
index d6f3e55..1c78db1 100644
--- a/fw/doc/design.md
+++ b/fw/doc/design.md
@@ -4,7 +4,7 @@
 two networks, or in front of one namespace.**
 
 Status: working and tested on the init-test VM, in all three modes. Not
-yet fit to run on a machine you care about — see `todo.md`, item 1.
+yet fit to run on a machine you care about — see `todo.md`.
 
 ## Why
 
@@ -72,7 +72,10 @@ write, and the text comes back out of `dial(2)` — which is a far better
 diagnostic than a dropped packet.
 
 Because it proxies the *assembled* `/net` rather than synthesising a
-tree, `cs` and `dns` come along for free.
+tree, `cs` and `dns` come along for free. That is a convenience and a
+hole in the same sentence: it is also why name resolution cannot be
+refused in this mode, so a program with an empty rule set can still get
+names looked up, which is exfiltration if you care about that.
 
 The real `/net` needs no second name and must not have one: any surviving
 path to it is a way around the filter. lib9p forks the server with
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.
diff --git a/fw/man/fw b/fw/man/fw
index 34a9d19..5a7f592 100644
--- a/fw/man/fw
+++ b/fw/man/fw
@@ -13,8 +13,8 @@ fw \- firewall
 .br
 .RB [ -e
 .I ether
-.B -a
-.IR addr / mask
+.RB [ -a
+.IR addr / mask ]
 .RB [ -g
 .IR gateway ]]
 .br
@@ -455,12 +455,12 @@ one with
 or the card configured again with
 .IR ipconfig (8)
 first.
-.I Fw
-tries to put the card back as it exits, which covers an orderly stop but
-not a kill.
+Nothing puts the card back.
 Whatever restarts
 .I fw
-should be prepared to configure the card first.
+must configure the card first, with the address it had, which
+.I fw
+no longer knows.
 .PP
 The first packet to an unresolved next hop is dropped while
 .I fw
diff --git a/fw/src/ether.c b/fw/src/ether.c
index 54f5068..dd20971 100644
--- a/fw/src/ether.c
+++ b/fw/src/ether.c
@@ -38,6 +38,7 @@ enum
 
 	Arplen		= 28,
 	Eminlen		= 60,		/* ethernet minimum frame, devether enforces it */
+	Maxframe	= Ehdrlen + 16*1024,
 	Arpreq		= 1,
 	Arpreply	= 2,
 
@@ -323,11 +324,16 @@ etherisip(uchar *f, int n)
 int
 etherwriteip(uchar *p, int n, uchar *mask)
 {
-	uchar f[Ehdrlen + 64*1024], dst[IPaddrlen], hop[IPaddrlen];
+	/*
+	 * On a proc stack, which libthread keeps small: 64K here
+	 * overran it and corrupted the data segment.  pktmedium's maxtu
+	 * is 4096, so this is already generous.
+	 */
+	uchar f[Maxframe], dst[IPaddrlen], hop[IPaddrlen];
 	uchar net[IPaddrlen], ournet[IPaddrlen], mac[Eaddrlen];
 	int type, len;
 
-	if(n < 20 || n > 64*1024 - Ehdrlen)
+	if(n < 20 || n > Maxframe - Ehdrlen)
 		return -1;
 	switch(p[0] >> 4){
 	case 4:
@@ -355,6 +361,20 @@ etherwriteip(uchar *p, int n, uchar *mask)
 		ipmove(hop, dst);
 
 	if(!arpget(hop, mac)){
+		/*
+		 * v6 has no ARP; resolving a neighbour needs ICMPv6
+		 * solicitation, which is not implemented.  Say so once,
+		 * rather than dropping every v6 unicast in silence.
+		 */
+		if(!isv4(hop)){
+			static int said;
+
+			if(!said++)
+				fprint(2, "fw: cannot resolve %I: "
+					"IPv6 neighbour discovery is not implemented, "
+					"so v6 unicast is dropped\n", hop);
+			return -1;
+		}
 		if(etherdebug)
 			fprint(2, "arp: no entry for %I, dropping and asking\n", hop);
 		arpask(hop);
diff --git a/fw/src/fw.c b/fw/src/fw.c
index 4f94090..a136194 100644
--- a/fw/src/fw.c
+++ b/fw/src/fw.c
@@ -48,6 +48,7 @@ struct Pkt
 	uchar	dst[IPaddrlen];
 	int	sport;
 	int	dport;
+	int	frag;			/* a later fragment: no ports in it */
 	int	verb;
 };
 
@@ -130,6 +131,15 @@ flowlook(int proto, uchar *src, uchar *dst, int sport, int dport, long now)
 	h = flowhash(proto, src, dst, sport, dport);
 	for(f = flowtab[h]; f != nil; f = f->next)
 		if(flowis(f, proto, src, dst, sport, dport)){
+			/*
+			 * An expired flow must not match.  Reaping only
+			 * happens when a flow is added, so on a quiet
+			 * firewall nothing is ever reaped and a flow that
+			 * timed out long ago would keep passing traffic,
+			 * refreshing itself on every packet.
+			 */
+			if(now - f->last > flowtimeout(f->proto))
+				return 0;
 			f->last = now;
 			return 1;
 		}
@@ -295,6 +305,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.
+	 */
+	if(fprint(w->cfd, "mtu 1500") < 0)
+		fprint(2, "fw: %s: cannot set mtu: %r\n", w->net);
 	if(fprint(w->cfd, "add %s %s", w->addr, w->mask) < 0)
 		sysfatal("%s: add %s %s: %r", w->net, w->addr, w->mask);
 
@@ -333,6 +350,15 @@ parsepkt(uchar *b, int n, Pkt *p)
 		p->proto = b[9];
 		v4tov6(p->src, b + 12);
 		v4tov6(p->dst, b + 16);
+		/*
+		 * Only the first fragment carries the transport header.
+		 * Reading one out of a later fragment gives payload bytes
+		 * as ports, which both loses the traffic - it matches no
+		 * flow - and lets a crafted fragment whose bytes happen to
+		 * match an open flow through.
+		 */
+		if((nhgets(b + 6) & 0x1FFF) != 0)
+			p->frag = 1;
 		t = b + hl;
 		n -= hl;
 		break;
@@ -349,7 +375,7 @@ parsepkt(uchar *b, int n, Pkt *p)
 		return;
 	}
 
-	if((p->proto == 6 || p->proto == 17) && n >= 4){
+	if(!p->frag && (p->proto == 6 || p->proto == 17) && n >= 4){
 		p->sport = nhgets(t);
 		p->dport = nhgets(t + 2);
 	}
@@ -382,7 +408,7 @@ relay(Wire *from, Wire *to, int verb)
 		if((n = read(from->dfd, buf, Maxpkt)) <= 0){
 			fprint(2, "fw: %s: read failed: %r\n", from->side);
 			syslog(0, "fw", "%s: read failed: %r", from->side);
-			return;
+			threadexitsall("wire");
 		}
 
 		parsepkt(buf, n, &p);
@@ -818,8 +844,15 @@ fswrite(Req *r)
 		err = editrules(arg, 1, 0);
 	else if(strcmp(buf, "append") == 0)
 		err = editrules(arg, 0, 0);
-	else if(strcmp(buf, "delete") == 0)
-		err = editrules(nil, 0, atoi(arg));
+	else if(strcmp(buf, "delete") == 0){
+		int nr;
+
+		nr = atoi(arg);
+		if(nr < 1)
+			err = "delete wants a rule number, from 1";
+		else
+			err = editrules(nil, 0, nr);
+	}
 	else
 		err = "unknown command; read ctl for the list";
 
@@ -854,14 +887,21 @@ rulesdidchange(void)
 static void
 servectl(char *mtpt, char *srvname)
 {
+	char *user;
 	File *root;
 
-	fs.tree = alloctree("fw", "fw", DMDIR|0555, nil);
+	/*
+	 * Owned by whoever is running fw, not by a user called "fw" that
+	 * does not exist: with 0600 that locked out the administrator as
+	 * effectively as everyone else.
+	 */
+	user = getuser();
+	fs.tree = alloctree(user, user, DMDIR|0555, nil);
 	root = fs.tree->root;
-	closefile(createfile(root, "ctl", "fw", 0666, (void*)Qctl));
-	closefile(createfile(root, "rules", "fw", 0666, (void*)Qrules));
-	closefile(createfile(root, "flows", "fw", 0444, (void*)Qflows));
-	closefile(createfile(root, "stats", "fw", 0444, (void*)Qstats));
+	closefile(createfile(root, "ctl", user, 0600, (void*)Qctl));
+	closefile(createfile(root, "rules", user, 0600, (void*)Qrules));
+	closefile(createfile(root, "flows", user, 0440, (void*)Qflows));
+	closefile(createfile(root, "stats", user, 0440, (void*)Qstats));
 	threadpostmountsrv(&fs, srvname, mtpt, MREPL);
 }
 
@@ -947,9 +987,14 @@ etherin(void *a)
 	buf = emalloc(Maxpkt);
 	for(;;){
 		if((n = read(efd, buf, Maxpkt)) <= 0){
+			/*
+			 * One relay stopping would leave the other running
+			 * and that direction unfiltered, with nothing to
+			 * notice.  Take the whole firewall down instead.
+			 */
 			fprint(2, "fw: %s: read the card: %r\n", w->side);
 			syslog(0, "fw", "stopped reading the card: %r");
-			return;
+			threadexitsall("card");
 		}
 		if(debug > 1)
 			fprint(2, "wire: %d bytes type %.4ux\n", n, (buf[12]<<8)|buf[13]);
@@ -977,7 +1022,7 @@ etherout(void *a)
 		if((n = read(w->dfd, buf, Maxpkt)) <= 0){
 			fprint(2, "fw: %s: read the stack: %r\n", w->side);
 			syslog(0, "fw", "stopped reading the stack: %r");
-			return;
+			threadexitsall("stack");
 		}
 		if(permitted(buf, n, Vout, &p))
 			etherwriteip(buf, n, ethermask);
@@ -1057,6 +1102,19 @@ learnaddr(char *net, char *dev, char *addr, int naddr, char *gw, int ngw)
  * it: a live fw would still be holding the card we are about to take.
  */
 /*
+ * Undo a previous fw that died.
+ *
+ * An IP stack outlives the program that configured it, so a fw that is
+ * killed leaves its pkt interface behind, holding the address, with
+ * nothing on the other end of it.  The machine has no network until
+ * someone unpicks that by hand, and the next fw to start makes a second
+ * interface with the same address and routes that could go to either.
+ *
+ * So before taking anything, throw away any pkt interface already
+ * carrying the address we are about to use.  Nothing else can have made
+ * it: a live fw would still be holding the card we are about to take.
+ */
+/*
  * Put the card back.
  *
  * Taking a card is destructive: the stack loses it, and the pkt
@@ -1248,6 +1306,7 @@ threadmain(int argc, char **argv)
 	fmtinstall('I', eipfmt);
 	fmtinstall('V', eipfmt);
 	fmtinstall('E', eipfmt);
+	fmtinstall('M', eipfmt);	/* fmtrules prints masks with it */
 
 	rulepath = argv[0];
 	readrules(rulepath);
@@ -1317,14 +1376,6 @@ threadmain(int argc, char **argv)
 		ethersetaddr(ip, gw, haveg);
 		reclaim(netmtpt, abuf);
 
-		backdev = etherdev;
-		backaddr = abuf;
-		backmask = m;
-		backnet = netmtpt;
-		backgw = ethergw;
-		atexit(putback);
-		threadnotify(notehandler, 1);
-
 		takecard(netmtpt, etherdev);
 
 		prot = emalloc(sizeof *prot);
diff --git a/fw/src/netfs.c b/fw/src/netfs.c
index c87412d..f28485f 100644
--- a/fw/src/netfs.c
+++ b/fw/src/netfs.c
@@ -121,11 +121,31 @@ hidden(char *path)
 static char*
 protect(char *path, int mode)
 {
+	char *p;
+
 	if(hidden(path))
 		return "fw: does not exist";
 	if(strcmp(path, "ipifc/clone") == 0)
 		return "fw: interface creation denied";
-	if(under(path, "ipifc") || under(path, "iproute") || under(path, "arp"))
+
+	/*
+	 * An interface's data file is a wire.  On a machine that is also
+	 * running fw -e it is *the* wire, so reading it hands a filtered
+	 * program the packet stream the filter exists to control.  Not
+	 * read-only: not at all.
+	 */
+	if(under(path, "ipifc"))
+		if((p = strrchr(path, '/')) != nil && strcmp(p+1, "data") == 0)
+			return "fw: denied";
+
+	/*
+	 * ndb is the machine's configuration and log is a trace of every
+	 * connection on it, both mode 0666.  Writing the first
+	 * reconfigures name service for everyone; reading the second
+	 * after enabling it watches the whole machine.
+	 */
+	if(under(path, "ipifc") || under(path, "iproute") || under(path, "arp")
+	|| under(path, "ndb") || under(path, "log"))
 		if((mode & 3) != OREAD)
 			return "fw: read-only under fw";
 	return nil;
@@ -181,6 +201,22 @@ ctlproto(char *path)
  * connect takes addr!port with optional trailing fields; announce takes
  * a bare port, or addr!port with addr often "*".
  */
+/*
+ * Control messages that only change how this one conversation behaves,
+ * and so cannot reach the network by themselves.  Everything outside
+ * this list is refused: see checkctl.
+ */
+static char *okverbs[] = {
+	"bind",			/* the local address; announce is what opens */
+	"ttl",
+	"tos",
+	"ignoreadvice",
+	"close",
+	"hangup",
+	"keepalive",
+	nil,
+};
+
 static char*
 checkctl(char *proto, char *msg, long n)
 {
@@ -188,7 +224,7 @@ checkctl(char *proto, char *msg, long n)
 	static char err[128];
 	uchar ip[IPaddrlen], mask[IPaddrlen];
 	Rule *rule;
-	int nf, na, verb, anyip, port, lport;
+	int nf, na, verb, anyip, port, lport, i;
 
 	if(n <= 0)
 		return nil;
@@ -203,8 +239,31 @@ checkctl(char *proto, char *msg, long n)
 		verb = Vout;
 	else if(strcmp(f[0], "announce") == 0)
 		verb = Vin;
-	else
-		return nil;			/* hangup, ttl, keepalive: not policy */
+	else{
+		/*
+		 * Anything else is refused unless it is known to be
+		 * harmless.  Letting unknown control messages through was
+		 * a hole, not a convenience: "headers" on a udp
+		 * conversation turns it into one that carries its own
+		 * destination, and the write queue is live from the
+		 * moment it is cloned, so three writes send a datagram
+		 * anywhere with no connect for a rule to match.  gre has
+		 * "raw" and "forward"; rudp and icmpv6 have "headers"
+		 * too.
+		 *
+		 * This is the same argument the rule parser already makes
+		 * about attributes it does not recognise, applied to the
+		 * place it was not.
+		 */
+		for(i = 0; okverbs[i] != nil; i++)
+			if(strcmp(f[0], okverbs[i]) == 0)
+				return nil;
+		syslog(0, "fw", "deny %s %s: control message not permitted",
+			proto, f[0]);
+		snprint(err, sizeof err,
+			"fw: %s: not a permitted control message", f[0]);
+		return err;
+	}
 	if(nf < 2)
 		return nil;			/* malformed; let the kernel say so */
 
diff --git a/fw/src/rules.c b/fw/src/rules.c
index c0df82d..55de9cb 100644
--- a/fw/src/rules.c
+++ b/fw/src/rules.c
@@ -30,7 +30,6 @@ Rule	*rules;
 void	(*rulechanged)(void);
 
 static Lock	rulelock;
-static Rule	*lastrule;
 static char	*rulefile;
 static char	*parseerr;
 static jmp_buf	parsejmp;
@@ -135,7 +134,7 @@ verbof(char *s)
  * a firewall gets to do.
  */
 static void
-addrule(Ndbtuple *t, int nr)
+addrule(Ndbtuple *t, int nr, Rule **head, Rule **tail)
 {
 	char *ip, *mask, *p, abuf[64];
 	Rule *r;
@@ -206,28 +205,34 @@ addrule(Ndbtuple *t, int nr)
 		r->anyip = 0;
 	}
 
-	if(lastrule == nil)
-		rules = r;
+	if(*head == nil)
+		*head = r;
 	else
-		lastrule->next = r;
-	lastrule = r;
+		(*tail)->next = r;
+	*tail = r;
 }
 
 /*
  * Parse without installing.  Returns the new list, or nil with *err set.
  * An empty file is a valid rule set: it denies everything.
  */
+/*
+ * Build the new list in locals.  Earlier this borrowed the globals and
+ * put them back afterwards, which meant that for the length of a reload
+ * the relay procs - which take rulelock, a lock this never held - were
+ * walking a list that was first empty and then half built.  Every packet
+ * in that window was judged against a partial rule set, and a parse
+ * failure freed nodes a relay might still have been holding.
+ */
 Rule*
 parserules(char *file, char **err)
 {
-	Rule *new, *save, *savelast;
+	Rule *head, *tail;
 	Ndbtuple *t;
 	Ndb *db;
 	int nr;
 
-	save = rules;
-	savelast = lastrule;
-	rules = lastrule = nil;
+	head = tail = nil;
 	rulefile = file;
 	parseerr = nil;
 
@@ -236,30 +241,25 @@ parserules(char *file, char **err)
 
 		snprint(eb, sizeof eb, "%s: %r", file);
 		*err = eb;
-		rules = save;
-		lastrule = savelast;
 		return nil;
 	}
 	parsing = 1;
 	if(setjmp(parsejmp) == 0){
 		for(nr = 1; (t = ndbparse(db)) != nil; nr++){
-			addrule(t, nr);
+			addrule(t, nr, &head, &tail);
 			ndbfree(t);
 		}
 	}
 	parsing = 0;
 	ndbclose(db);
 
-	new = rules;
-	rules = save;
-	lastrule = savelast;
 	if(parseerr != nil){
-		freerules(new);
+		freerules(head);
 		*err = parseerr;
 		return nil;
 	}
 	*err = nil;
-	return new;
+	return head;
 }
 
 void