[RFC PATCH 0/1] netfilter: nat: restore default DNAT behavior

[RFC PATCH 0/1] netfilter: nat: restore default DNAT behavior

[Kyle Swenson]

Hello,

We have noticed what appears to be a regression from v4.4 in the DNAT
port selection logic in nf_nat_core.c.  Specifically, we're expecting an
iptables command like

    iptables -t nat -A PREROUTING -p tcp -d 10.0.0.2 -m tcp --dport 32000:32010
    -j DNAT --to-destination 192.168.0.10:21000-21010

to DNAT traffic sent to 10.0.0.2:32000-32010 to
192.168.0.10:21000-21010.  We use the range on the LAN side to handle
tuple collisions that might occur with a single port specified via
--to-destination.

The behavior we're seeing currently, however, is the behavior we'd
expect if we were passing --random to iptables (but we are not passing
--random): the DNAT'd port is random within the range.

Through my naive debugging, I've arrived at this RFC patch and have
included it mostly to illustrate the behavior our userspace is expecting
with the above iptables command.  The hope is that I can be educated
with what other folks expect the behavior to be, or what I can change in
our iptables command to get the behavior we're expecting.

Thanks so much for your time,
Kyle

-- 
2.43.0

[RFC PATCH 1/1] netfilter: nat: restore default DNAT behavior

[Kyle Swenson]

When a DNAT rule is configured via iptables with different port ranges,

iptables -t nat -A PREROUTING -p tcp -d 10.0.0.2 -m tcp --dport 32000:32010
-j DNAT --to-destination 192.168.0.10:21000-21010

we seem to be DNATing to some random port on the LAN side. While this is
expected if --random is passed to the iptables command, it is not
expected without passing --random.  The expected behavior (and the
observed behavior in v4.4) is the traffic will be DNAT'd to
192.168.0.10:21000 unless there is a tuple collision with that
destination.  In that case, we expect the traffic to be instead DNAT'd
to 192.168.0.10:21001, so on so forth until the end of the range.

This patch is a naive attempt to restore the behavior seen in v4.4.  I'm
hopeful folks will point out problems and regressions this could cause
elsewhere, since I've little experience in the net tree.

Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
---
 net/netfilter/nf_nat_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index c3d7ecbc777c..bd275c3906f7 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -549,12 +549,14 @@ static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
 	}
 
 find_free_id:
 	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
 		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
-	else
+	else if (range->flags & NF_NAT_RANGE_PROTO_RANDOM)
 		off = get_random_u16();
+	else
+		off = 0;
 
 	attempts = range_size;
 	if (attempts > NF_NAT_MAX_ATTEMPTS)
 		attempts = NF_NAT_MAX_ATTEMPTS;
 
-- 
2.43.0

Re: [RFC PATCH 1/1] netfilter: nat: restore default DNAT behavior

[Florian Westphal]

Kyle Swenson <kyle.swenson@est.tech> wrote:
> When a DNAT rule is configured via iptables with different port ranges,
> 
> iptables -t nat -A PREROUTING -p tcp -d 10.0.0.2 -m tcp --dport 32000:32010
> -j DNAT --to-destination 192.168.0.10:21000-21010
> 
> we seem to be DNATing to some random port on the LAN side. While this is
> expected if --random is passed to the iptables command, it is not
> expected without passing --random.  The expected behavior (and the
> observed behavior in v4.4) is the traffic will be DNAT'd to
> 192.168.0.10:21000 unless there is a tuple collision with that
> destination.  In that case, we expect the traffic to be instead DNAT'd
> to 192.168.0.10:21001, so on so forth until the end of the range.
> 
> This patch is a naive attempt to restore the behavior seen in v4.4.  I'm
> hopeful folks will point out problems and regressions this could cause
> elsewhere, since I've little experience in the net tree.
> 
> Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> ---
>  net/netfilter/nf_nat_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index c3d7ecbc777c..bd275c3906f7 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -549,12 +549,14 @@ static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
>  	}
>  
>  find_free_id:
>  	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
>  		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
> -	else
> +	else if (range->flags & NF_NAT_RANGE_PROTO_RANDOM)
>  		off = get_random_u16();
> +	else
> +		off = 0;

Can you restrict this to NF_NAT_MANIP_DST?
I don't want predictable src port conflict resolution.

Probably something like (untested):

find_free_id:
 	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
 		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
