Re: [PATCH net v2] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED
flat view
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.
quoted hunk ↗ jump to hunk
- /* 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).
quoted hunk ↗ jump to hunk
- 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ 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).