[RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl

[RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl

[Jiri Kosina]

After I've upgraded backbone router of rather large-ish network to 4.9, 
users started complaining about their GRE / PPTP tunnels not working any 
more.

Long time of staring into code revealed that 4.9 kernel has

	static bool nf_ct_auto_assign_helper __read_mostly = false;

which causes automatic matching of conntrack helpers not to work any more. 
Turns out the default was flipped in 3bb398d925 ("netfilter: nf_ct_helper: 
disable automatic helper assignment") (*) in 4.7.

Digging further back into history, it turns out that the kernel started to 
print a warning message about automatic helper assignment being deprecated 
in 3.5+; given the fact that this message is ususally burried somewhere 
deep in the boot sequence (and therefore hardly noticed by each and every 
router admin on the planet), and given the fact that this has proven 
itself to severely break at least mine router config (which has been 
working for years), I propose to revert the patches flipping the default. 
Anyone is still of course free to set up an explicit CT-based matching for 
better reliability, but the automatic assignment should stay.

Considering this being really close to the "userspace breakage" 
borderline, I'm CCing Linus as well.

(*) the changelog of that commit is odd by itself as well, as it 
references SHA-1 72110dfaa907, but that doesn't exist in my tree at least.

Jiri Kosina (2):
      Revert "netfilter: nf_ct_helper: disable automatic helper assignment"
      Revert "netfilter: fix nf_conntrack_helper documentation"

 Documentation/networking/nf_conntrack-sysctl.txt | 7 ++-----
 net/netfilter/nf_conntrack_helper.c              | 4 ++--
 2 files changed, 4 insertions(+), 7 deletions(-)

-- 
Jiri Kosina
SUSE Labs

Re: [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl

[Linus Torvalds]

On Mon, Jan 23, 2017 at 4:06 PM, Jiri Kosina <jikos@kernel.org> wrote:
>
> Considering this being really close to the "userspace breakage"
> borderline, I'm CCing Linus as well.

For all I know, there may be some security reason why we really don't
want the automatic helpers, even if they can be convenient.

Also, you can just enable them with a kernel command line or a sysctl,
so it's not like you can't get the old behavior back.

Do networking people have any comments? Was there a reason to actually
switch the default? Because the commit messages aren't all that
helpful.

               Linus

Re: [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl

[Pablo Neira Ayuso]

On Mon, Jan 23, 2017 at 05:09:55PM -0800, Linus Torvalds wrote:
> On Mon, Jan 23, 2017 at 4:06 PM, Jiri Kosina <jikos@kernel.org> wrote:
> >
> > Considering this being really close to the "userspace breakage"
> > borderline, I'm CCing Linus as well.
> 
> For all I know, there may be some security reason why we really don't
> want the automatic helpers, even if they can be convenient.

Yes, with helper modules in place, this is known to allow attackers to
push holes in your firewall.  Eric Leblond actually show that it's
perfectly feasible to exploit this via handcrafted packets [1]. The
problem is documented here [2].

> Also, you can just enable them with a kernel command line or a sysctl,
> so it's not like you can't get the old behavior back.

Right.

[1] https://cansecwest.com/csw12/conntrack-attack.pdf
[2] https://home.regit.org/netfilter-en/secure-use-of-helpers/

[PATCH] netfilter: nf_ct_helper: warn when not applying default helper assignment (was Re: [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl)

[Jiri Kosina]

On Mon, 23 Jan 2017, Linus Torvalds wrote:

> For all I know, there may be some security reason why we really don't
> want the automatic helpers, even if they can be convenient.
> 
> Also, you can just enable them with a kernel command line or a sysctl,
> so it's not like you can't get the old behavior back.

Yeah, the only concern really is causing instant breakage of existing 
firewall configurations just by upgrading the kernel.

On Tue, 24 Jan 2017, Pablo Neira Ayuso wrote:

> Yes, with helper modules in place, this is known to allow attackers to
> push holes in your firewall.  Eric Leblond actually show that it's
> perfectly feasible to exploit this via handcrafted packets [1]. The
> problem is documented here [2].
> 
> > Also, you can just enable them with a kernel command line or a sysctl,
> > so it's not like you can't get the old behavior back.
> 
> Right.
> 
> [1] https://cansecwest.com/csw12/conntrack-attack.pdf
> [2] https://home.regit.org/netfilter-en/secure-use-of-helpers/

Alright, that's a valid reason.

Still, I'd like us to be as helpful as possible when we really have no 
other choice than breaking existing userspace setup.

So how about issuing a warning in case we'd normally perform the automatic 
helper assignment, but we actually don't due to the new default setting? 
The fact that we've had the 'deprecated' warning there since 3.5 is nice, 
but let's face it -- that's not where the poor guy would be debugging why 
his firewall doesn't work. It'd be the kernel with the new default, and 
that doesn't give any hints whatsoever.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] netfilter: nf_ct_helper: warn when not applying default helper assignment

Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper 
assignment") is causing behavior regressions in firewalls, as traffic 
handled by conntrack helpers is now by default not passed through even 
though it was before due to missing CT targets (which were not necessary 
before this commit).

The default had to be switched off due to security reasons [1] [2] and
therefore should stay the way it is, but let's be friendly to firewall
admins and issue a warning the first time we're in situation where packet
would be likely passed through with the old default but we're likely going
to drop it on the floor now.

Re-use the 'net->ct.auto_assign_helper_warned' flag, as it'd be sufficient 
to warn one way or the other.

[1] https://cansecwest.com/csw12/conntrack-attack.pdf
[2] https://home.regit.org/netfilter-en/secure-use-of-helpers/

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 net/netfilter/nf_conntrack_helper.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 7341adf..02a26b0 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -213,17 +213,28 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 	}
 
 	help = nfct_help(ct);
-	if (net->ct.sysctl_auto_assign_helper && helper == NULL) {
-		helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
-		if (unlikely(!net->ct.auto_assign_helper_warned && helper)) {
+	if (!helper)
+	{
+		if (__nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple) &&
+				!net->ct.sysctl_auto_assign_helper &&
+				!net->ct.auto_assign_helper_warned) {
+			pr_info("nf_conntrack: default automatic helper assignment "
+				"has been turned off for security reasons "
+				"and CT-based firewall rule not found. Use the "
+				"iptables CT target to attach helpers instead.\n");
+			net->ct.auto_assign_helper_warned = true;
+		} else {
+			helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+			if (unlikely(!net->ct.auto_assign_helper_warned && helper &&
+					!net->ct.auto_assign_helper_warned)) {
 			pr_info("nf_conntrack: automatic helper "
 				"assignment is deprecated and it will "
 				"be removed soon. Use the iptables CT target "
 				"to attach helpers instead.\n");
 			net->ct.auto_assign_helper_warned = true;
+			}
 		}
 	}
-
 	if (helper == NULL) {
 		if (help)
 			RCU_INIT_POINTER(help->helper, NULL);

-- 
Jiri Kosina
SUSE Labs

[PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper
assignment") is causing behavior regressions in firewalls, as traffic
handled by conntrack helpers is now by default not passed through even
though it was before due to missing CT targets (which were not necessary
before this commit).

The default had to be switched off due to security reasons [1] [2] and
therefore should stay the way it is, but let's be friendly to firewall
admins and issue a warning the first time we're in situation where packet
would be likely passed through with the old default but we're likely going
to drop it on the floor now.

Re-use the 'net->ct.auto_assign_helper_warned' flag, as it'd be sufficient
to warn one way or the other.

[1] https://cansecwest.com/csw12/conntrack-attack.pdf
[2] https://home.regit.org/netfilter-en/secure-use-of-helpers/

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

v1 -> v2: polished the condition; put unlikely() in place and reordered
	  so that we perform __nf_ct_helper_find() lookup only if we 
	  haven't warned before and the sysctl is unset

 net/netfilter/nf_conntrack_helper.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 7341adf..d82d5ee 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -213,17 +213,27 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 	}
 
 	help = nfct_help(ct);
-	if (net->ct.sysctl_auto_assign_helper && helper == NULL) {
-		helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
-		if (unlikely(!net->ct.auto_assign_helper_warned && helper)) {
+	if (!helper) {
+		if (unlikely(!net->ct.sysctl_auto_assign_helper &&
+				!net->ct.auto_assign_helper_warned &&
+				__nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple))) {
+			pr_info("nf_conntrack: default automatic helper assignment "
+				"has been turned off for security reasons "
+				"and CT-based firewall rule not found. Use the "
+				"iptables CT target to attach helpers instead.\n");
+			net->ct.auto_assign_helper_warned = true;
+		} else {
+			helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+			if (unlikely(!net->ct.auto_assign_helper_warned && helper &&
+					!net->ct.auto_assign_helper_warned)) {
 			pr_info("nf_conntrack: automatic helper "
 				"assignment is deprecated and it will "
 				"be removed soon. Use the iptables CT target "
 				"to attach helpers instead.\n");
 			net->ct.auto_assign_helper_warned = true;
+			}
 		}
 	}
-
 	if (helper == NULL) {
 		if (help)
 			RCU_INIT_POINTER(help->helper, NULL);
-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Linus Torvalds]

On Tue, Jan 24, 2017 at 2:17 AM, Jiri Kosina <jikos@kernel.org> wrote:
> +       if (!helper) {
> +               if (unlikely(!net->ct.sysctl_auto_assign_helper &&
> +                               !net->ct.auto_assign_helper_warned &&
> +                               __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple))) {
> +                       pr_info("nf_conntrack: default automatic helper assignment "
> +                               "has been turned off for security reasons "
> +                               "and CT-based firewall rule not found. Use the "
> +                               "iptables CT target to attach helpers instead.\n");
> +                       net->ct.auto_assign_helper_warned = true;
> +               } else {
> +                       helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> +                       if (unlikely(!net->ct.auto_assign_helper_warned && helper &&
> +                                       !net->ct.auto_assign_helper_warned)) {
>                         pr_info("nf_conntrack: automatic helper "
>                                 "assignment is deprecated and it will "
>                                 "be removed soon. Use the iptables CT target "
>                                 "to attach helpers instead.\n");
>                         net->ct.auto_assign_helper_warned = true;
> +                       }
>                 }
>         }

I don't disagree that this kind of warning might be useful, but that
code makes my eyes bleed, and is really really hard to follow.

Please make it a helper function. And don't have crazy conditionals
with else statements and other crazy conditionals. With random
likely/unlikely things that are not necessariyl even true.

For example, you can rewrite the logic something like

    static struct nf_conntrack_helper *find_auto_helper(struct nf_conn *ct)
    {
            return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple;
    }

    static struct nf_conntrack_helper *ct_lookup_helper(struct nf_conn
*ct, struct net *net)
    {
        struct nf_conntrack_helper *ret;

        if (!net->ct.sysctl_auto_assign_helper) {
                if (net->ct.auto_assign_helper_warned)
                        return NULL;
                if (!find_auto_helper(ct))
                        return NULL;

                .. warn about helper existing but not used ..

                net->ct.auto_assign_helper_warned = 1;
                return NULL;
        }

        ret = find_auto_helper(ct);
        if (!ret || net->ct.auto_assign_helper_warned)
                return ret;

        ... warn about helper existing but automatic helpers deprecated..

        net->ct.auto_assign_helper_warned = 1;
        return ret;
    }

and now each particular case is a lot easier to follow. Then you just have

        if (!helper) {
                helper = ct_lookup_helper(ct, net);
                if (!helper) {
                        if (help)
                                RCU_INIT_POINTER(help->helper, NULL);
                        return 0;
                }
         }

in __nf_ct_try_assign_helper()

All of the above is entirely untested and just written in my email
client. It may be garbage. It's not meant to be used, it's meant to
just illustrate avoiding complex nested conditionals. It's a few more
lines, but now each part has simple logic and is much more
understandable.

                       Linus

Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Jiri Kosina]

On Wed, 25 Jan 2017, Linus Torvalds wrote:

> I don't disagree that this kind of warning might be useful, but that
> code makes my eyes bleed, and is really really hard to follow.

Yeah, I agree. Mea culpa for not keeping 'RFC' in subject for v2 still; I 
was mostly worried about the fact that neither Pablo nor you seemed to be 
concerned too much about the whole breakage, and hence I wanted to propose 
a compromise at least.

> Please make it a helper function.

So I ended up with the patch below. It's boot-and-sysctl-flip tested. As 
you've already come up with the identifiers for the lookup functions, I've 
retained those.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] netfilter: nf_ct_helper: warn when not applying default helper assignment

Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper
assignment") is causing behavior regressions in firewalls, as traffic
handled by conntrack helpers is now by default not passed through even
though it was before due to missing CT targets (which were not necessary
before this commit).

The default had to be switched off due to security reasons [1] [2] and
therefore should stay the way it is, but let's be friendly to firewall
admins and issue a warning the first time we're in situation where packet
would be likely passed through with the old default but we're likely going
to drop it on the floor now.

Rewrite the code a little bit as suggested by Linus, so that we avoid
spaghettiing the code even more -- namely the whole decision making
process regarding helper selection (either automatic or not) is being
separated, so that the whole logic can be simplified and code (condition)
duplication reduced.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 net/netfilter/nf_conntrack_helper.c | 58 +++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 16 deletions(-)

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 7341adf..c93a331 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -188,6 +188,39 @@ struct nf_conn_help *
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
 
+static struct nf_conntrack_helper *find_auto_helper(struct nf_conn *ct)
+{
+	return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+}
+
+static struct nf_conntrack_helper *ct_lookup_helper(struct nf_conn *ct, struct net *net)
+{
+	struct nf_conntrack_helper *ret;
+
+	if (!net->ct.sysctl_auto_assign_helper) {
+		if (net->ct.auto_assign_helper_warned)
+			return NULL;
+		if (!find_auto_helper(ct))
+			return NULL;
+		pr_info("nf_conntrack: default automatic helper assignment "
+			"has been turned off for security reasons and CT-based "
+			" firewall rule not found. Use the iptables CT target "
+			"to attach helpers instead.\n");
+		net->ct.auto_assign_helper_warned = 1;
+		return NULL;
+	}
+
+	ret = find_auto_helper(ct);
+	if (!ret || net->ct.auto_assign_helper_warned)
+		return ret;
+	pr_info("nf_conntrack: automatic helper assignment is deprecated and it will "
+		"be removed soon. Use the iptables CT target to attach helpers "
+		" instead.\n");
+	net->ct.auto_assign_helper_warned = 1;
+	return ret;
+}
+
+
 int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 			      gfp_t flags)
 {
@@ -213,26 +246,19 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 	}
 
 	help = nfct_help(ct);
-	if (net->ct.sysctl_auto_assign_helper && helper == NULL) {
-		helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
-		if (unlikely(!net->ct.auto_assign_helper_warned && helper)) {
-			pr_info("nf_conntrack: automatic helper "
-				"assignment is deprecated and it will "
-				"be removed soon. Use the iptables CT target "
-				"to attach helpers instead.\n");
-			net->ct.auto_assign_helper_warned = true;
-		}
-	}
 
-	if (helper == NULL) {
-		if (help)
-			RCU_INIT_POINTER(help->helper, NULL);
-		return 0;
+	if (!helper) {
+		helper = ct_lookup_helper(ct, net);
+		if (!helper) {
+			if (help)
+				RCU_INIT_POINTER(help->helper, NULL);
+			return 0;
+		}
 	}
 
-	if (help == NULL) {
+	if (!help) {
 		help = nf_ct_helper_ext_add(ct, helper, flags);
-		if (help == NULL)
+		if (!help)
 			return -ENOMEM;
 	} else {
 		/* We only allow helper re-assignment of the same sort since

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Joe Perches]

On Wed, 2017-01-25 at 21:43 +0100, Jiri Kosina wrote:
> Rewrite the code a little bit as suggested by Linus, so that we avoid
> spaghettiing the code even more -- namely the whole decision making
> process regarding helper selection (either automatic or not) is being
> separated, so that the whole logic can be simplified and code (condition)
> duplication reduced.
[]
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
[]
> @@ -188,6 +188,39 @@ struct nf_conn_help *
>  }
>  EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
>  
> +static struct nf_conntrack_helper *find_auto_helper(struct nf_conn *ct)
> +{
> +	return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> +}
> +
> +static struct nf_conntrack_helper *ct_lookup_helper(struct nf_conn *ct, struct net *net)
> +{
> +	struct nf_conntrack_helper *ret;
> +
> +	if (!net->ct.sysctl_auto_assign_helper) {
> +		if (net->ct.auto_assign_helper_warned)
> +			return NULL;
> +		if (!find_auto_helper(ct))
> +			return NULL;
> +		pr_info("nf_conntrack: default automatic helper assignment "
> +			"has been turned off for security reasons and CT-based "
> +			" firewall rule not found. Use the iptables CT target "
> +			"to attach helpers instead.\n");
> +		net->ct.auto_assign_helper_warned = 1;
> +		return NULL;
> +	}
> +
> +	ret = find_auto_helper(ct);
> +	if (!ret || net->ct.auto_assign_helper_warned)
> +		return ret;
> +	pr_info("nf_conntrack: automatic helper assignment is deprecated and it will "
> +		"be removed soon. Use the iptables CT target to attach helpers "
> +		" instead.\n");
> +	net->ct.auto_assign_helper_warned = 1;
> +	return ret;
> +}

There are whitespece defects concatenating these multi-line strings.

How about an exit block that emits the message like

{
	[...]
	const char *msg;

	[...]

	if (!net->ct.sysctl_auto_assign_helper) {
		if (net->ct.auto_assign_helper_warned)
			return NULL;
		if (!find_auto_helper(ct))
			return NULL;
		msg = "default automatic helper assignment has been turned off for security reasons and CT-based firewall rule not found";
		ret = NULL;
	} else {
		ret = find_auto_helper(ct);
		if (!ret || net->ct.auto_assign_helper_warned)
			return ret;
		msg = "automatic helper assignment is deprecated and it will be removed soon";
		net->ct.auto_assign_helper_warned = 1;
	}

	pr_info("nf_conntrack: %s.  Use the iptables CT target to attach helpers instead.\n", msg);
	return ret;
}

Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Pablo Neira Ayuso]

On Wed, Jan 25, 2017 at 09:43:14PM +0100, Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] netfilter: nf_ct_helper: warn when not applying default helper assignment
> 
> Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper
> assignment") is causing behavior regressions in firewalls, as traffic
> handled by conntrack helpers is now by default not passed through even
> though it was before due to missing CT targets (which were not necessary
> before this commit).
>
> The default had to be switched off due to security reasons [1] [2] and
> therefore should stay the way it is, but let's be friendly to firewall
> admins and issue a warning the first time we're in situation where packet
> would be likely passed through with the old default but we're likely going
> to drop it on the floor now.

Links to [1] [2] are now gone in the patch description.

> Rewrite the code a little bit as suggested by Linus, so that we avoid
> spaghettiing the code even more -- namely the whole decision making
> process regarding helper selection (either automatic or not) is being
> separated, so that the whole logic can be simplified and code (condition)
> duplication reduced.
>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  net/netfilter/nf_conntrack_helper.c | 58 +++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 7341adf..c93a331 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -188,6 +188,39 @@ struct nf_conn_help *
>  }
>  EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
>  
> +static struct nf_conntrack_helper *find_auto_helper(struct nf_conn *ct)
> +{
> +	return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> +}
> +
> +static struct nf_conntrack_helper *ct_lookup_helper(struct nf_conn *ct, struct net *net)

Please use the prefixes that we already have in place:
nf_ct_lookup_helper() probably?

> +{
> +	struct nf_conntrack_helper *ret;
> +
> +	if (!net->ct.sysctl_auto_assign_helper) {
> +		if (net->ct.auto_assign_helper_warned)
> +			return NULL;
> +		if (!find_auto_helper(ct))

This fits in one line, so I think no need for find_auto_helper(), look:

                if (!__nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple))

> +			return NULL;
> +		pr_info("nf_conntrack: default automatic helper assignment "
> +			"has been turned off for security reasons and CT-based "
> +			" firewall rule not found. Use the iptables CT target "
> +			"to attach helpers instead.\n");
> +		net->ct.auto_assign_helper_warned = 1;
> +		return NULL;
> +	}
> +
> +	ret = find_auto_helper(ct);
> +	if (!ret || net->ct.auto_assign_helper_warned)
> +		return ret;
> +	pr_info("nf_conntrack: automatic helper assignment is deprecated and it will "
> +		"be removed soon. Use the iptables CT target to attach helpers "
> +		" instead.\n");

You can probably remove this older message now if you prefer the new
one you wrote, actually it's a bit redundant IMO.

> +	net->ct.auto_assign_helper_warned = 1;
> +	return ret;
> +}
> +
> +
>  int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
>  			      gfp_t flags)
>  {
> @@ -213,26 +246,19 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
>  	}
>  
>  	help = nfct_help(ct);
> -	if (net->ct.sysctl_auto_assign_helper && helper == NULL) {
> -		helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> -		if (unlikely(!net->ct.auto_assign_helper_warned && helper)) {
> -			pr_info("nf_conntrack: automatic helper "
> -				"assignment is deprecated and it will "
> -				"be removed soon. Use the iptables CT target "
> -				"to attach helpers instead.\n");
> -			net->ct.auto_assign_helper_warned = true;
> -		}
> -	}
>  
> -	if (helper == NULL) {
> -		if (help)
> -			RCU_INIT_POINTER(help->helper, NULL);
> -		return 0;
> +	if (!helper) {
> +		helper = ct_lookup_helper(ct, net);
> +		if (!helper) {
> +			if (help)
> +				RCU_INIT_POINTER(help->helper, NULL);
> +			return 0;
> +		}
>  	}
>  
> -	if (help == NULL) {
> +	if (!help) {

I don't think these cleanups belong this patch. If you aim to net
tree, then please let's keep this smaller.

I know general coding style prefer different notation, but we have
quite many spots in netfilter that are using this style already.

Thank you.

Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Jiri Kosina]

On Wed, 1 Feb 2017, Pablo Neira Ayuso wrote:

> > +{
> > +	struct nf_conntrack_helper *ret;
> > +
> > +	if (!net->ct.sysctl_auto_assign_helper) {
> > +		if (net->ct.auto_assign_helper_warned)
> > +			return NULL;
> > +		if (!find_auto_helper(ct))
> 
> This fits in one line, so I think no need for find_auto_helper(), look:

It does, but OTOH we're calling the lookup more than once, and that for me 
justifies extra function (with a descriptive name), so that we don't have 
to change multiple places in case the data structure changes in the 
future.

But I don't have a strong preference either way, so I'll change it.

> > +	if (!ret || net->ct.auto_assign_helper_warned)
> > +		return ret;
> > +	pr_info("nf_conntrack: automatic helper assignment is deprecated and it will "
> > +		"be removed soon. Use the iptables CT target to attach helpers "
> > +		" instead.\n");
> 
> You can probably remove this older message now if you prefer the new
> one you wrote, actually it's a bit redundant IMO.

Fair enough; we'll be issuing the warning in cases where actual "harm" 
would be caused, and that should be enough.

I'll trim down the CC list a little bit and submit v3 shortly.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH v3] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper 
assignment") is causing behavior regressions in firewalls, as traffic 
handled by conntrack helpers is now by default not passed through even 
though it was before due to missing CT targets (which were not necessary 
before this commit).

The default had to be switched off due to security reasons [1] [2] and 
therefore should stay the way it is, but let's be friendly to firewall 
admins and issue a warning the first time we're in situation where packet 
would be likely passed through with the old default but we're likely going 
to drop it on the floor now.

Rewrite the code a little bit as suggested by Linus, so that we avoid 
spaghettiing the code even more -- namely the whole decision making 
process regarding helper selection (either automatic or not) is being 
separated, so that the whole logic can be simplified and code (condition) 
duplication reduced.

[1] https://cansecwest.com/csw12/conntrack-attack.pdf
[2] https://home.regit.org/netfilter-en/secure-use-of-helpers/

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 net/netfilter/nf_conntrack_helper.c | 38 ++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 7341adf..3457456 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -188,6 +188,25 @@ struct nf_conn_help *
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
 
+static struct nf_conntrack_helper *nf_ct_lookup_helper(struct nf_conn *ct, struct net *net)
+{
+	if (!net->ct.sysctl_auto_assign_helper) {
+		if (net->ct.auto_assign_helper_warned)
+			return NULL;
+		if (!__nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple))
+			return NULL;
+		pr_info("nf_conntrack: default automatic helper assignment "
+			"has been turned off for security reasons and CT-based "
+			" firewall rule not found. Use the iptables CT target "
+			"to attach helpers instead.\n");
+		net->ct.auto_assign_helper_warned = 1;
+		return NULL;
+	}
+
+	return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+}
+
+
 int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 			      gfp_t flags)
 {
@@ -213,21 +232,14 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
 	}
 
 	help = nfct_help(ct);
-	if (net->ct.sysctl_auto_assign_helper && helper == NULL) {
-		helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
-		if (unlikely(!net->ct.auto_assign_helper_warned && helper)) {
-			pr_info("nf_conntrack: automatic helper "
-				"assignment is deprecated and it will "
-				"be removed soon. Use the iptables CT target "
-				"to attach helpers instead.\n");
-			net->ct.auto_assign_helper_warned = true;
-		}
-	}
 
 	if (helper == NULL) {
-		if (help)
-			RCU_INIT_POINTER(help->helper, NULL);
-		return 0;
+		helper = nf_ct_lookup_helper(ct, net);
+		if (helper == NULL) {
+			if (help)
+				RCU_INIT_POINTER(help->helper, NULL);
+			return 0;
+		}
 	}
 
 	if (help == NULL) {
-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Pablo Neira Ayuso]

On Wed, Feb 01, 2017 at 09:01:54PM +0100, Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> 
> Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper 
> assignment") is causing behavior regressions in firewalls, as traffic 
> handled by conntrack helpers is now by default not passed through even 
> though it was before due to missing CT targets (which were not necessary 
> before this commit).
> 
> The default had to be switched off due to security reasons [1] [2] and 
> therefore should stay the way it is, but let's be friendly to firewall 
> admins and issue a warning the first time we're in situation where packet 
> would be likely passed through with the old default but we're likely going 
> to drop it on the floor now.
> 
> Rewrite the code a little bit as suggested by Linus, so that we avoid 
> spaghettiing the code even more -- namely the whole decision making 
> process regarding helper selection (either automatic or not) is being 
> separated, so that the whole logic can be simplified and code (condition) 
> duplication reduced.
> 
> [1] https://cansecwest.com/csw12/conntrack-attack.pdf
> [2] https://home.regit.org/netfilter-en/secure-use-of-helpers/

Applied, thanks.