+	else if ((range->flags & NF_NAT_RANGE_PROTO_SPECIFIED) &&
+	  	  maniptype == NF_NAT_MANIP_DST))
+ 		off = 1;
	else
  		off = get_random_u16();

Re: [RFC PATCH 1/1] netfilter: nat: restore default DNAT behavior

[Kyle Swenson]

On Fri, Jan 26, 2024 at 04:57:20PM +0100, Florian Westphal wrote:
> Kyle Swenson <kyle.swenson@est.tech> wrote:
> > When a DNAT rule is configured via iptables with different port ranges,
> > 
> > iptables -t nat -A PREROUTING -p tcp -d 10.0.0.2 -m tcp --dport 32000:32010
> > -j DNAT --to-destination 192.168.0.10:21000-21010
> > 
> > we seem to be DNATing to some random port on the LAN side. While this is
> > expected if --random is passed to the iptables command, it is not
> > expected without passing --random.  The expected behavior (and the
> > observed behavior in v4.4) is the traffic will be DNAT'd to
> > 192.168.0.10:21000 unless there is a tuple collision with that
> > destination.  In that case, we expect the traffic to be instead DNAT'd
> > to 192.168.0.10:21001, so on so forth until the end of the range.
> > 
> > This patch is a naive attempt to restore the behavior seen in v4.4.  I'm
> > hopeful folks will point out problems and regressions this could cause
> > elsewhere, since I've little experience in the net tree.
> > 
> > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> > ---
> >  net/netfilter/nf_nat_core.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index c3d7ecbc777c..bd275c3906f7 100644
> > --- a/net/netfilter/nf_nat_core.c
> > +++ b/net/netfilter/nf_nat_core.c
> > @@ -549,12 +549,14 @@ static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
> >  	}
> >  
> >  find_free_id:
> >  	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
> >  		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
> > -	else
> > +	else if (range->flags & NF_NAT_RANGE_PROTO_RANDOM)
> >  		off = get_random_u16();
> > +	else
> > +		off = 0;
> 
> Can you restrict this to NF_NAT_MANIP_DST?
> I don't want predictable src port conflict resolution.
> 
> Probably something like (untested):
> 
> find_free_id:
>  	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
>  		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
> +	else if ((range->flags & NF_NAT_RANGE_PROTO_SPECIFIED) &&
> +	  	  maniptype == NF_NAT_MANIP_DST))
> + 		off = 1;
> 	else
>   		off = get_random_u16();

Yes, absolutely.  I'll test out the change and send a v2 next week.

Thanks,
Kyle

Re: [RFC PATCH 1/1] netfilter: nat: restore default DNAT behavior

[Florian Westphal]

Kyle Swenson <kyle.swenson@est.tech> wrote:
> > Can you restrict this to NF_NAT_MANIP_DST?
> > I don't want predictable src port conflict resolution.
> > 
> > Probably something like (untested):
> > 
> > find_free_id:
> >  	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
> >  		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
> > +	else if ((range->flags & NF_NAT_RANGE_PROTO_SPECIFIED) &&
> > +	  	  maniptype == NF_NAT_MANIP_DST))
> > + 		off = 1;
> > 	else
> >   		off = get_random_u16();
> 
> Yes, absolutely.  I'll test out the change and send a v2 next week.

Thanks! Please tweak the suggestion so that --random still overrides
--range behavior.

Re: [RFC PATCH 1/1] netfilter: nat: restore default DNAT behavior

[Kyle Swenson]

On Mon, Jan 29, 2024 at 08:16:56AM +0100, Florian Westphal wrote:
> Kyle Swenson <kyle.swenson@est.tech> wrote:
> > > Can you restrict this to NF_NAT_MANIP_DST?
> > > I don't want predictable src port conflict resolution.
> > > 
> > > Probably something like (untested):
> > > 
> > > find_free_id:
> > >  	if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
> > >  		off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
> > > +	else if ((range->flags & NF_NAT_RANGE_PROTO_SPECIFIED) &&
> > > +	  	  maniptype == NF_NAT_MANIP_DST))
> > > + 		off = 1;
> > > 	else
> > >   		off = get_random_u16();
> > 
> > Yes, absolutely.  I'll test out the change and send a v2 next week.
> 
> Thanks! Please tweak the suggestion so that --random still overrides
> --range behavior.

Sure, no problem.