[PATCH net v5 0/2] Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[PATCH net v5 0/2] Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Cole Dishington]

Thanks for your time reviewing!

Changes:
Divide changes into two patches
- Limit the number of ports checked by ftp helper to 128.
- Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED.

Cole Dishington (2):
  net: netfilter: Limit the number of ftp helper port attempts
  net: netfilter: Fix port selection of FTP for
    NF_NAT_RANGE_PROTO_SPECIFIED

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

-- 
2.33.0

[PATCH net v5 1/2] net: netfilter: Limit the number of ftp helper port attempts

[Cole Dishington]

In preparation of fixing the port selection of ftp helper when using
NF_NAT_RANGE_PROTO_SPECIFIED, limit the number of ftp helper port
attempts to 128.

Looping a large port range takes too long. Instead select a random
offset within [ntohs(exp->saved_proto.tcp.port), 65535] and try 128
ports.

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>
---
 net/netfilter/nf_nat_ftp.c | 39 +++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c
index aace6768a64e..7dcb4f179ac9 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -72,8 +72,11 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	u_int16_t port;
 	int dir = CTINFO2DIR(ctinfo);
 	struct nf_conn *ct = exp->master;
+	unsigned int i, min, max, range_size;
+	static const unsigned int max_attempts = 128;
 	char buffer[sizeof("|1||65535|") + INET6_ADDRSTRLEN];
 	unsigned int buflen;
+	int ret;
 
 	pr_debug("type %i, off %u len %u\n", type, matchoff, matchlen);
 
@@ -86,22 +89,32 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	 * this one. */
 	exp->expectfn = nf_nat_follow_master;
 
-	/* Try to get same port: if not, try to change it. */
-	for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
-		int ret;
-
-		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;
-			break;
+	min = ntohs(exp->saved_proto.tcp.port);
+	max = 65535;
+
+	/* Try to get same port */
+	ret = nf_ct_expect_related(exp, 0);
+
+	/* if same port is not in range or available, try to change it. */
+	if (ret != 0) {
+		range_size = max - min + 1;
+		if (range_size > max_attempts)
+			range_size = max_attempts;
+
+		port = min + prandom_u32_max(max - 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 (port == 0) {
-		nf_ct_helper_log(skb, ct, "all ports in use");
+	if (ret != 0) {
+		nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
 		return NF_DROP;
 	}
 
-- 
2.33.0

Re: [PATCH net v5 1/2] net: netfilter: Limit the number of ftp helper port attempts

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4318 bytes --]

Hi Cole,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Cole-Dishington/Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210920-090056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git e30cd812dffadc58241ae378e48728e6a161becd
config: i386-defconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/b90b875dc5be3c59ec418ce403a8d749690a24ec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cole-Dishington/Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210920-090056
        git checkout b90b875dc5be3c59ec418ce403a8d749690a24ec
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   net/netfilter/nf_nat_ftp.c: In function 'nf_nat_ftp':
>> net/netfilter/nf_nat_ftp.c:117:37: warning: format '%u' expects a matching 'unsigned int' argument [-Wformat=]
     117 |   nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
         |                                    ~^
         |                                     |
         |                                     unsigned int


vim +117 net/netfilter/nf_nat_ftp.c

    60	
    61	/* So, this packet has hit the connection tracking matching code.
    62	   Mangle it, and change the expectation to match the new version. */
    63	static unsigned int nf_nat_ftp(struct sk_buff *skb,
    64				       enum ip_conntrack_info ctinfo,
    65				       enum nf_ct_ftp_type type,
    66				       unsigned int protoff,
    67				       unsigned int matchoff,
    68				       unsigned int matchlen,
    69				       struct nf_conntrack_expect *exp)
    70	{
    71		union nf_inet_addr newaddr;
    72		u_int16_t port;
    73		int dir = CTINFO2DIR(ctinfo);
    74		struct nf_conn *ct = exp->master;
    75		unsigned int i, min, max, range_size;
    76		static const unsigned int max_attempts = 128;
    77		char buffer[sizeof("|1||65535|") + INET6_ADDRSTRLEN];
    78		unsigned int buflen;
    79		int ret;
    80	
    81		pr_debug("type %i, off %u len %u\n", type, matchoff, matchlen);
    82	
    83		/* Connection will come from wherever this packet goes, hence !dir */
    84		newaddr = ct->tuplehash[!dir].tuple.dst.u3;
    85		exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
    86		exp->dir = !dir;
    87	
    88		/* When you see the packet, we need to NAT it the same as the
    89		 * this one. */
    90		exp->expectfn = nf_nat_follow_master;
    91	
    92		min = ntohs(exp->saved_proto.tcp.port);
    93		max = 65535;
    94	
    95		/* Try to get same port */
    96		ret = nf_ct_expect_related(exp, 0);
    97	
    98		/* if same port is not in range or available, try to change it. */
    99		if (ret != 0) {
   100			range_size = max - min + 1;
   101			if (range_size > max_attempts)
   102				range_size = max_attempts;
   103	
   104			port = min + prandom_u32_max(max - min);
   105			for (i = 0; i < range_size; i++) {
   106				exp->tuple.dst.u.tcp.port = htons(port);
   107				ret = nf_ct_expect_related(exp, 0);
   108				if (ret != -EBUSY)
   109					break;
   110				port++;
   111				if (port > max)
   112					port = min;
   113			}
   114		}
   115	
   116		if (ret != 0) {
 > 117			nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
   118			return NF_DROP;
   119		}
   120	
   121		buflen = nf_nat_ftp_fmt_cmd(ct, type, buffer, sizeof(buffer),
   122					    &newaddr, port);
   123		if (!buflen)
   124			goto out;
   125	
   126		pr_debug("calling nf_nat_mangle_tcp_packet\n");
   127	
   128		if (!nf_nat_mangle_tcp_packet(skb, ct, ctinfo, protoff, matchoff,
   129					      matchlen, buffer, buflen))
   130			goto out;
   131	
   132		return NF_ACCEPT;
   133	
   134	out:
   135		nf_ct_helper_log(skb, ct, "cannot mangle packet");
   136		nf_ct_unexpect_related(exp);
   137		return NF_DROP;
   138	}
   139	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29546 bytes --]

Re: [PATCH net v5 1/2] net: netfilter: Limit the number of ftp helper port attempts

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4475 bytes --]

