[PATCH net v4] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[PATCH net v4] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Cole Dishington]

FTP port selection ignores specified port ranges (with iptables
masquerade --to-ports) when creating an expectation, based on
FTP commands PORT or PASV, for the data connection.

For masquerading, this issue allows an FTP client to use unassigned source ports
for their data connection (in both the PORT and PASV cases). This can
cause problems in setups that allocate different masquerade port ranges for each
client.

The proposed fix involves storing a port range (on nf_conn_nat) to:
- Fix FTP PORT data connections using the stored port range to select a
  port number in nf_conntrack_ftp.
- Fix FTP PASV data connections using the stored port range to specify a
  port range on source port in nf_nat_helper if the FTP PORT/PASV packet
  comes from the client.

Co-developed-by: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>
Signed-off-by: Anthony Lineham <anthony.lineham@alliedtelesis.co.nz>
Co-developed-by: Scott Parlane <scott.parlane@alliedtelesis.co.nz>
Signed-off-by: Scott Parlane <scott.parlane@alliedtelesis.co.nz>
Co-developed-by: Blair Steven <blair.steven@alliedtelesis.co.nz>
Signed-off-by: Blair Steven <blair.steven@alliedtelesis.co.nz>
Signed-off-by: Cole Dishington <Cole.Dishington@alliedtelesis.co.nz>
---

Notes:
    Thanks for your time reviewing!
    
    Changes:
    - Avoid allocating same port to ftp data connection expectation as used for ftp control.
      - Changed ftp port selection back to a for loop with expectation checking.
      - Iterate over a range of ports rather than exp dst port to max port.
    - Added further description to commit message to detail the problem this patch is fixing.

 include/net/netfilter/nf_nat.h |  6 ++++++
 net/netfilter/nf_nat_core.c    |  9 +++++++++
 net/netfilter/nf_nat_ftp.c     | 29 +++++++++++++++++++++--------
 net/netfilter/nf_nat_helper.c  | 10 ++++++++++
 4 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
index 0d412dd63707..231cffc16722 100644
--- a/include/net/netfilter/nf_nat.h
+++ b/include/net/netfilter/nf_nat.h
@@ -27,12 +27,18 @@ union nf_conntrack_nat_help {
 #endif
 };
 
