kern/164402: [pf] pf crashes with a particular set of rules when first matching packet arrives

[Ermal Luçi]

2012/4/17 Gleb Smirnoff <glebius at freebsd.org>:
> On Tue, Apr 17, 2012 at 10:38:31AM +0200, Ermal Lu?i wrote:
> E> 2012/4/17 Gleb Smirnoff <glebius at freebsd.org>:

<removed some comments>

> E> >
> E> > In this case crash or freeze is fixed, but still packet is dropped. Example
> E> > of such rule:
> E> >
> E> > pass in on igb0 fastroute proto tcp from any to $localip
> E> >
> E> > Anyway, dropping packets is much better than crashing.
> E> >
> E>
> E> Actually after some coffee :) i think its better marking the packet
> E> with M_SKIP_FIREWALL since
> E> it has already taken its decision on this packet.
> E>
> E> The simloop consumers seem to be just facilities of how things work
> E> from what i can see.
> E>
> E> So just delivering the packet by sending skipping the firewalls seems
> E> more sensibile!
> E>
> E> Though the persistent case for the tags should be revisited since it
> E> may fix some other issues with pf(4) tags, and some others.
>
> We can make the assignment like:
>
> if (ifp->if_flags & IFF_LOOPBACK)
>        m->m_flags |= M_SKIP_FIREWALL;

Yeah the protection seems sensible enough, probably needs some more
testing first.
I will try to do some asap.

The issue is that pf(4) with state keeping is not designed to see a
looping packet.
Also the pf(4) route-to/reply-to logic is not meant to loop packet
back but deliver them to final configured destination.
Hence the skip firewall.

The only problem i might see is when running more than one firewall
together but still there are other issues when you do that at pfil(9)
level.
Also, if_simloop is not meant for packet leaving the host so that
should be safe no?

>
> Because only loopback interface is special: processing its ifp->if_output()
> leads to re-entering ip_input().
>
> I'm afraid that if we mark all pf-routed packets as M_SKIP_FIREWALL, that
> can lead to a case when packet is routed by pf processing on input hook, and
> then it skips processing the output hook, and that can lead to state
> not being updated and session stuck.
>
> Still, I think that pf tag deserves MTAG_PERSISTENT. That would keep us
> in-line with OpenBSD, since they store pf information right in the pkthdr,
> and I don't think that they erase it anywhere until mbuf is freed.

Do agree

>
> --
> Totus tuus, Glebius.



-- 
Ermal