Page MenuHomeFreeBSD

netinet*: Fix redirects for connections from localhost
ClosedPublic

Authored by dfr on May 24 2023, 1:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 9:15 PM
Unknown Object (File)
Sat, Mar 16, 9:53 PM
Unknown Object (File)
Sat, Mar 16, 9:50 PM
Unknown Object (File)
Sat, Mar 16, 9:50 PM
Unknown Object (File)
Thu, Mar 14, 6:21 AM
Unknown Object (File)
Thu, Mar 14, 6:17 AM
Unknown Object (File)
Thu, Mar 14, 6:17 AM
Unknown Object (File)
Sun, Mar 10, 10:11 PM

Details

Summary

Redirect rules use PFIL_IN and PFIL_OUT events to allow packet filter
rules to change the destination address and port for a connection.
Typically, the rule triggers on an input event when a packet is received
by a router and the destination address and/or port is changed to
implement the redirect. When a reply packet on this connection is output
to the network, the rule triggers again, reversing the modification.

When the connection is initiated on the same host as the packet filter,
it is initially output via lo0 which queues it for input processing.
This causes an input event on the lo0 interface, allowing redirect
processing to rewrite the destination and create state for the
connection. However, when the reply is received, no corresponding output
event is generated; instead, the packet is delivered to the higher level
protocol (e.g. tcp or udp) without reversing the redirect, the reply is
not matched to the connection and the packet is dropped (for tcp, a
connection reset is also sent).

This commit fixes the problem by adding a second packet filter call in
the input path. The second call happens right before the handoff to
higher level processing and provides the missing output event to allow
the redirect's reply processing to perform its rewrite. This extra
processing is disabled by default and can be enabled using sysctl:

sysctl net.inet.ip.filter_local_output=1
sysctl net.inet6.ip6.filter_local_output=1

PR: 268717

Test Plan

kyua -k /usr/tests/Kyuafile sys/netpfil/pf/rdr

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 51677
Build 48568: arc lint + arc unit

Event Timeline

dfr requested review of this revision.May 24 2023, 1:13 PM
kp added a subscriber: kp.

I still don't like this, but I appreciate that it fixes a real problem (and that I've not come up with a better version either).

It's also very hard to be upset at people who include test cases with their patches ;)

tests/sys/netpfil/pf/rdr.sh
78

Not strictly required, but this might be more appropriate in netpfil/common, because then a single test case could cover pf, ipf and ipfw.

This revision is now accepted and ready to land.May 24 2023, 2:21 PM
In D40256#916511, @kp wrote:

I still don't like this, but I appreciate that it fixes a real problem (and that I've not come up with a better version either).

It's also very hard to be upset at people who include test cases with their patches ;)

Thanks for the review. I'll take the time to investigate how the tests in netpfil/common work and improve coverage for the other firewalls.

melifaro added inline comments.
sys/netinet/ip_input.c
828

Does it have to be then standard _outbound_ hook? I'd rather consider using a new hook and avoiding adding new sysctl variables at all.

sys/netinet/ip_input.c
828

In this scenario, the packet is notionally leaving L3, in this case for local L4 processing rather than being forwarded to some other host via L2. Looking at linux, this can be hooked separately using NF_*_LOCAL_OUT.

I chose to re-use the existing OUT hook since it matched the expectations of both PF and IPF's redirect processing and fixes the issue for both filters. I could add a new hook but for both firewalls it would end up going through the same output processing as PFIL_HOOKED_OUT. It should still be opt-in to avoid the risk of breaking existing setups with the extra event so we'll still need a sysctl somewhere.

If people feel strongly that we should have a new hook, I'll change it but right now, I'm not quite seeing the benefit of adding this (slight) extra complexity.

sys/netinet/ip_input.c
828

Thank you for the reply!
Let me expand a bit on my original comment - I wrote it in a hurry.
First of all, I agree that there is a problem with redirects & currently there is no hook for the local outbound connections. I support the idea of adding such hook, my concern is about the current implementation.

I also agree that opt-in is a great idea. For example, I'm not sure what effects it can have for ipfw(8) (which I believe is notably more popular than ipfilter). Phil(9) allows perfect opt-in semantics - you subscribe to the hooks you need and that's it. It also simplifies data path (a single check instead of checking the head & sysctl) and allows to tune behaviour per-firewall, instead of per-system. For example, link-level pfil hooks were added not so long ago, using exactly this semantics.

sys/netinet/ip_input.c
828

Thanks for the clarification. I'll take a look at adding a new hook tomorrow and updating the diff along those lines.

sys/netinet/ip_input.c
828

Thanks for the clarification. I'll take a look at adding a new hook tomorrow and updating the diff along those lines.

I will add two new pfil heads for ipv4 and ipv6 local traffic. The existing pf hooks can be linked to these, either by default or opt-in via pfilctl. This will supply the necessary event to fix redirects

I'm not sure what to do for packets output to local addresses. Currently, ip_output routes these to ip_input where they causes input events on V_inet_pfil_head - for symmetry, perhaps these events should be sent to the new head but that means the events will only be seen if at least the input hooks are linked to the new heads by default.

Add new pfil heads for the 'output to local' events.

This revision now requires review to proceed.May 25 2023, 12:22 PM
dfr marked 2 inline comments as done.May 25 2023, 12:34 PM

LGTM!
I'd be nice to add the new hook to pfil(9) for consistency.

sys/netinet/ip_input.c
331

I'd explicitly set pa_flags = PFIL_OUT here to reflect the reality.

828

Thank you for addressing the comments!
Re output: I'd simply mark the new heads as PFIL_OUT only.

This revision is now accepted and ready to land.May 25 2023, 12:59 PM

Also: I guess, there should be another change that enables use of this hook in pf?

Change the new hook heads to PFIL_OUT, move rdr tests to tests/sys/netpfil/common and add ipfnat versions.

This revision now requires review to proceed.May 26 2023, 11:53 AM
dfr marked 3 inline comments as done.May 26 2023, 12:00 PM

Also: I guess, there should be another change that enables use of this hook in pf?

This doesn't need any changes to pf. To opt-in the user can enable by linking the existing output hooks to the new heads using pfilctl.

I plan to merge this change to 13-stable, keeping it as opt-in but I'd like to enable it by default for 14.0 unless there are objections. That can be in a separate commit.

melifaro added inline comments.
sys/netinet/ip_input.c
331

Nit: while line would probably improve readability here a bit.

This revision is now accepted and ready to land.May 26 2023, 1:59 PM
sys/netinet/ip_input.c
331

Not sure what you mean here, possibly "blank line"? If so, I agree and will add.

dfr marked an inline comment as done.May 30 2023, 12:17 PM

Are there any other concerns or comments for this one? If not, I'll land it and probably add a followup diff to enable by default on 14.0.

No concerns from my side.

sys/netinet/ip_input.c
331

Yep, sorry, blank line indeed :-)