Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1565377 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/2] restore original default of nf_conntrack_helper  sysctl Jiri Kosina <jikos@kernel.org> - 2017-01-24 01:10 +0100
    Re: [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl Linus Torvalds <torvalds@linux-foundation.org> - 2017-01-24 02:20 +0100
      Re: [RFC PATCH 0/2] restore original default of nf_conntrack_helper  sysctl Pablo Neira Ayuso <pablo@netfilter.org> - 2017-01-24 02:30 +0100
        [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 <jikos@kernel.org> - 2017-01-24 09:00 +0100
          [PATCH v2] netfilter: nf_ct_helper: warn when not applying default  helper assignment Jiri Kosina <jikos@kernel.org> - 2017-01-24 11:20 +0100
            Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying  default helper assignment Linus Torvalds <torvalds@linux-foundation.org> - 2017-01-25 20:20 +0100
              Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying  default helper assignment Jiri Kosina <jikos@kernel.org> - 2017-01-25 21:50 +0100
                Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying  default helper assignment Joe Perches <joe@perches.com> - 2017-01-26 06:50 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1565402

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/

[toc] | [prev] | [next] | [standalone]

#1565545 — [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)

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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;
}

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web