diff mbox series

net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED

Message ID 20210728032134.21983-1-Cole.Dishington@alliedtelesis.co.nz (mailing list archive)
State New
Headers show
Series net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED | expand

Commit Message

Cole Dishington July 28, 2021, 3:21 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.

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:
    Currently with iptables -t nat -j MASQUERADE -p tcp --to-ports 10000-10005,
    creating a passive ftp connection from a client will result in the control
    connection being within the specified port range but the data connection being
    outside of the range. This patch fixes this behaviour to have both connections
    be in the specified range.

 include/net/netfilter/nf_conntrack.h |  3 +++
 net/netfilter/nf_nat_core.c          | 10 ++++++----
 net/netfilter/nf_nat_ftp.c           | 26 ++++++++++++--------------
 net/netfilter/nf_nat_helper.c        | 12 ++++++++----
 4 files changed, 29 insertions(+), 22 deletions(-)

Comments

Florian Westphal July 28, 2021, 9:06 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.
> 
> 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:
>     Currently with iptables -t nat -j MASQUERADE -p tcp --to-ports 10000-10005,
>     creating a passive ftp connection from a client will result in the control
>     connection being within the specified port range but the data connection being
>     outside of the range. This patch fixes this behaviour to have both connections
>     be in the specified range.
> 
>  include/net/netfilter/nf_conntrack.h |  3 +++
>  net/netfilter/nf_nat_core.c          | 10 ++++++----
>  net/netfilter/nf_nat_ftp.c           | 26 ++++++++++++--------------
>  net/netfilter/nf_nat_helper.c        | 12 ++++++++----
>  4 files changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> index cc663c68ddc4..b98d5d04c7ab 100644
> --- a/include/net/netfilter/nf_conntrack.h
> +++ b/include/net/netfilter/nf_conntrack.h
> @@ -24,6 +24,8 @@
>  
>  #include <net/netfilter/nf_conntrack_tuple.h>
>  
> +#include <uapi/linux/netfilter/nf_nat.h>
> +
>  struct nf_ct_udp {
>  	unsigned long	stream_ts;
>  };
> @@ -99,6 +101,7 @@ struct nf_conn {
>  
>  #if IS_ENABLED(CONFIG_NF_NAT)
>  	struct hlist_node	nat_bysource;
> +	struct nf_nat_range2 range;
>  #endif

Thats almost a 20% size increase of this structure.

Could you try to rework it based on this?
diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
--- 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. */

... and then store the range min/max proto iff nf_nat_setup_info had
NF_NAT_RANGE_PROTO_SPECIFIED flag set.

I don't think there is a need to keep the information in nf_conn.
diff mbox series

Patch

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index cc663c68ddc4..b98d5d04c7ab 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -24,6 +24,8 @@ 
 
 #include <net/netfilter/nf_conntrack_tuple.h>
 
+#include <uapi/linux/netfilter/nf_nat.h>
+
 struct nf_ct_udp {
 	unsigned long	stream_ts;
 };
@@ -99,6 +101,7 @@  struct nf_conn {
 
 #if IS_ENABLED(CONFIG_NF_NAT)
 	struct hlist_node	nat_bysource;
+	struct nf_nat_range2 range;
 #endif
 	/* all members below initialized via memset */
 	struct { } __nfct_init_offset;
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 7de595ead06a..4772c8457ef2 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -360,10 +360,10 @@  find_best_ips_proto(const struct nf_conntrack_zone *zone,
  *
  * Per-protocol part of tuple is initialized to the incoming packet.
  */
-static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
-					const struct nf_nat_range2 *range,
-					enum nf_nat_manip_type maniptype,
-					const struct nf_conn *ct)
+void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
+				 const struct nf_nat_range2 *range,
+				 enum nf_nat_manip_type maniptype,
+				 const struct nf_conn *ct)
 {
 	unsigned int range_size, min, max, i, attempts;
 	__be16 *keyptr;
@@ -586,6 +586,8 @@  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)
+		ct->range = *range;
 
 	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..6794aa77162b 100644
--- a/net/netfilter/nf_nat_ftp.c
+++ b/net/netfilter/nf_nat_ftp.c
@@ -17,6 +17,10 @@ 
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_expect.h>
 #include <linux/netfilter/nf_conntrack_ftp.h>
+void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
+				 const struct nf_nat_range2 *range,
+				 enum nf_nat_manip_type maniptype,
+				 const struct nf_conn *ct);
 
 #define NAT_HELPER_NAME "ftp"
 
@@ -74,6 +78,7 @@  static unsigned int nf_nat_ftp(struct sk_buff *skb,
 	struct nf_conn *ct = exp->master;
 	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,21 +91,14 @@  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;
-		}
-	}
+	/* Find a port that matches the NAT rule */
+	nf_nat_l4proto_unique_tuple(&exp->tuple, &ct->range,
+				    dir ? NF_NAT_MANIP_SRC : NF_NAT_MANIP_DST,
+				    ct);
+	port = ntohs(exp->tuple.dst.u.tcp.port);
+	ret = nf_ct_expect_related(exp, 0);
 
-	if (port == 0) {
+	if ((ret != 0 && ret != -EBUSY) || port == 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..912bf50be58a 100644
--- a/net/netfilter/nf_nat_helper.c
+++ b/net/netfilter/nf_nat_helper.c
@@ -184,10 +184,14 @@  void nf_nat_follow_master(struct nf_conn *ct,
 	/* This must be a fresh one. */
 	BUG_ON(ct->status & IPS_NAT_DONE_MASK);
 
-	/* Change src to where master sends to */
-	range.flags = NF_NAT_RANGE_MAP_IPS;
-	range.min_addr = range.max_addr
-		= ct->master->tuplehash[!exp->dir].tuple.dst.u3;
+	if (exp->master && !exp->dir) {
+		range = exp->master->range;
+	} else {
+		/* Change src to where master sends to */
+		range.flags = NF_NAT_RANGE_MAP_IPS;
+		range.min_addr = ct->master->tuplehash[!exp->dir].tuple.dst.u3;
+		range.max_addr = ct->master->tuplehash[!exp->dir].tuple.dst.u3;
+	}
 	nf_nat_setup_info(ct, &range, NF_NAT_MANIP_SRC);
 
 	/* For DST manip, map port here to where it's expected. */