This also really needs a test case.

What real-life situation is this fix for? If this is for connections coming from behind a 3rd party SNAT, where SNAT reuses source ports faster than pf expires states, then maybe tuning pf timeouts would be enough. Or we could allow pf states to transition from TCPS_FIN_WAIT_2 back to TCPS_SYN_SENT, basically implementing SO_REUSEPORT for pf.

In D44488#1014674, @vegeta_tuxpowered.net wrote:

What real-life situation is this fix for? If this is for connections coming from behind a 3rd party SNAT, where SNAT reuses source ports faster than pf expires states, then maybe tuning pf timeouts would be enough. Or we could allow pf states to transition from TCPS_FIN_WAIT_2 back to TCPS_SYN_SENT, basically implementing SO_REUSEPORT for pf.

It is for behind a load balancer, where the initial connection comes in one public IP #1, but then all future connections come in on public IP #2, but the external source port from the incoming IP stays the same.
So when PF NAT's it to the web server behind the PF RDR rules, the incoming source ports conflict.

I've spent some time looking into the underlying problem, and I'm not sure that this approach (quietly rewriting the src port) is correct. The pf.conf manual page says:

In addition to modifying the address, some translation rules may modify
source or destination ports for tcp(4) or udp(4) connections; implicitly
in the case of nat rules and explicitly in the case of rdr rules.  Port
numbers are never translated with a binat rule.

but with this change we are implicitly rewriting the source port. What explicit mechanism is the man page alluding to here?

As an aside, I also found that if a nat rule matches an outbound packet, but we fail to allocate a src port for it (e.g., because static-port is configured and a collision occurs, or pf_get_sport() fails to find a free port), then pf will pass the packet out without performing any translation. That is, we end up passing out unmodified packets from the internal network. This happens in the "NAT proxy port allocation failed" path in pf_get_translation(); I'd really expect pf to drop the packet instead. Is this a bug?

In D44488#1031012, @markj wrote:

but with this change we are implicitly rewriting the source port. What explicit mechanism is the man page alluding to here?

That'd be things like rdr on $ext_if proto tcp from any to any port 80 -> 127.0.0.1 port 8080, where we redirect incoming connections on port 80 to localhost port 8080.
When we're doing explicit rewrites it's always going to be the destination port, of course.

As an aside, I also found that if a nat rule matches an outbound packet, but we fail to allocate a src port for it (e.g., because static-port is configured and a collision occurs, or pf_get_sport() fails to find a free port), then pf will pass the packet out without performing any translation. That is, we end up passing out unmodified packets from the internal network. This happens in the "NAT proxy port allocation failed" path in pf_get_translation(); I'd really expect pf to drop the packet instead. Is this a bug?

Yeah, I'd say that's a bug. I suspect it mostly stems from confusion between 'there is no translation required' and 'something went wrong', because both return NULL.

In D44488#1031012, @markj wrote:

I've spent some time looking into the underlying problem, and I'm not sure that this approach (quietly rewriting the src port) is correct. The pf.conf manual page says:

In addition to modifying the address, some translation rules may modify
source or destination ports for tcp(4) or udp(4) connections; implicitly
in the case of nat rules and explicitly in the case of rdr rules.  Port
numbers are never translated with a binat rule.

but with this change we are implicitly rewriting the source port. What explicit mechanism is the man page alluding to here?

There may be a subtlety here, the static-port flag is mostly talking about 'NAT' rules, and the purpose of the static-port flag is to keep the source-port used by internal machines (on the inside) when translated to the public IP (outside).
In the case this patch is addressing, we're modulating the source-port of an inbound connection, and we are only changing it 'inside' our network, because the only other choice is to drop the packet.

In D44488#1031217, @kp wrote:

In D44488#1031012, @markj wrote:

but with this change we are implicitly rewriting the source port. What explicit mechanism is the man page alluding to here?

That'd be things like rdr on $ext_if proto tcp from any to any port 80 -> 127.0.0.1 port 8080, where we redirect incoming connections on port 80 to localhost port 8080.
When we're doing explicit rewrites it's always going to be the destination port, of course.

Ah, I had read that as meaning that it was possible for rdr rules to rewrite source ports, but upon a second look, it seems not.

There may be a subtlety here, the static-port flag is mostly talking about 'NAT' rules, and the purpose of the static-port flag is to keep the source-port used by internal machines (on the inside) when translated to the public IP (outside).
In the case this patch is addressing, we're modulating the source-port of an inbound connection, and we are only changing it 'inside' our network, because the only other choice is to drop the packet.

Right, I guess what I'm getting at is that the rdr rule syntax probably ought to provide some control over how source port rewriting is done. For instance, one might want to provide a range of source ports to use, or to disable rewriting entirely (ala static-port for nat rules).

In D44488#1031217, @kp wrote:

Yeah, I'd say that's a bug. I suspect it mostly stems from confusion between 'there is no translation required' and 'something went wrong', because both return NULL.

I posted https://reviews.freebsd.org/D45672 to fix this, though I still need to write a test case.

Beating a closed ticket here, but in case anyone is reading this - wouldn't there be a way to DoS this?
If you know anything at all about the target network, it would seem that you could hammer the load balanced address 15k times to create states for all the source ports in the range, and then hit one of the other addresses that have rdr rules that translate to the same address. Each such hit would cause the loop to run 15k times, for as long as the initial states remain valid. Or am I completely misreading this?

Being the one who raised this issue in the first place I should probably have chimed in earlier. Sorry 'bout that.

In D44488#1056431, @ltning-freebsd_anduin.net wrote:

Beating a closed ticket here, but in case anyone is reading this - wouldn't there be a way to DoS this?
If you know anything at all about the target network, it would seem that you could hammer the load balanced address 15k times to create states for all the source ports in the range, and then hit one of the other addresses that have rdr rules that translate to the same address. Each such hit would cause the loop to run 15k times, for as long as the initial states remain valid. Or am I completely misreading this?

Yes, that's true. The loop is effectively the same as the one used by NAT rules to assign a source port, but that would typically apply to outbound connections and so is less concerning. I'm not sure if 15k iterations is expensive enough for this to be a fatal problem, but it could well be.

Probably the easiest mitigation is to give up after a short number of attempts, on the basis that these kinds of state conflicts are rare in practice. I had been working on an extension to this patch to ignore conflicts if the conflicting state is in a FIN_WAIT_2 or TIME_WAIT state, as pf_state_key_attach() will simply reuse the state in that case, but wasn't sure if that was really necessary. If that seems desirable, I'll finish it.

Yes, that's true. The loop is effectively the same as the one used by NAT rules to assign a source port, but that would typically apply to outbound connections and so is less concerning. I'm not sure if 15k iterations is expensive enough for this to be a fatal problem, but it could well be.

Probably not for random drive-by, but we've seen (D)DoS attack vectors requiring a high degree of knowledge about our particular network (not to mention motivation well beyond trophy-hunting). If *I* could think of it...

Probably the easiest mitigation is to give up after a short number of attempts, on the basis that these kinds of state conflicts are rare in practice. I had been working on an extension to this patch to ignore conflicts if the conflicting state is in a FIN_WAIT_2 or TIME_WAIT state, as pf_state_key_attach() will simply reuse the state in that case, but wasn't sure if that was really necessary. If that seems desirable, I'll finish it.

Sounds like a good idea. And perhaps a configurable loop count limiter? We were seeing this on a pretty large percentage of connections from a specific (and deeply troubled..) source due to the way the client worked: Connections to the LB and to the RDR target handled by the same process, using the same starting port number, increasing more or less in sync.