Hi Cole,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net/master]

url:    https://github.com/0day-ci/linux/commits/Cole-Dishington/Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210920-090056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git e30cd812dffadc58241ae378e48728e6a161becd
config: x86_64-randconfig-a002-20210920 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c8b3d7d6d6de37af68b2f379d0e37304f78e115f)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b90b875dc5be3c59ec418ce403a8d749690a24ec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cole-Dishington/Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210920-090056
        git checkout b90b875dc5be3c59ec418ce403a8d749690a24ec
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/netfilter/nf_nat_ftp.c:117:37: warning: more '%' conversions than data arguments [-Wformat-insufficient-args]
                   nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
                                                    ~^
   1 warning generated.


vim +117 net/netfilter/nf_nat_ftp.c

    60	
    61	/* So, this packet has hit the connection tracking matching code.
    62	   Mangle it, and change the expectation to match the new version. */
    63	static unsigned int nf_nat_ftp(struct sk_buff *skb,
    64				       enum ip_conntrack_info ctinfo,
    65				       enum nf_ct_ftp_type type,
    66				       unsigned int protoff,
    67				       unsigned int matchoff,
    68				       unsigned int matchlen,
    69				       struct nf_conntrack_expect *exp)
    70	{
    71		union nf_inet_addr newaddr;
    72		u_int16_t port;
    73		int dir = CTINFO2DIR(ctinfo);
    74		struct nf_conn *ct = exp->master;
    75		unsigned int i, min, max, range_size;
    76		static const unsigned int max_attempts = 128;
    77		char buffer[sizeof("|1||65535|") + INET6_ADDRSTRLEN];
    78		unsigned int buflen;
    79		int ret;
    80	
    81		pr_debug("type %i, off %u len %u\n", type, matchoff, matchlen);
    82	
    83		/* Connection will come from wherever this packet goes, hence !dir */
    84		newaddr = ct->tuplehash[!dir].tuple.dst.u3;
    85		exp->saved_proto.tcp.port = exp->tuple.dst.u.tcp.port;
    86		exp->dir = !dir;
    87	
    88		/* When you see the packet, we need to NAT it the same as the
    89		 * this one. */
    90		exp->expectfn = nf_nat_follow_master;
    91	
    92		min = ntohs(exp->saved_proto.tcp.port);
    93		max = 65535;
    94	
    95		/* Try to get same port */
    96		ret = nf_ct_expect_related(exp, 0);
    97	
    98		/* if same port is not in range or available, try to change it. */
    99		if (ret != 0) {
   100			range_size = max - min + 1;
   101			if (range_size > max_attempts)
   102				range_size = max_attempts;
   103	
   104			port = min + prandom_u32_max(max - min);
   105			for (i = 0; i < range_size; i++) {
   106				exp->tuple.dst.u.tcp.port = htons(port);
   107				ret = nf_ct_expect_related(exp, 0);
   108				if (ret != -EBUSY)
   109					break;
   110				port++;
   111				if (port > max)
   112					port = min;
   113			}
   114		}
   115	
   116		if (ret != 0) {
 > 117			nf_ct_helper_log(skb, ct, "tried %u ports, all were in use");
   118			return NF_DROP;
   119		}
   120	
   121		buflen = nf_nat_ftp_fmt_cmd(ct, type, buffer, sizeof(buffer),
   122					    &newaddr, port);
   123		if (!buflen)
   124			goto out;
   125	
   126		pr_debug("calling nf_nat_mangle_tcp_packet\n");
   127	
   128		if (!nf_nat_mangle_tcp_packet(skb, ct, ctinfo, protoff, matchoff,
   129					      matchlen, buffer, buflen))
   130			goto out;
   131	
   132		return NF_ACCEPT;
   133	
   134	out:
   135		nf_ct_helper_log(skb, ct, "cannot mangle packet");
   136		nf_ct_unexpect_related(exp);
   137		return NF_DROP;
   138	}
   139	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37628 bytes --]

