Proposed fix for NPF bug bin/54124

[Tom Ivar Helbekkmo <tih%hamartun.priv.no@localhost>]

Sevan and I were playing with NPF yesterday, mapping out a problem where
the documented default "flags S/SAFR" for stateful rules that affected
TCP packets but didn't specify any flags, didn't actually get applied
to a rule like "pass stateful out all".  The big problem with this is
that when you then do a "block return-rst", that RST packet will create
state for the connection it's blocking, so that a second attempt from
the same source will pass through.

The result of our testing is PR bin/54124.

I've figured out that the problem is in npfctl_build_code(), in
usr.sbin/npf/npfctl/npf_build.c, where we find this code to implement
the default:

	/*
	 * If this is a stateful rule and TCP flags are not specified,
	 * then add "flags S/SAFR" filter for TCP protocol case.
	 */
	if ((npf_rule_getattr(rl) & NPF_RULE_STATEFUL) != 0 &&
	    (proto == -1 || (proto == IPPROTO_TCP && !op->op_opts))) {
		npfctl_bpf_tcpfl(bc, TH_SYN,
		    TH_SYN | TH_ACK | TH_FIN | TH_RST, proto == -1);
	}

That's all good - but a few lines above this, there's a check to see if
there's even anything to generate byte code for:

	/* If none specified, then no byte-code. */
	noproto = family == AF_UNSPEC && proto == -1 && !op->op_opts;
	noaddrs = !apfrom->ap_netaddr && !apto->ap_netaddr;
	noports = !apfrom->ap_portrange && !apto->ap_portrange;
	if (noproto && noaddrs && noports) {
		return false;
	}

The rule "pass stateful out all" gets trapped there, even though it is,
in fact, necessary to generate byte code for it: we have to check if the
packet is TCP, and, if so, apply the default flags test and store state.

I wrote a simple fix for this, which I've verified against my own
various configurations, which all work just right with the change, and
for which I can see that the generated BPF byte code only differs in the
specific ways I expect it to.  npftest(8) is also happy with the change.

While testing this, I found that the function npfctl_bpf_tcpfl(), in
usr.sbin/npf/npfctl/npf_bpf_comp.c, had a related bug that hasn't been
observed, because it didn't get invoked in the relevant case.  This
caused 'npfctl debug' to crash.

Here's my proposed fix:

Index: usr.sbin/npf/npfctl/npf_bpf_comp.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/npf/npfctl/npf_bpf_comp.c,v
retrieving revision 1.11
diff -u -p -r1.11 npf_bpf_comp.c
--- usr.sbin/npf/npfctl/npf_bpf_comp.c	29 Sep 2018 14:41:36 -0000	1.11
+++ usr.sbin/npf/npfctl/npf_bpf_comp.c	17 Apr 2019 10:14:05 -0000
@@ -565,10 +565,8 @@ npfctl_bpf_tcpfl(npf_bpf_t *ctx, uint8_t
 	};
 	add_insns(ctx, insns_cmp, __arraycount(insns_cmp));
 
-	if (!checktcp) {
-		uint32_t mwords[] = { BM_TCPFL, 2, tf, tf_mask};
-		done_block(ctx, mwords, sizeof(mwords));
-	}
+	uint32_t mwords[] = { BM_TCPFL, 2, tf, tf_mask};
+	done_block(ctx, mwords, sizeof(mwords));
 }
 
 /*
Index: usr.sbin/npf/npfctl/npf_build.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/npf/npfctl/npf_build.c,v
retrieving revision 1.47
diff -u -p -r1.47 npf_build.c
--- usr.sbin/npf/npfctl/npf_build.c	19 Jan 2019 21:19:32 -0000	1.47
+++ usr.sbin/npf/npfctl/npf_build.c	17 Apr 2019 10:14:05 -0000
@@ -363,7 +363,7 @@ static bool
 npfctl_build_code(nl_rule_t *rl, sa_family_t family, const opt_proto_t *op,
     const filt_opts_t *fopts)
 {
-	bool noproto, noaddrs, noports, need_tcpudp = false;
+	bool noproto, noaddrs, noports, nostate, need_tcpudp = false;
 	const addr_port_t *apfrom = &fopts->fo_from;
 	const addr_port_t *apto = &fopts->fo_to;
 	const int proto = op->op_proto;
@@ -375,7 +375,8 @@ npfctl_build_code(nl_rule_t *rl, sa_fami
 	noproto = family == AF_UNSPEC && proto == -1 && !op->op_opts;
 	noaddrs = !apfrom->ap_netaddr && !apto->ap_netaddr;
 	noports = !apfrom->ap_portrange && !apto->ap_portrange;
-	if (noproto && noaddrs && noports) {
+	nostate = !(npf_rule_getattr(rl) & NPF_RULE_STATEFUL);
+	if (noproto && noaddrs && noports && nostate) {
 		return false;
 	}
 

-tih
-- 
Most people who graduate with CS degrees don't understand the significance
of Lisp.  Lisp is the most important idea in computer science.  --Alan Kay