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

Reply via email to