Hello, during our testing we've discovered small glitch in ICMP state handling. we use simple rule as follows:
# pfctl -sr pass in on vnet2 all flags S/SA next we create a local outbound traffic using ping to arbitrary destination over vnet2 interface. This is what we get: # ping 172.16.1.2 PING 172.16.1.2 (172.16.1.2): 56 data bytes 64 bytes from 172.16.1.2: icmp_seq=0 ttl=255 time=0.718 ms ping: sendto: No route to host ping: wrote 172.16.1.2 64 chars, ret=-1 ping: sendto: No route to host ping: wrote 172.16.1.2 64 chars, ret=-1 </snip> 64 bytes from 172.16.1.2: icmp_seq=20 ttl=255 time=0.587 ms ping: sendto: No route to host ping: wrote 172.16.1.2 64 chars, ret=-1 it looks like state created by icmp_seq=0 response must expire first before firewall is able to put next packet to wire. It took me a while to figure out what's going on here. I think the problem is PF keeps packet direction in pf_state::direction, when state gets created, while the pf_icmp_state_lookup() uses icmp_dir to verify whether packet is valid or invalid for given state. The idea of the fix is straightforward: remember ICMP direction in pf_pdesc, so it can be passed to newly created state for ICMP packet. the straightforward fix is bit cluttered by change, which switches icmp_dir to u_int8_t. as soon as fix get applied the ping command works, all ICMP probes leave firewall host. patch is attached. regards sasha
? icmp-state.patch ? pf_table.c.diff Index: pf.c =================================================================== RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.913 diff -u -r1.913 pf.c --- pf.c 11 May 2015 12:22:14 -0000 1.913 +++ pf.c 18 May 2015 17:07:14 -0000 @@ -148,8 +148,8 @@ struct pf_state_peer *); void pf_change_a6(struct pf_pdesc *, struct pf_addr *a, struct pf_addr *an); -int pf_icmp_mapping(struct pf_pdesc *, u_int8_t, int *, - int *, u_int16_t *, u_int16_t *); +int pf_icmp_mapping(struct pf_pdesc *, u_int8_t, + u_int8_t *, int *, u_int16_t *, u_int16_t *); void pf_change_icmp(struct pf_pdesc *, struct pf_addr *, u_int16_t *, struct pf_addr *, struct pf_addr *, u_int16_t, sa_family_t); @@ -197,7 +197,7 @@ u_short *); int pf_icmp_state_lookup(struct pf_pdesc *, struct pf_state_key_cmp *, struct pf_state **, - u_int16_t, u_int16_t, int, int *, int, int); + u_int16_t, u_int16_t, u_int8_t, int *, int, int); int pf_test_state_icmp(struct pf_pdesc *, struct pf_state **, u_short *); u_int8_t pf_get_wscale(struct pf_pdesc *); @@ -1689,8 +1689,8 @@ #endif /* INET6 */ int -pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, int *icmp_dir, int *multi, - u_int16_t *virtual_id, u_int16_t *virtual_type) +pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, u_int8_t *icmp_dir, + int *multi, u_int16_t *virtual_id, u_int16_t *virtual_type) { /* * ICMP types marked with PF_OUT are typically responses to @@ -3081,9 +3081,10 @@ int tag = -1; int asd = 0; int match = 0; - int state_icmp = 0, icmp_dir, multi; + int state_icmp = 0, multi; u_int16_t virtual_type, virtual_id; u_int8_t icmptype = 0, icmpcode = 0; + u_int8_t icmp_dir = (u_int8_t)-1; bzero(&act, sizeof(act)); bzero(sns, sizeof(sns)); @@ -3124,7 +3125,10 @@ } break; #endif /* INET6 */ + default: + icmp_dir = (u_int8_t)-1; } + pd->icmp_dir = icmp_dir; ruleset = &pf_main_ruleset; r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr); @@ -3549,7 +3553,7 @@ goto csfailed; } } - s->direction = pd->dir; + s->direction = (pd->icmp_dir == (u_int8_t)-1) ? pd->dir : pd->icmp_dir; if (pf_state_key_setup(pd, skw, sks, act->rtableid)) { REASON_SET(&reason, PFRES_MEMORY); @@ -3627,7 +3631,7 @@ int pf_translate(struct pf_pdesc *pd, struct pf_addr *saddr, u_int16_t sport, struct pf_addr *daddr, u_int16_t dport, u_int16_t virtual_type, - int icmp_dir) + u_int8_t icmp_dir) { /* * when called from bpf_mtap_pflog, there are extra constraints: @@ -4416,7 +4420,7 @@ int pf_icmp_state_lookup(struct pf_pdesc *pd, struct pf_state_key_cmp *key, struct pf_state **state, u_int16_t icmpid, u_int16_t type, - int icmp_dir, int *iidx, int multi, int inner) + u_int8_t icmp_dir, int *iidx, int multi, int inner) { int direction; @@ -4469,8 +4473,8 @@ { struct pf_addr *saddr = pd->src, *daddr = pd->dst; u_int16_t virtual_id, virtual_type; - u_int8_t icmptype; - int icmp_dir, iidx, ret, multi, copyback = 0; + u_int8_t icmptype, icmp_dir; + int iidx, ret, multi, copyback = 0; struct pf_state_key_cmp key; @@ -6041,6 +6045,7 @@ pd->didx = (dir == PF_IN) ? 1 : 0; pd->af = pd->naf = af; pd->rdomain = rtable_l2(pd->m->m_pkthdr.ph_rtableid); + pd->icmp_dir = -1; switch (pd->af) { case AF_INET: { Index: pfvar.h =================================================================== RCS file: /cvs/src/sys/net/pfvar.h,v retrieving revision 1.414 diff -u -r1.414 pfvar.h --- pfvar.h 11 Apr 2015 13:00:12 -0000 1.414 +++ pfvar.h 18 May 2015 17:07:14 -0000 @@ -1284,6 +1284,7 @@ u_int8_t tos; u_int8_t ttl; u_int8_t dir; /* direction */ + u_int8_t icmp_dir; u_int8_t sidx; /* key index for source */ u_int8_t didx; /* key index for destination */ u_int8_t destchg; /* flag set when destination changed */ @@ -1850,7 +1851,7 @@ void pf_pkt_addr_changed(struct mbuf *); int pf_state_key_attach(struct pf_state_key *, struct pf_state *, int); int pf_translate(struct pf_pdesc *, struct pf_addr *, u_int16_t, - struct pf_addr *, u_int16_t, u_int16_t, int); + struct pf_addr *, u_int16_t, u_int16_t, u_int8_t); int pf_translate_af(struct pf_pdesc *); void pf_route(struct mbuf **, struct pf_rule *, int, struct ifnet *, struct pf_state *);