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 [email protected]@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
