* [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl
@ 2017-01-24 0:06 Jiri Kosina
2017-01-24 1:09 ` Linus Torvalds
0 siblings, 1 reply; 12+ messages in thread
From: Jiri Kosina @ 2017-01-24 0:06 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal
Cc: netfilter-devel, coreteam, linux-kernel, info, Linus Torvalds
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl
2017-01-24 0:06 [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl Jiri Kosina
@ 2017-01-24 1:09 ` Linus Torvalds
2017-01-24 1:28 ` Pablo Neira Ayuso
0 siblings, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2017-01-24 1:09 UTC (permalink / raw)
To: Jiri Kosina
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, NetFilter,
coreteam, Linux Kernel Mailing List, info
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl
2017-01-24 1:09 ` Linus Torvalds
@ 2017-01-24 1:28 ` Pablo Neira Ayuso
2017-01-24 7:40 ` [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
0 siblings, 1 reply; 12+ messages in thread
From: Pablo Neira Ayuso @ 2017-01-24 1:28 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jiri Kosina, Jozsef Kadlecsik, Florian Westphal, NetFilter,
coreteam, Linux Kernel Mailing List, info, eric
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/
^ permalink raw reply [flat|nested] 12+ messages in thread
* [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)
2017-01-24 1:28 ` Pablo Neira Ayuso
@ 2017-01-24 7:40 ` Jiri Kosina
2017-01-24 10:17 ` [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment Jiri Kosina
0 siblings, 1 reply; 12+ messages in thread
From: Jiri Kosina @ 2017-01-24 7:40 UTC (permalink / raw)
To: Linus Torvalds, Pablo Neira Ayuso
Cc: Jozsef Kadlecsik, Florian Westphal, NetFilter, coreteam,
Linux Kernel Mailing List, info, eric
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment
2017-01-24 7:40 ` [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
@ 2017-01-24 10:17 ` Jiri Kosina
2017-01-25 19:13 ` Linus Torvalds
0 siblings, 1 reply; 12+ messages in thread
From: Jiri Kosina @ 2017-01-24 10:17 UTC (permalink / raw)
To: Linus Torvalds, Pablo Neira Ayuso
Cc: Jozsef Kadlecsik, Florian Westphal, NetFilter, coreteam,
Linux Kernel Mailing List, info, eric
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment
2017-01-24 10:17 ` [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment Jiri Kosina
@ 2017-01-25 19:13 ` Linus Torvalds
2017-01-25 20:43 ` Jiri Kosina
0 siblings, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2017-01-25 19:13 UTC (permalink / raw)
To: Jiri Kosina
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, NetFilter,
coreteam, Linux Kernel Mailing List, info, eric
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment
2017-01-25 19:13 ` Linus Torvalds
@ 2017-01-25 20:43 ` Jiri Kosina
2017-01-26 5:40 ` Joe Perches
2017-02-01 16:27 ` Pablo Neira Ayuso
0 siblings, 2 replies; 12+ messages in thread
From: Jiri Kosina @ 2017-01-25 20:43 UTC (permalink / raw)
To: Linus Torvalds
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, NetFilter,
coreteam, Linux Kernel Mailing List, info, eric
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment
2017-01-25 20:43 ` Jiri Kosina
@ 2017-01-26 5:40 ` Joe Perches
2017-02-01 16:27 ` Pablo Neira Ayuso
1 sibling, 0 replies; 12+ messages in thread
From: Joe Perches @ 2017-01-26 5:40 UTC (permalink / raw)
To: Jiri Kosina, Linus Torvalds
Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, NetFilter,
coreteam, Linux Kernel Mailing List, info, eric
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;
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment
2017-01-25 20:43 ` Jiri Kosina
2017-01-26 5:40 ` Joe Perches
@ 2017-02-01 16:27 ` Pablo Neira Ayuso
2017-02-01 19:43 ` Jiri Kosina
1 sibling, 1 reply; 12+ messages in thread
From: Pablo Neira Ayuso @ 2017-02-01 16:27 UTC (permalink / raw)
To: Jiri Kosina
Cc: Linus Torvalds, Jozsef Kadlecsik, Florian Westphal, NetFilter,
coreteam, Linux Kernel Mailing List, info, eric
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment
2017-02-01 16:27 ` Pablo Neira Ayuso
@ 2017-02-01 19:43 ` Jiri Kosina
2017-02-01 20:01 ` [PATCH v3] " Jiri Kosina
0 siblings, 1 reply; 12+ messages in thread
From: Jiri Kosina @ 2017-02-01 19:43 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Linus Torvalds, Jozsef Kadlecsik, Florian Westphal, NetFilter,
coreteam, Linux Kernel Mailing List, info, eric
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] netfilter: nf_ct_helper: warn when not applying default helper assignment
2017-02-01 19:43 ` Jiri Kosina
@ 2017-02-01 20:01 ` Jiri Kosina
2017-02-02 13:26 ` Pablo Neira Ayuso
0 siblings, 1 reply; 12+ messages in thread
From: Jiri Kosina @ 2017-02-01 20:01 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Jozsef Kadlecsik, Florian Westphal, NetFilter, coreteam,
Linux Kernel Mailing List, info, eric
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] netfilter: nf_ct_helper: warn when not applying default helper assignment
2017-02-01 20:01 ` [PATCH v3] " Jiri Kosina
@ 2017-02-02 13:26 ` Pablo Neira Ayuso
0 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2017-02-02 13:26 UTC (permalink / raw)
To: Jiri Kosina
Cc: Jozsef Kadlecsik, Florian Westphal, NetFilter, coreteam,
Linux Kernel Mailing List, info, eric
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-02-02 13:27 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-24 0:06 [RFC PATCH 0/2] restore original default of nf_conntrack_helper sysctl Jiri Kosina
2017-01-24 1:09 ` Linus Torvalds
2017-01-24 1:28 ` Pablo Neira Ayuso
2017-01-24 7:40 ` [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
2017-01-24 10:17 ` [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment Jiri Kosina
2017-01-25 19:13 ` Linus Torvalds
2017-01-25 20:43 ` Jiri Kosina
2017-01-26 5:40 ` Joe Perches
2017-02-01 16:27 ` Pablo Neira Ayuso
2017-02-01 19:43 ` Jiri Kosina
2017-02-01 20:01 ` [PATCH v3] " Jiri Kosina
2017-02-02 13:26 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).