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

Groups > linux.kernel > #1571698 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying  default helper assignment Pablo Neira Ayuso <pablo@netfilter.org> - 2017-02-01 17:30 +0100
    Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying  default helper assignment Jiri Kosina <jikos@kernel.org> - 2017-02-01 20:50 +0100
      [PATCH v3] netfilter: nf_ct_helper: warn when not applying default  helper assignment Jiri Kosina <jikos@kernel.org> - 2017-02-01 21:10 +0100
        Re: [PATCH v3] netfilter: nf_ct_helper: warn when not applying  default helper assignment Pablo Neira Ayuso <pablo@netfilter.org> - 2017-02-02 14:30 +0100

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

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.

[toc] | [next] | [standalone]

#1571920

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

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

#1571946 — [PATCH v3] 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.

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

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

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web