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 <[email protected]>
Signed-off-by: Anthony Lineham <[email protected]>
Co-developed-by: Scott Parlane <[email protected]>
Signed-off-by: Scott Parlane <[email protected]>
Co-developed-by: Blair Steven <[email protected]>
Signed-off-by: Blair Steven <[email protected]>
Signed-off-by: Cole Dishington <[email protected]>
---
Notes:
Thanks for your time reviewing!
Changes:
- Avoid storing full range structure, only store min_proto and max_proto.
- Store min and max_proto on nf_conn_nat rather than nf_conn.
- Only store extra range info on nf_conn if NF_NAT_RANGE_PROTO_SPECIFIED and
fallback to selecting from full port range.
- Try to get same port if it matches the range.
- Added net tag to subject.
include/net/netfilter/nf_nat.h | 6 +++++
net/netfilter/nf_nat_core.c | 17 +++++++++----
net/netfilter/nf_nat_ftp.c | 44 +++++++++++++++++++++++++---------
net/netfilter/nf_nat_helper.c | 10 ++++++++
4 files changed, 62 insertions(+), 15 deletions(-)
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 ea923f8cf9c4..5412e9cf8189 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;
@@ -601,6 +601,7 @@ nf_nat_setup_info(struct nf_conn *ct,
const struct nf_nat_range2 *range,
enum nf_nat_manip_type maniptype)
{
+ struct nf_conn_nat *nat;
struct net *net = nf_ct_net(ct);
struct nf_conntrack_tuple curr_tuple, new_tuple;
@@ -623,6 +624,14 @@ 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)) {
+ 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..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);
#define NAT_HELPER_NAME "ftp"
@@ -72,8 +76,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 +96,33 @@ 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;
+ if (htons(nat->range_info.min_proto.all) == 0 ||
+ htons(nat->range_info.max_proto.all) == 0) {
+ 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;
- 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);
}
+ port = ntohs(exp->tuple.dst.u.tcp.port);
+ ret = nf_ct_expect_related(exp, 0);
}
-
- if (port == 0) {
+ if (ret != 0 || 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..29fa78cfdb9c 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->master && !exp->dir) {
+ 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;
+ }
+ }
nf_nat_setup_info(ct, &range, NF_NAT_MANIP_SRC);
/* For DST manip, map port here to where it's expected. */
--
2.33.0
Hi Cole,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net/master]
url: https://github.com/0day-ci/linux/commits/Cole-Dishington/net-netfilter-Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210907-101823
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git b539c44df067ac116ec1b58b956efda51b6a7fc1
config: arm-randconfig-r003-20210906 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9c476172b93367d2cb88d7d3f4b1b5b456fa6020)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/3d790f5d7c3d6069948749b4697090adfcc48e51
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Cole-Dishington/net-netfilter-Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210907-101823
git checkout 3d790f5d7c3d6069948749b4697090adfcc48e51
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> net/netfilter/nf_nat_core.c:373:6: warning: no previous prototype for function 'nf_nat_l4proto_unique_tuple' [-Wmissing-prototypes]
void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
^
net/netfilter/nf_nat_core.c:373:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
^
static
1 warning generated.
vim +/nf_nat_l4proto_unique_tuple +373 net/netfilter/nf_nat_core.c
367
368 /* Alter the per-proto part of the tuple (depending on maniptype), to
369 * give a unique tuple in the given range if possible.
370 *
371 * Per-protocol part of tuple is initialized to the incoming packet.
372 */
> 373 void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
374 const struct nf_nat_range2 *range,
375 enum nf_nat_manip_type maniptype,
376 const struct nf_conn *ct)
377 {
378 unsigned int range_size, min, max, i, attempts;
379 __be16 *keyptr;
380 u16 off;
381 static const unsigned int max_attempts = 128;
382
383 switch (tuple->dst.protonum) {
384 case IPPROTO_ICMP:
385 case IPPROTO_ICMPV6:
386 /* id is same for either direction... */
387 keyptr = &tuple->src.u.icmp.id;
388 if (!(range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
389 min = 0;
390 range_size = 65536;
391 } else {
392 min = ntohs(range->min_proto.icmp.id);
393 range_size = ntohs(range->max_proto.icmp.id) -
394 ntohs(range->min_proto.icmp.id) + 1;
395 }
396 goto find_free_id;
397 #if IS_ENABLED(CONFIG_NF_CT_PROTO_GRE)
398 case IPPROTO_GRE:
399 /* If there is no master conntrack we are not PPTP,
400 do not change tuples */
401 if (!ct->master)
402 return;
403
404 if (maniptype == NF_NAT_MANIP_SRC)
405 keyptr = &tuple->src.u.gre.key;
406 else
407 keyptr = &tuple->dst.u.gre.key;
408
409 if (!(range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
410 min = 1;
411 range_size = 65535;
412 } else {
413 min = ntohs(range->min_proto.gre.key);
414 range_size = ntohs(range->max_proto.gre.key) - min + 1;
415 }
416 goto find_free_id;
417 #endif
418 case IPPROTO_UDP:
419 case IPPROTO_UDPLITE:
420 case IPPROTO_TCP:
421 case IPPROTO_SCTP:
422 case IPPROTO_DCCP:
423 if (maniptype == NF_NAT_MANIP_SRC)
424 keyptr = &tuple->src.u.all;
425 else
426 keyptr = &tuple->dst.u.all;
427
428 break;
429 default:
430 return;
431 }
432
433 /* If no range specified... */
434 if (!(range->flags & NF_NAT_RANGE_PROTO_SPECIFIED)) {
435 /* If it's dst rewrite, can't change port */
436 if (maniptype == NF_NAT_MANIP_DST)
437 return;
438
439 if (ntohs(*keyptr) < 1024) {
440 /* Loose convention: >> 512 is credential passing */
441 if (ntohs(*keyptr) < 512) {
442 min = 1;
443 range_size = 511 - min + 1;
444 } else {
445 min = 600;
446 range_size = 1023 - min + 1;
447 }
448 } else {
449 min = 1024;
450 range_size = 65535 - 1024 + 1;
451 }
452 } else {
453 min = ntohs(range->min_proto.all);
454 max = ntohs(range->max_proto.all);
455 if (unlikely(max < min))
456 swap(max, min);
457 range_size = max - min + 1;
458 }
459
460 find_free_id:
461 if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
462 off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
463 else
464 off = prandom_u32();
465
466 attempts = range_size;
467 if (attempts > max_attempts)
468 attempts = max_attempts;
469
470 /* We are in softirq; doing a search of the entire range risks
471 * soft lockup when all tuples are already used.
472 *
473 * If we can't find any free port from first offset, pick a new
474 * one and try again, with ever smaller search window.
475 */
476 another_round:
477 for (i = 0; i < attempts; i++, off++) {
478 *keyptr = htons(min + off % range_size);
479 if (!nf_nat_used_tuple(tuple, ct))
480 return;
481 }
482
483 if (attempts >= range_size || attempts < 16)
484 return;
485 attempts /= 2;
486 off = prandom_u32();
487 goto another_round;
488 }
489
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Cole Dishington <[email protected]> wrote:
> 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.
> @@ -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).
Hi Cole,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Cole-Dishington/net-netfilter-Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210907-101823
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git b539c44df067ac116ec1b58b956efda51b6a7fc1
config: s390-defconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3d790f5d7c3d6069948749b4697090adfcc48e51
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Cole-Dishington/net-netfilter-Fix-port-selection-of-FTP-for-NF_NAT_RANGE_PROTO_SPECIFIED/20210907-101823
git checkout 3d790f5d7c3d6069948749b4697090adfcc48e51
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "nf_nat_l4proto_unique_tuple" [net/netfilter/nf_nat_ftp.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Tuesday 2021-09-07 15:54, Florian Westphal wrote:
>> - /* 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).
If one has the option, one should always prefer to put htons/htonl on
the side with the constant literal;
Propagation of constants and compile-time evaluation is the target.
That works for some other functions as well (e.g.
strlen("fixedstring")).
On Tue, Sep 07, 2021 at 05:11:42PM +0200, Jan Engelhardt wrote:
>
> On Tuesday 2021-09-07 15:54, Florian Westphal wrote:
> >> - /* 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).
>
> If one has the option, one should always prefer to put htons/htonl on
> the side with the constant literal;
> Propagation of constants and compile-time evaluation is the target.
>
> That works for some other functions as well (e.g.
> strlen("fixedstring")).
When comparing against constant zero, why use htons/htonl at all?
Cheers ... Duncan.
On Wednesday 2021-09-08 04:22, Duncan Roe wrote:
>> >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).
>>
>> If one has the option, one should always prefer to put htons/htonl on
>> the side with the constant literal;
>> Propagation of constants and compile-time evaluation is the target.
>>
>> That works for some other functions as well (e.g.
>> strlen("fixedstring")).
>
>When comparing against constant zero, why use htons/htonl at all?
Logical correctness.
Remember, it was the sparse tool that complained in the first place.