On Tue, May 19, 2015 at 14:07 +0200, Alexandr Nedvedicky wrote: > Hello Mike, > > I've reworked patch from yesterday. I've done some quick testing > to see if it fixes problem. It looks like it works. I have not > tested NAT-64 yet. Also I'd like to come up with test case, which > will show the state check is still able to block invalid ICMP packet > (invalid with respect to state). > > The idea of fix is to keep icmp_dir in state as well. The icmp_dir > indicates whether state got created by ICMP request or response. > This is useful later in pf_icmp_state_lookup() to check whether > ICMP request/response matches state direction. >
This feels slightly convoluted... check my diff out! (: > The attached patch keeps ICMP state match for both cases: > > pass in on dst-nic from any to any > > and > pass out on dst-nic from any to any > > the dst-nic is NIC towards ICMP destination network. > > regards > sasha > > P.S. I took discussion off-line not to create extra noise on [email protected] > feel free go get the alias back to loop. Nah, that's what tech@ is for! diff --git sys/net/pf.c sys/net/pf.c index 39d5cb6..81e23de 100644 --- sys/net/pf.c +++ sys/net/pf.c @@ -177,11 +177,11 @@ int pf_test_rule(struct pf_pdesc *, struct pf_rule **, static __inline int pf_create_state(struct pf_pdesc *, struct pf_rule *, struct pf_rule *, struct pf_rule *, struct pf_state_key **, struct pf_state_key **, int *, struct pf_state **, int, struct pf_rule_slist *, struct pf_rule_actions *, - struct pf_src_node *[]); + struct pf_src_node *[], int); static __inline int pf_state_key_addr_setup(struct pf_pdesc *, void *, int, struct pf_addr *, int, struct pf_addr *, int, int); int pf_state_key_setup(struct pf_pdesc *, struct pf_state_key **, struct pf_state_key **, int); @@ -3365,11 +3365,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm, REASON_SET(&reason, PFRES_MAXSTATES); goto cleanup; } action = pf_create_state(pd, r, a, nr, &skw, &sks, &rewrite, - sm, tag, &rules, &act, sns); + sm, tag, &rules, &act, sns, icmp_dir); if (action != PF_PASS) return (action); if (sks != skw) { struct pf_state_key *sk; @@ -3433,11 +3433,12 @@ cleanup: static __inline int pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a, struct pf_rule *nr, struct pf_state_key **skw, struct pf_state_key **sks, int *rewrite, struct pf_state **sm, int tag, struct pf_rule_slist *rules, - struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX]) + struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX], + int icmp_dir) { struct pf_state *s = NULL; struct tcphdr *th = pd->hdr.tcp; u_int16_t mss = tcp_mssdflt; u_short reason; @@ -3446,10 +3447,11 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a, s = pool_get(&pf_state_pl, PR_NOWAIT | PR_ZERO); if (s == NULL) { REASON_SET(&reason, PFRES_MEMORY); goto csfailed; } + s->direction = pd->dir; s->rule.ptr = r; s->anchor.ptr = a; s->natrule.ptr = nr; memcpy(&s->match_rules, rules, sizeof(s->match_rules)); STATE_INC_COUNTERS(s); @@ -3517,10 +3519,14 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a, break; case IPPROTO_ICMP: #ifdef INET6 case IPPROTO_ICMPV6: #endif + /* XOR Magic! */ + s->direction = s->direction == icmp_dir ? + PF_IN : PF_OUT; + s->timeout = PFTM_ICMP_FIRST_PACKET; break; default: s->src.state = PFOTHERS_SINGLE; s->dst.state = PFOTHERS_NO_TRAFFIC; @@ -3543,11 +3549,10 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a, DPFPRINTF(LOG_ERR, "pf_normalize_tcp_stateful failed on first pkt"); goto csfailed; } } - s->direction = pd->dir; if (pf_state_key_setup(pd, skw, sks, act->rtableid)) { REASON_SET(&reason, PFRES_MEMORY); goto csfailed; }
