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>

4 files changed, 173 insertions, 43 deletions

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