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

Authored by markj on Oct 22 2024, 8:30 PM.

Details

Reviewers

Commits

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

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 Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Oct 22 2024, 8:30 PM

Herald added subscribers: vegeta_tuxpowered.net, glebius, melifaro and 3 others. · View Herald TranscriptOct 22 2024, 8:30 PM

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

Harbormaster completed remote builds in B60165: Diff 145371.Oct 22 2024, 8:30 PM

franco_opnsense.org added a subscriber: franco_opnsense.org.Oct 23 2024, 6:15 AM

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 D47257#1077561, @markj 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().

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

Harbormaster completed remote builds in B60185: Diff 145439.Oct 24 2024, 8:08 PM

In D47257#1077599, @kp wrote:

In D47257#1077561, @markj 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.

In D47257#1078043, @markj wrote:

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

Closed by commit rG4a7c6d6206cf: pf: Fix handling of v6 loopback connections with pf syncookies enabled (authored by markj). · Explain WhyOct 29 2024, 3:50 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rG4a7c6d6206cf: pf: Fix handling of v6 loopback connections with pf syncookies enabled.