| Message ID | 20210914233006.8710-1-Cole.Dishington@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v3] net: netfilter: Fix port selection of FTP for NF_NAT_RANGE_PROTO_SPECIFIED | expand |
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. Can you elaborate here? Why is this a problem, how to address it, etc? > 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: > Thanks for your time reviewing! > > Changes: > - Removed check for port == 0. > - Added nf_nat_l4proto_unique_tuple() to include/net/netfilter/nf_nat.h. > - Simplify htons/ntohs calls. > - Move away from conditional call of nf_nat_l4proto_unique_tuple() to applying full range to dst port > if ftp active with PORT/ePORT is used. > - Remove check for exp->master != NULL. > > Comments: > - dir is the direction of the ftp PORT/PASV request, so exp->dir is the direction > of the data connection. In nat helper, the range should be applied on !exp->dir > source port since this is the connection from the client. This should be part of the commit log, or augment your ... /* Avoid applying range to external ports */ ... comment in the source code. > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c > index ea923f8cf9c4..edfd72524c38 100644 > --- a/net/netfilter/nf_nat_core.c > +++ b/net/netfilter/nf_nat_core.c > @@ -397,10 +397,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) > { Are you sure this doesn't need an EXPORT_SYMBOL_GPL()? I seem to recall seeing a buildbot linker error because of this. > 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 (WARN_ON_ONCE(!nat)) > + return NF_DROP; > + This WARN needs to be removed, nf_ct_nat_ext_add() can fail if memory is low. WARN signals 'cannot happen, if it does something else is wrong and has to be debugged'. > diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c > index aace6768a64e..f14e53e8dc04 100644 > --- a/net/netfilter/nf_nat_ftp.c > +++ b/net/netfilter/nf_nat_ftp.c > - /* Try to get same port: if not, try to change it. */ > - for (port = ntohs(exp->saved_proto.tcp.port); port != 0; port++) { > - int ret; > + /* Avoid applying range to external ports */ > + if (!exp->dir || !nat->range_info.min_proto.all || !nat->range_info.max_proto.all) { > + range.min_proto.all = htons(1); > + range.max_proto.all = htons(65535); > + } else { > + range.min_proto = nat->range_info.min_proto; > + range.max_proto = nat->range_info.max_proto; > + } > + range.flags = NF_NAT_RANGE_PROTO_SPECIFIED; > + > + /* Try to get same port if it matches range */ > + ret = -1; > + port = ntohs(exp->tuple.dst.u.tcp.port); > + if (ntohs(range.min_proto.all) <= port && port <= ntohs(range.max_proto.all)) > + ret = nf_ct_expect_related(exp, 0); > > - exp->tuple.dst.u.tcp.port = htons(port); > + /* Same port is not available, try to change it */ > + if (ret != 0) { > + nf_nat_l4proto_unique_tuple(&exp->tuple, &range, NF_NAT_MANIP_DST, ct); This looks buggy. nf_nat_l4proto_unique_tuple() uses 'ct' as 'please ignore this one' when checking for port collisions. But the way its used here, 'ct' has little to do with the future tuple. So, as far as i can see, this needs a fixup to also detect when exp->tuple reply direction would collide with the existing ct, no?
diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h index 0d412dd63707..89796ed2aad3 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. */ @@ -40,6 +46,11 @@ unsigned int nf_nat_setup_info(struct nf_conn *ct, const struct nf_nat_range2 *range, enum nf_nat_manip_type maniptype); +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); + extern unsigned int nf_nat_alloc_null_binding(struct nf_conn *ct, unsigned int hooknum); diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c index ea923f8cf9c4..edfd72524c38 100644 --- a/net/netfilter/nf_nat_core.c +++ b/net/netfilter/nf_nat_core.c @@ -397,10 +397,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; @@ -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 (WARN_ON_ONCE(!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 aace6768a64e..f14e53e8dc04 100644 --- a/net/netfilter/nf_nat_ftp.c +++ b/net/netfilter/nf_nat_ftp.c @@ -72,8 +72,14 @@ 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); + struct nf_nat_range2 range = {}; 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); @@ -86,21 +92,29 @@ 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; + /* Avoid applying range to external ports */ + if (!exp->dir || !nat->range_info.min_proto.all || !nat->range_info.max_proto.all) { + range.min_proto.all = htons(1); + range.max_proto.all = htons(65535); + } else { + range.min_proto = nat->range_info.min_proto; + range.max_proto = nat->range_info.max_proto; + } + range.flags = NF_NAT_RANGE_PROTO_SPECIFIED; + + /* Try to get same port if it matches range */ + ret = -1; + port = ntohs(exp->tuple.dst.u.tcp.port); + if (ntohs(range.min_proto.all) <= port && port <= ntohs(range.max_proto.all)) + ret = nf_ct_expect_related(exp, 0); - exp->tuple.dst.u.tcp.port = htons(port); + /* Same port is not available, try to change it */ + if (ret != 0) { + nf_nat_l4proto_unique_tuple(&exp->tuple, &range, NF_NAT_MANIP_DST, ct); + port = ntohs(exp->tuple.dst.u.tcp.port); ret = nf_ct_expect_related(exp, 0); - if (ret == 0) - break; - else if (ret != -EBUSY) { - port = 0; - break; - } } - - if (port == 0) { + if (ret != 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..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. */