On Mon, May 18, 2015 at 19:24 +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> during our testing we've discovered small glitch in ICMP state handling.
> we use simple rule as follows:
>
Hi,
> # pfctl -sr
> pass in on vnet2 all flags S/SA
>
If that is the only rule there is, then you need to consider
the default stateless pass. So effectively you've got:
pass all no state
pass in on vnet2 all flags S/SA keep state
> next we create a local outbound traffic using ping to arbitrary destination
> over vnet2 interface. This is what we get:
>
Which is a bad combo really. You're supposed to use a
"pass out keep state" for that, however the behaviour you're
describing is rather peculiar (and reproducible on OpenBSD).
> # ping 172.16.1.2
> PING 172.16.1.2 (172.16.1.2): 56 data bytes
> 64 bytes from 172.16.1.2: icmp_seq=0 ttl=255 time=0.718 ms
> ping: sendto: No route to host
> ping: wrote 172.16.1.2 64 chars, ret=-1
> ping: sendto: No route to host
> ping: wrote 172.16.1.2 64 chars, ret=-1
> </snip>
> 64 bytes from 172.16.1.2: icmp_seq=20 ttl=255 time=0.587 ms
> ping: sendto: No route to host
> ping: wrote 172.16.1.2 64 chars, ret=-1
>
> it looks like state created by icmp_seq=0 response must expire first before
> firewall is able to put next packet to wire.
>
> It took me a while to figure out what's going on here. I think the problem is
> PF keeps packet direction in pf_state::direction, when state gets created,
> while
> the pf_icmp_state_lookup() uses icmp_dir to verify whether packet is valid or
> invalid for given state.
>
> The idea of the fix is straightforward:
>
> remember ICMP direction in pf_pdesc, so it can be passed to newly created
> state for ICMP packet.
>
> the straightforward fix is bit cluttered by change, which switches icmp_dir to
> u_int8_t.
>
This needs a much closer look, but it might be a result of a bad
interaction between stateless and stateful rules. This code is
rather fragile and needs special care.
If you enable debugging (pfctl -x debug) you'll see that the state
is being created for the reply packet (10.10.1.2 is pinging 10.10.1.1):
May 18 20:26:54 pf: key search, if=vmx1:
ICMP wire: (0) 10.10.1.1:8 10.10.1.2:15988
May 18 20:26:54 pf: key search, if=vmx1:
ICMP wire: (0) 10.10.1.1:8 10.10.1.2:15988
May 18 20:26:54 pf: key setup:
ICMP wire: (0) 10.10.1.1:8 10.10.1.2:15988 stack: (0) -
And then the next packet I send matches the state but fails
the direction check that is there to prevent reply spoofing/
replaying:
May 18 20:26:55 pf: key search, if=vmx1:
ICMP wire: (0) 10.10.1.1:8 10.10.1.2:15988
May 18 20:26:55 pf: icmp type 8 in wrong direction (1):
ICMP in wire: (0) 10.10.1.1:8 10.10.1.2:15988 stack: (0) - 0:0 @0
Perhaps we should deny state creation on reply in the first
place, but this might have repercussions of its own. Generally
speaking the advice is to have a proper outgoing state and not
relying on the default "pass all no state" rule. One recommended
way of achieving it is to have a "block all" as your first rule.
> as soon as fix get applied the ping command works, all ICMP probes leave
> firewall host.
>
Thanks for the patch, we'll be investigating this further.
> patch is attached.
>
> regards
> sasha
