Hello,
On Mon, Jan 04, 2021 at 06:37:54PM +0100, Alexander Bluhm wrote: > On Mon, Jan 04, 2021 at 11:21:50PM +1000, David Gwynne wrote: > > this chunk pops out as a standalone change. > > > > having pf_find_state() return PF_PASS here means the callers short > > circuit and let the packet go through without running it through the > > a lot of the state handling, which includes things like protocol state > > updates, nat, scrubbing, some pflog handling, and most importantly, > > later calls to pf_route(). > > pf_route() calls pf_test() again with a different interface. yes, that's correct. In ideal world one of those should happen: a) no state is found, try to find matching rule b) matching rule found, create a new state if the rule orders to do so c) no rule found either, just accept packet > > The idea of this code is, that the interface which is passed to > pf_test() from ip_output() is wrong. The call to pf_set_rt_ifp() > changes it in the state. can you clarify what 'wrong' means here? > > Index: pf.c > > =================================================================== > > RCS file: /cvs/src/sys/net/pf.c,v > > retrieving revision 1.1097 > > diff -u -p -r1.1097 pf.c > > --- pf.c 4 Jan 2021 12:48:27 -0000 1.1097 > > +++ pf.c 4 Jan 2021 13:08:26 -0000 > > @@ -1122,12 +1122,6 @@ pf_find_state(struct pf_pdesc *pd, struc > > } > > > > *state = s; > > - if (pd->dir == PF_OUT && s->rt_kif != NULL && s->rt_kif != pd->kif && verify interface matches the one specified in state for outbound packet. if the interfaces don't match, then.... > > - ((s->rule.ptr->rt == PF_ROUTETO && > > - s->rule.ptr->direction == PF_OUT) || we must be either dealing with state created by route-to action bound to outbound rule. > > - (s->rule.ptr->rt == PF_REPLYTO && > > - s->rule.ptr->direction == PF_IN))) > > - return (PF_PASS); or reply-to action bound to inbound rule... ...if it is the case then we short circuit the state check and assume the state matches... let's consider the rules as follows: pass out from 1.2.3.4 to any route-to 10.20.30.40@em1 pass out from any to 1.2.3.4 route-to 192.168.1.10@em0 pass out on em1 from 1.2.3.4 to any nat-to (em1) let's further assume there is outbound packet on em0 interface. the packet matches the rule with 'route-to' action. state gets created: out@em0 1.2.3.4 -> a.b.c.d route-to 10.20.30.40@em1 pf_route() gets called, and packet arrives to pf_test() again as outbound on em1 interface. No state should be found, because em1 != em0. so that complex if does not kick in. Now let there be a response packet at em1 interface. It gets translated to a.b.c.d -> 1.2.3.4 and forwarded. let's further assume the packet hits (default) route, which goes via em3. There is outbound packet at em3 inspected by pf_test(): out@em3 a.b.c.d -> 1.2.3.4 no matching state is found because of em3 != em0, so packet hits the 'pass out from any to 1.2.3.4' rule. The rule creates state: out@em3 a.b.c.d -> 1.2.3.4 route-to 192.168.1.10@em0 and packet is sent out via pf_route('em0'). The packet enters pf_test() again. We find matching state this time: in@em0 1.2.3.4 -> a.b.c.d route-to@10.20.30.40@em1 let's see if the complex condition is met this time: pd->dir == PF_OUT (it is outbound packet) pd->kif == em0 (bound to em0 interface) s->rt_kif == em1 (route* action is bound to em1) s->rule.ptr->direction == PF_OUT (state got created by outbound rule) s->rule.ptr == PF_ROUTETO (there is a route-to action at rule) It looks like the complex if() test is satisfied this time, so we bail out with PF_PASS. So what are consequences of skipping proper state check? first of all the state does not get updated. If the packet, which bypasses state check is SYN-ACK, the state is left in SYN-SENT state, although the connection initiator is in connected state already (assuming it receives SYN-ACK). I hope I have not missed anything in story above. it seems to me the code in question can indeed do some harm (sometimes...) thanks and regards sashan