+struct nf_conn_nat_range_info {
+	union nf_conntrack_man_proto    min_proto;
+	union nf_conntrack_man_proto    max_proto;
+};
+
 /* The structure embedded in the conntrack structure. */
 struct nf_conn_nat {
 	union nf_conntrack_nat_help help;
 #if IS_ENABLED(CONFIG_NF_NAT_MASQUERADE)
 	int masq_index;
 #endif
+	struct nf_conn_nat_range_info range_info;
 };
 
 /* Set up the info structure to map into this range. */
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index ea923f8cf9c4..5ae27cf7e808 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -623,6 +623,15 @@ nf_nat_setup_info(struct nf_conn *ct,
 			   &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
 
 	get_unique_tuple(&new_tuple, &curr_tuple, range, ct, maniptype);
+	if (range && (range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
+		struct nf_conn_nat *nat = nf_ct_nat_ext_add(ct);
+
+		if (!nat)
+			return NF_DROP;
+
+		nat->range_info.min_proto = range->min_proto;
+		nat->range_info.max_proto = range->max_proto;
+	}
 
 	if (!nf_ct_tuple_equal(&new_tuple, &curr_tuple)) {
 		struct nf_conntrack_tuple reply;
diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index aace6768a64e..499798ade988 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -72,8 +72,14 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	u_int16_t port;
 	int dir = CTINFO2DIR(ctinfo);
 	struct nf_conn *ct = exp->master;
+	struct nf_conn_nat *nat = nfct_nat(ct);
+	unsigned int i, first_port, min, range_size;
 	char buffer[sizeof("|1||65535|") + INET6_ADDRSTRLEN];
 	unsigned int buflen;
+	int ret;
+
+	if (WARN_ON_ONCE(!nat))
+		return NF_DROP;
 
 	pr_debug("type %i, off %u len %u\n", type, matchoff, matchlen);
 
@@ -86,21 +92,28 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	 * this one. */
 	exp->expectfn = nf_nat_follow_master;
 
+	/* Avoid applying nat->range to the reply direction */
+	if (!exp->dir || !nat->range_info.min_proto.all || !nat->range_info.max_proto.all) {
+		min = ntohs(exp->saved_proto.tcp.port);
+		range_size = 65535 - min + 1;
+	} else {
+		min = ntohs(nat->range_info.min_proto.all);
+		range_size = ntohs(nat->range_info.max_proto.all) - min + 1;
+	}
+
 	/* Try to get same port: if not, try to change it. */
-	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
-		int ret;
+	first_port = ntohs(exp->saved_proto.tcp.port);
+	if (min > first_port || first_port > (min + range_size - 1))
+		first_port = min;
 
+	for (i = 0, port = first_port; i < range_size; i++, port = (port - first_port + i) % range_size) {
 		exp->tuple.dst.u.tcp.port = htons(port);
 		ret = nf_ct_expect_related(exp, 0);
-		if (ret == 0)
-			break;
-		else if (ret != -EBUSY) {
-			port = 0;
+		if (ret != -EBUSY)
 			break;
-		}
 	}
 
-	if (port == 0) {
+	if (ret != 0) {
 		nf_ct_helper_log(skb, ct, "all ports in use");
 		return NF_DROP;
 	}
diff --git a/net/netfilter/nf_nat_helper.c b/net/netfilter/nf_nat_helper.c
index a263505455fc..718fc423bc44 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -188,6 +188,16 @@ void nf_nat_follow_master(struct nf_conn *ct,
 	range.flags = NF_NAT_RANGE_MAP_IPS;
 	range.min_addr = range.max_addr
 		= ct->master->tuplehash[!exp->dir].tuple.dst.u3;
+	if (!exp->dir) {
+		struct nf_conn_nat *nat = nfct_nat(exp->master);
+
+		if (nat && nat->range_info.min_proto.all &&
+		    nat->range_info.max_proto.all) {
+			range.min_proto = nat->range_info.min_proto;
+			range.max_proto = nat->range_info.max_proto;
+			range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
+		}
+	}
 	nf_nat_setup_info(ct, &range, NF_NAT_MANIP_SRC);
 
 	/* For DST manip, map port here to where it's expected. */
-- 
2.33.0

Re: [PATCH net v4] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Florian Westphal]

Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
> +	/* Avoid applying nat->range to the reply direction */
> +	if (!exp->dir || !nat->range_info.min_proto.all || !nat->range_info.max_proto.all) {
> +		min = ntohs(exp->saved_proto.tcp.port);
> +		range_size = 65535 - min + 1;
> +	} else {
> +		min = ntohs(nat->range_info.min_proto.all);
> +		range_size = ntohs(nat->range_info.max_proto.all) - min + 1;
> +	}
> +
>  	/* Try to get same port: if not, try to change it. */
> -	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
> -		int ret;
> +	first_port = ntohs(exp->saved_proto.tcp.port);
> +	if (min > first_port || first_port > (min + range_size - 1))
> +		first_port = min;
>  
> +	for (i = 0, port = first_port; i < range_size; i++, port = (port - first_port + i) % range_size) {

This looks complicated.  As far as I understand, this could instead be
written like this (not even compile tested):

	/* Avoid applying nat->range to the reply direction */
	if (!exp->dir || !nat->range_info.min_proto.all || !nat->range_info.max_proto.all) {
		min = 1;
		max = 65535;
		range_size = 65535;
	} else {
		min = ntohs(nat->range_info.min_proto.all);
		max = ntohs(nat->range_info.max_proto.all);
		range_size = max - min + 1;
	}

  	/* Try to get same port: if not, try to change it. */
	port = ntohs(exp->saved_proto.tcp.port);

	if (port < min || port > max)
		port = min;

	for (i = 0; i < range_size; i++) {
  		exp->tuple.dst.u.tcp.port = htons(port);
  		ret = nf_ct_expect_related(exp, 0);
		if (ret != -EBUSY)
 			break;
		port++;
		if (port > max)
			port = min;
  	}

	if (ret != 0) {
	...

AFAICS this is the same, we loop at most range_size times,
in case range_size is 64k, we will loop through all (hmmm,
not good actually, but better make that a different change)
else through given min - max range.

If orig port was in-range, we try it first, then increment.
If port exceeds upper bound, cycle back to min.

What do you think?

Re: [PATCH net v4] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Cole Dishington]

On Thu, 2021-09-16 at 13:26 +0200, Florian Westphal wrote:
> Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
> > +	/* Avoid applying nat->range to the reply direction */
> > +	if (!exp->dir || !nat->range_info.min_proto.all || !nat-
> > >range_info.max_proto.all) {
> > +		min = ntohs(exp->saved_proto.tcp.port);
> > +		range_size = 65535 - min + 1;
> > +	} else {
> > +		min = ntohs(nat->range_info.min_proto.all);
> > +		range_size = ntohs(nat->range_info.max_proto.all) - min
> > + 1;
> > +	}
> > +
> >  	/* Try to get same port: if not, try to change it. */
> > -	for (port = ntohs(exp->saved_proto.tcp.port); port != 0;
> > port++) {
> > -		int ret;
> > +	first_port = ntohs(exp->saved_proto.tcp.port);
> > +	if (min > first_port || first_port > (min + range_size - 1))
> > +		first_port = min;
> >  
> > +	for (i = 0, port = first_port; i < range_size; i++, port =
> > (port - first_port + i) % range_size) {
> 
> This looks complicated.  As far as I understand, this could instead
> be
> written like this (not even compile tested):
> 
> 	/* Avoid applying nat->range to the reply direction */
> 	if (!exp->dir || !nat->range_info.min_proto.all || !nat-
> >range_info.max_proto.all) {
> 		min = 1;
> 		max = 65535;
> 		range_size = 65535;
> 	} else {
> 		min = ntohs(nat->range_info.min_proto.all);
> 		max = ntohs(nat->range_info.max_proto.all);
> 		range_size = max - min + 1;
> 	}

The original code defined the range as [ntohs(exp->saved_proto.tcp.port), 65535]. The above would
cause a change in behaviour, should we try to avoid it?

> 
>   	/* Try to get same port: if not, try to change it. */
> 	port = ntohs(exp->saved_proto.tcp.port);
> 
> 	if (port < min || port > max)
> 		port = min;
> 
> 	for (i = 0; i < range_size; i++) {
>   		exp->tuple.dst.u.tcp.port = htons(port);
>   		ret = nf_ct_expect_related(exp, 0);
> 		if (ret != -EBUSY)
>  			break;
> 		port++;
> 		if (port > max)
> 			port = min;
>   	}
> 
> 	if (ret != 0) {
> 	...
> 
> AFAICS this is the same, we loop at most range_size times,
> in case range_size is 64k, we will loop through all (hmmm,
> not good actually, but better make that a different change)
> else through given min - max range.
> 
> If orig port was in-range, we try it first, then increment.
> If port exceeds upper bound, cycle back to min.
> 
> What do you think?
Looks good, just the one question above.

Thanks for your time reviewing!

Re: [PATCH net v4] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Florian Westphal]

Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
> On Thu, 2021-09-16 at 13:26 +0200, Florian Westphal wrote:
> > >range_info.max_proto.all) {
> > 		min = 1;
> > 		max = 65535;
> > 		range_size = 65535;
> > 	} else {
> > 		min = ntohs(nat->range_info.min_proto.all);
> > 		max = ntohs(nat->range_info.max_proto.all);
> > 		range_size = max - min + 1;
> > 	}
> 
> The original code defined the range as [ntohs(exp->saved_proto.tcp.port), 65535]. The above would
> cause a change in behaviour, should we try to avoid it?

Oh indeed, oversight on my part.  Good question, current loop
is not good either as it might take too long to complete.

Maybe best to limit/cap the range to 128, e.g. try to use port
as-is, then pick a random value between 1024 and 65535 - 128,
make 128 tries and if all is taken, error out.

I will leave it up to you on how you'd like to handle this.

One way would be to make a small preparation patch and then
built the range patch on top of it.