Proposed fix for NPF bug bin/54124

[Tom Ivar Helbekkmo]

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

[Tom Ivar Helbekkmo]

[...]

rmind has verified this to be correct, and merged my equivalent pull
request into the github.com NPF repository. As this is a security
issue, and a local modification in NetBSD won't be in conflict with a
future import of a newer NPF, I'll commit my solution, and request a
pullup to NetBSD 8.

-tih