Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
From: Florian Westphal <fw@strlen.de>
Date: 2021-09-07 13:55:11
Also in:
lkml, netfilter-devel
Cole Dishington [off-list ref] wrote:
quoted hunk ↗ jump to hunk
index aace6768a64e..cf675dc589be 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);
Please add this to a header, e.g. include/net/netfilter/nf_nat.h.
- /* Try to get same port: if not, try to change it. */
- for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) {
- int ret;
+ if (htons(nat->range_info.min_proto.all) == 0 ||
+ htons(nat->range_info.max_proto.all) == 0) {Either use if (nat->range_info.min_proto.all || ... or use ntohs(). I will leave it up to you if you prefer ntohs(nat->range_info.min_proto.all) == 0 or nat->range_info.min_proto.all == ntohs(0). (Use of htons here will trigger endian warnings from sparse tool).
- exp->tuple.dst.u.tcp.port = htons(port);
+ /* Try to get same port if it matches NAT rule: if not, try to change it. */
+ ret = -1;
+ port = ntohs(exp->tuple.dst.u.tcp.port);
+ if (port != 0 && ntohs(range.min_proto.all) <= port &&
+ port <= ntohs(range.max_proto.all)) {
ret = nf_ct_expect_related(exp, 0);
- if (ret == 0)
- break;
- else if (ret != -EBUSY) {
- port = 0;
- break;
+ }
+ if (ret != 0 || port == 0) {
+ if (!dir) {
+ nf_nat_l4proto_unique_tuple(&exp->tuple, &range,
+ NF_NAT_MANIP_DST,
+ ct);
A small comment that explains why nf_nat_l4proto_unique_tuple() is
called conditionally would be good.
I don't understand this new logic, can you explain?
Old:
for (port = expr>tuple.port ; port > 0 ;port++)
nf_ct_expect_related(exp, 0);
if (success || fatal_error)
break;
New:
port = exp->tuple.port;
if (port && min <= port && port <= max) // in which case is port 0 here?
ret = nf_ct_expect_related();
if (fatal_error || port == 0) // how can port be 0?
if (!dir) {
nf_nat_l4proto_unique_tuple();
ret = nf_ct_expect_related();
}
}
How can this work? This removes the loop and relies on
nf_nat_l4proto_unique_tuple(), but NF_NAT_MANIP_DST doesn't support
port rewrite in !NF_NAT_RANGE_PROTO_SPECIFIED case.
Plus, it restricts nf_nat_l4proto_unique_tuple to !dir case, which
I don't understand either.
+ port = ntohs(exp->tuple.dst.u.tcp.port);
+ ret = nf_ct_expect_related(exp, 0);
}
-
- if (port == 0) {
+ if (ret != 0 || port == 0) {How can port be 0? In the old code, it becomes 0 if all attempts to find unused port failed, but after the rewrite I don't see how it can happen.
quoted hunk ↗ jump to hunk
@@ -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->master && !exp->dir) {
AFAIK exp->master can't be NULL.
+ struct nf_conn_nat *nat = nfct_nat(exp->master);
+
+ if (nat && nat->range_info.min_proto.all != 0 &&
+ nat->range_info.max_proto.all != 0) {
+ range.min_proto = nat->range_info.min_proto;
+ range.max_proto = nat->range_info.max_proto;
+ range.flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
+ }
+ }!expr->dir means REPLY, i.e. new connection is reversed compared to the master connection (from responder back to initiator). So, why are we munging range in this case? I would have expected exp->dir == IP_CT_DIR_ORIGINAL for your use case (original connection subject to masquerade and source ports mangled to fall into special range, so related conntion should also be mangled to match said range).