diff mbox series

[net,v5,2/2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

Message ID 20210920005905.9583-3-Cole.Dishington@alliedtelesis.co.nz (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 53 this patch: 53
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 92 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 51 this patch: 51
netdev/header_inline success Link

Commit Message

Cole Dishington Sept. 20, 2021, 12:59 a.m. UTC 
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(-)

Comments

Florian Westphal Sept. 20, 2021, 7:23 a.m. UTC | #1 
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>
diff mbox series

Patch

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. */