Page MenuHomeFreeBSD

pf: Fix handling of v6 loopback connections with pf syncookies enabled
ClosedPublic

Authored by markj on Oct 22 2024, 8:30 PM.
Tags
None
Referenced Files
F104732754: D47257.diff
Mon, Dec 9, 5:11 AM
Unknown Object (File)
Wed, Dec 4, 11:58 PM
Unknown Object (File)
Sun, Nov 24, 8:16 AM
Unknown Object (File)
Sun, Nov 17, 12:50 AM
Unknown Object (File)
Sun, Nov 10, 3:02 PM
Unknown Object (File)
Nov 7 2024, 4:41 PM
Unknown Object (File)
Nov 7 2024, 11:28 AM
Unknown Object (File)
Nov 7 2024, 2:25 AM

Details

Summary

The SYN|ACK generated by pf needs to inherit M_LOOP from the original
SYN, otherwise it gets dropped by ip6_input().

Fix this by adding an mbuf_flags argument to pf_build_tcp() that can be
used to set both M_SKIP_FIREWALL and M_LOOP as needed. Set M_LOOP on
the output mbuf if it was generated in response to an mbuf with M_LOOP
set.

Add a regression test case. The v4 case had no problems, but the v6
case fails without this change.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60185
Build 57069: arc lint + arc unit

Event Timeline

markj requested review of this revision.Oct 22 2024, 8:30 PM

Do you have a bit more detail on exactly how this breaks?

Is the problem that we're getting a scope mismatch because pf_build_tcp() does m->m_pkthdr.rcvif = V_loif?
That leaves me thinking we ought to set the correct rcvif (i.e. the one from the ACK that causes us to re-create the original SYN packet) rather than tell the stack to just ignore the scope check.

In D47257#1077521, @kp wrote:

Do you have a bit more detail on exactly how this breaks?

Is the problem that we're getting a scope mismatch because pf_build_tcp() does m->m_pkthdr.rcvif = V_loif?
That leaves me thinking we ought to set the correct rcvif (i.e. the one from the ACK that causes us to re-create the original SYN packet) rather than tell the stack to just ignore the scope check.

For packets with src/dstaddr == ::1, this check in ip6_input() fails:

808                 if (V_ip6_sav && !(m->m_flags & M_LOOP) &&                                                                                                                                                                                                                                                               
809                     __predict_false(in6_localip_fib(&ip6->ip6_src,                                                                                                                                                                                                                                                       
810                             rcvif->if_fib))) {                                                                                                                                                                                                                                                                           
811                         printf("%s:%d\n", __func__, __LINE__);                                                                                                                                                                                                                                                           
812                         IP6STAT_INC(ip6s_badscope); /* XXX */                                                                                                                                                                                                                                                            
813                         goto bad;                                                                                                                                                                                                                                                                                        
814                 }

In particular, I don't think setting the rcvif is sufficient for that case, but I think you're right that we should also do that.

In particular, I don't think setting the rcvif is sufficient for that case, but I think you're right that we should also do that.

It wouldn't be, no, because I was thinking of cases where we received an external packet, and this is about packets on the loopback interface.
Especially because we already set rcvif = V_loif in pf_build_tcp().

sys/netpfil/pf/pf.c
2210

The IPv4 stack doesn't check for it, but we may as well set M_LOOP here too, to be consistent with the IPv6 case.

2231–2235

I wonder if we should MPASS(pfse->pfse_m->m_pkthdr.rcvif == V_loif); here.

This revision is now accepted and ready to land.Oct 23 2024, 3:10 PM
  • Set M_LOOP for v4 packets as well.
  • Add a suggested assertion.
  • Test non-loopback addresses as well.
This revision now requires review to proceed.Oct 24 2024, 8:08 PM
In D47257#1077599, @kp wrote:

In particular, I don't think setting the rcvif is sufficient for that case, but I think you're right that we should also do that.

It wouldn't be, no, because I was thinking of cases where we received an external packet, and this is about packets on the loopback interface.
Especially because we already set rcvif = V_loif in pf_build_tcp().

I think for now I'll leave the rcvif alone, since it's not necessary to change it for loopback traffic. The one edge case is loopback packets between link-local addresses - pf needs some logic to set the embedded scope ID, and I'm not sure it's worth the complexity yet.

I think for now I'll leave the rcvif alone, since it's not necessary to change it for loopback traffic. The one edge case is loopback packets between link-local addresses - pf needs some logic to set the embedded scope ID, and I'm not sure it's worth the complexity yet.

I'm inclined to agree.

I know there are issues with embedded scope in other contexts as wel. I believe rdr to loopback is still broken for IPv6 due to scope issues.
It's also extremely common for users to 'set skip on lo', so this is an edge case that mostly doesn't affect users anyway.

This revision is now accepted and ready to land.Oct 25 2024, 8:31 AM