Re: [PATCH net v5 1/2] net: netfilter: Limit the number of ftp helper port attempts

[Florian Westphal]

Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
> In preparation of fixing the port selection of ftp helper when using
> NF_NAT_RANGE_PROTO_SPECIFIED, limit the number of ftp helper port
> attempts to 128.
> 
> Looping a large port range takes too long. Instead select a random
> offset within [ntohs(exp->saved_proto.tcp.port), 65535] and try 128
> ports.

LGTM, please fix the format argument error the kbuild robot reported
and resend.  You may add

Acked-by: Florian Westphal <fw@strlen.de>

when doing so.

[PATCH net v5 2/2] 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>
---
 include/net/netfilter/nf_nat.h |  6 ++++++
 net/netfilter/nf_nat_core.c    |  9 +++++++++
 net/netfilter/nf_nat_ftp.c     | 22 +++++++++++++++++++---
 net/netfilter/nf_nat_helper.c  | 10 ++++++++++
 4 files changed, 44 insertions(+), 3 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 b7c3c902290f..2acec7fd56bd 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 7dcb4f179ac9..baf353932cd4 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -72,12 +72,16 @@ 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, min, max, range_size;
 	static const unsigned int max_attempts = 128;
 	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);
 
 	/* Connection will come from wherever this packet goes, hence !dir */
@@ -89,11 +93,23 @@ static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	 * this one. */
 	exp->expectfn = nf_nat_follow_master;
 
-	min = ntohs(exp->saved_proto.tcp.port);
-	max = 65535;
+	/* 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);
+		max = 65535;
+	} else {
+		min = ntohs(nat->range_info.min_proto.all);
+		max = ntohs(nat->range_info.max_proto.all);
+		if (unlikely(max < min))
+			swap(max, min);
+	}
 
 	/* Try to get same port */
-	ret = nf_ct_expect_related(exp, 0);
+	ret = -1;
+	port = ntohs(exp->saved_proto.tcp.port);
+	if (min < port && port < max) {
+		ret = nf_ct_expect_related(exp, 0);
+	}
 
 	/* if same port is not in range or available, try to change it. */
 	if (ret != 0) {
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 v5 2/2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

[Florian Westphal]

Cole Dishington <Cole.Dishington@alliedtelesis.co.nz> wrote:
> 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.

Looks much simpler now, thanks.

Acked-by: Florian Westphal <fw@strlen.de>