[PATCH nf] netfilter: conntrack: disable generic protocol tracking

[PATCH nf] netfilter: conntrack: disable generic protocol tracking

[Florian Westphal]

Given following iptables ruleset:
-P FORWARD DROP
-A FORWARD -m sctp --dport 9 -j ACCEPT
-A FORWARD -p tcp --dport 80 -j ACCEPT
-A FORWARD -p tcp -m conntrack -m state ESTABLISHED,RELATED -j ACCEPT

One would assume that this allows SCTP on port 9 and TCP on port 80.
Unfortunately, if the SCTP conntrack module is not loaded, this allows
*all* SCTP communication to pass through, i.e. -p sctp -j ACCEPT,
which we think is a security issue.

This is because on the first SCTP packet on port 9, we create a dummy
"generic l4" conntrack entry without any port information (since
conntrack doesn't know how to extract this information).

All subsequent packets that are unknown will then be in established
state since they fallback to proto_generic and the tuple lookup will
match the 'generic' entry.

Unfortunately, the only reasonable fix seems to be to completely
disable generic protocol tracking, i.e. force all packets to be in
invalid state.

Joint work with Daniel Borkmann.

Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/netfilter/nf_conntrack_proto_generic.c | 68 +++---------------------------
 1 file changed, 5 insertions(+), 63 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
index d25f293..eac3a31 100644
--- a/net/netfilter/nf_conntrack_proto_generic.c
+++ b/net/netfilter/nf_conntrack_proto_generic.c
@@ -12,7 +12,7 @@
 #include <linux/netfilter.h>
 #include <net/netfilter/nf_conntrack_l4proto.h>
 
-static unsigned int nf_ct_generic_timeout __read_mostly = 600*HZ;
+static unsigned int nf_ct_generic_timeout __read_mostly;
 
 static inline struct nf_generic_net *generic_pernet(struct net *net)
 {
@@ -23,19 +23,13 @@ static bool generic_pkt_to_tuple(const struct sk_buff *skb,
 				 unsigned int dataoff,
 				 struct nf_conntrack_tuple *tuple)
 {
-	tuple->src.u.all = 0;
-	tuple->dst.u.all = 0;
-
-	return true;
+	return false;
 }
 
 static bool generic_invert_tuple(struct nf_conntrack_tuple *tuple,
 				 const struct nf_conntrack_tuple *orig)
 {
-	tuple->src.u.all = 0;
-	tuple->dst.u.all = 0;
-
-	return true;
+	return false;
 }
 
 /* Print out the per-protocol part of the tuple. */
@@ -59,59 +53,16 @@ static int generic_packet(struct nf_conn *ct,
 			  unsigned int hooknum,
 			  unsigned int *timeout)
 {
-	nf_ct_refresh_acct(ct, ctinfo, skb, *timeout);
-	return NF_ACCEPT;
+	return NF_DROP;
 }
 
 /* Called when a new connection for this protocol found. */
 static bool generic_new(struct nf_conn *ct, const struct sk_buff *skb,
 			unsigned int dataoff, unsigned int *timeouts)
 {
-	return true;
-}
-
-#if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
-
-#include <linux/netfilter/nfnetlink.h>
-#include <linux/netfilter/nfnetlink_cttimeout.h>
-
-static int generic_timeout_nlattr_to_obj(struct nlattr *tb[],
-					 struct net *net, void *data)
-{
-	unsigned int *timeout = data;
-	struct nf_generic_net *gn = generic_pernet(net);
-
-	if (tb[CTA_TIMEOUT_GENERIC_TIMEOUT])
-		*timeout =
-		    ntohl(nla_get_be32(tb[CTA_TIMEOUT_GENERIC_TIMEOUT])) * HZ;
-	else {
-		/* Set default generic timeout. */
-		*timeout = gn->timeout;
-	}
-
-	return 0;
+	return false;
 }
 
-static int
-generic_timeout_obj_to_nlattr(struct sk_buff *skb, const void *data)
-{
-	const unsigned int *timeout = data;
-
-	if (nla_put_be32(skb, CTA_TIMEOUT_GENERIC_TIMEOUT, htonl(*timeout / HZ)))
-		goto nla_put_failure;
-
-	return 0;
-
-nla_put_failure:
-        return -ENOSPC;
-}
-
-static const struct nla_policy
-generic_timeout_nla_policy[CTA_TIMEOUT_GENERIC_MAX+1] = {
-	[CTA_TIMEOUT_GENERIC_TIMEOUT]	= { .type = NLA_U32 },
-};
-#endif /* CONFIG_NF_CT_NETLINK_TIMEOUT */
-
 #ifdef CONFIG_SYSCTL
 static struct ctl_table generic_sysctl_table[] = {
 	{
@@ -202,15 +153,6 @@ struct nf_conntrack_l4proto nf_conntrack_l4proto_generic __read_mostly =
 	.packet			= generic_packet,
 	.get_timeouts		= generic_get_timeouts,
 	.new			= generic_new,
-#if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
-	.ctnl_timeout		= {
-		.nlattr_to_obj	= generic_timeout_nlattr_to_obj,
-		.obj_to_nlattr	= generic_timeout_obj_to_nlattr,
-		.nlattr_max	= CTA_TIMEOUT_GENERIC_MAX,
-		.obj_size	= sizeof(unsigned int),
-		.nla_policy	= generic_timeout_nla_policy,
-	},
-#endif /* CONFIG_NF_CT_NETLINK_TIMEOUT */
 	.init_net		= generic_init_net,
 	.get_net_proto		= generic_get_net_proto,
 };
-- 
1.8.3.1

Re: [PATCH nf] netfilter: conntrack: disable generic protocol tracking

[Jozsef Kadlecsik]

Hi Florian,

On Thu, 25 Sep 2014, Florian Westphal wrote:

> Given following iptables ruleset:
> -P FORWARD DROP
> -A FORWARD -m sctp --dport 9 -j ACCEPT
> -A FORWARD -p tcp --dport 80 -j ACCEPT
> -A FORWARD -p tcp -m conntrack -m state ESTABLISHED,RELATED -j ACCEPT
> 
> One would assume that this allows SCTP on port 9 and TCP on port 80.
> Unfortunately, if the SCTP conntrack module is not loaded, this allows
> *all* SCTP communication to pass through, i.e. -p sctp -j ACCEPT,
> which we think is a security issue.
> 
> This is because on the first SCTP packet on port 9, we create a dummy
> "generic l4" conntrack entry without any port information (since
> conntrack doesn't know how to extract this information).
> 
> All subsequent packets that are unknown will then be in established
> state since they fallback to proto_generic and the tuple lookup will
> match the 'generic' entry.
> 
> Unfortunately, the only reasonable fix seems to be to completely
> disable generic protocol tracking, i.e. force all packets to be in
> invalid state.
> 
> Joint work with Daniel Borkmann.

That means the generic connection tracking is completely thrown out, which 
is totally backward incompatible and comes out of the blue for everyone 
who relies on it (for example runs OSPF).

I understand that this is the simplest way to handle the security issue, 
but I also believe the price is too high. Why don't we check in the sctp 
match that the conntrack module is loaded in? Something like

	if (nf_conntrack_loaded_in &&
	    !nf_conntrack_proto_sctp_loaded_in &&
	     request_module("nf_conntrack_proto_sctp") < 0)
		return false;

in match_packet() so that it won't match if conntrack there but sctp 
conntrack won't load.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH nf] netfilter: conntrack: disable generic protocol tracking

[Florian Westphal]

Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> On Thu, 25 Sep 2014, Florian Westphal wrote:
> > Given following iptables ruleset:
> > -P FORWARD DROP
> > -A FORWARD -m sctp --dport 9 -j ACCEPT
> > -A FORWARD -p tcp --dport 80 -j ACCEPT
> > -A FORWARD -p tcp -m conntrack -m state ESTABLISHED,RELATED -j ACCEPT
> > 
> > One would assume that this allows SCTP on port 9 and TCP on port 80.
> > Unfortunately, if the SCTP conntrack module is not loaded, this allows
> > *all* SCTP communication to pass through, i.e. -p sctp -j ACCEPT,
> > which we think is a security issue.
> > 
> > This is because on the first SCTP packet on port 9, we create a dummy
> > "generic l4" conntrack entry without any port information (since
> > conntrack doesn't know how to extract this information).
> > 
> > All subsequent packets that are unknown will then be in established
> > state since they fallback to proto_generic and the tuple lookup will
> > match the 'generic' entry.
> > 
> > Unfortunately, the only reasonable fix seems to be to completely
> > disable generic protocol tracking, i.e. force all packets to be in
> > invalid state.
> > 
> > Joint work with Daniel Borkmann.
> 
> That means the generic connection tracking is completely thrown out, which 
> is totally backward incompatible and comes out of the blue for everyone 
> who relies on it (for example runs OSPF).

Yes, this is unfortunate.  I don't see any other way.
I consider the generic tracker to be broken-by-design.

When a protocol tracker, e.g. tcp, finds that the packet doesn't
meet some criteria, it will be in INVALID state.

But when we don't even know the l4 protocol we happily accept it via
NEW/ESTABLISHED?

That seems just wrong to me...

> I understand that this is the simplest way to handle the security issue, 
> but I also believe the price is too high. Why don't we check in the sctp 
> match that the conntrack module is loaded in? Something like
> 
> 	if (nf_conntrack_loaded_in &&
> 	    !nf_conntrack_proto_sctp_loaded_in &&
> 	     request_module("nf_conntrack_proto_sctp") < 0)
> 		return false;
> 
> in match_packet() so that it won't match if conntrack there but sctp 
> conntrack won't load.

Well, this would keep the issue open for all other protocols where
we have no specific tracker available.

Also, I don't want to force people to use sctp connection tracking --
stateless filtering should still work (perhaps I misread what you
said above).

The only other solution that I can think of is to alter the generic
tracker to try to auto-probe every incoming l4 protocol (and remember
which l4protos we already tried).  But I don't like that a lot either.

Regards,
Florian

Re: [PATCH nf] netfilter: conntrack: disable generic protocol tracking

[Jozsef Kadlecsik]

On Thu, 25 Sep 2014, Florian Westphal wrote:

> > > Unfortunately, the only reasonable fix seems to be to completely
> > > disable generic protocol tracking, i.e. force all packets to be in
> > > invalid state.
> > 
> > That means the generic connection tracking is completely thrown out, which 
> > is totally backward incompatible and comes out of the blue for everyone 
> > who relies on it (for example runs OSPF).
> 
> Yes, this is unfortunate.  I don't see any other way.
> I consider the generic tracker to be broken-by-design.
> 
> When a protocol tracker, e.g. tcp, finds that the packet doesn't
> meet some criteria, it will be in INVALID state.
> 
> But when we don't even know the l4 protocol we happily accept it via
> NEW/ESTABLISHED?
> 
> That seems just wrong to me...

Originally all protocol trackers were included in the conntrack module. 
The whole issue comes from the fact that at present four ones can be in 
modules and might not be loaded in. If the trackers work, then the whole 
issue disappears.
 
> Also, I don't want to force people to use sctp connection tracking --
> stateless filtering should still work (perhaps I misread what you
> said above).

Stateless filtering can be chosen by the user via CT and notrack.

With disabling generic tracking you force everyone to switch to stateless 
filtering, without any other option.

> The only other solution that I can think of is to alter the generic
> tracker to try to auto-probe every incoming l4 protocol (and remember
> which l4protos we already tried).  But I don't like that a lot either.

Four protocol trackers should be probed, so I'd favour the auto-probing 
from the generic tracker.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH nf] netfilter: conntrack: disable generic protocol tracking

[Florian Westphal]

Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> On Thu, 25 Sep 2014, Florian Westphal wrote:
> 
> > > > Unfortunately, the only reasonable fix seems to be to completely
> > > > disable generic protocol tracking, i.e. force all packets to be in
> > > > invalid state.
> > > 
> > > That means the generic connection tracking is completely thrown out, which 
> > > is totally backward incompatible and comes out of the blue for everyone 
> > > who relies on it (for example runs OSPF).
> > 
> > Yes, this is unfortunate.  I don't see any other way.
> > I consider the generic tracker to be broken-by-design.
> > 
> > When a protocol tracker, e.g. tcp, finds that the packet doesn't
> > meet some criteria, it will be in INVALID state.
> > 
> > But when we don't even know the l4 protocol we happily accept it via
> > NEW/ESTABLISHED?
> > 
> > That seems just wrong to me...
> 
> Originally all protocol trackers were included in the conntrack module. 
> The whole issue comes from the fact that at present four ones can be in 
> modules and might not be loaded in. If the trackers work, then the whole 
> issue disappears.

I don't think so.  You'd actually have to implement trackers for all l4
protocols, and you'd need kernel where these are then also built.

> > Also, I don't want to force people to use sctp connection tracking --
> > stateless filtering should still work (perhaps I misread what you
> > said above).
> 
> Stateless filtering can be chosen by the user via CT and notrack.
> 
> With disabling generic tracking you force everyone to switch to stateless 
> filtering, without any other option.

How is generic tracking different from stateless filtering?
Its identical to

-p $proto -j ACCEPT

since the generic tracking entry doesn't contain any l4 lookup keys?
So I don't see any benefit from the generic tracker (except the
backwards compat issue, I agree, it is a problem)

> > The only other solution that I can think of is to alter the generic
> > tracker to try to auto-probe every incoming l4 protocol (and remember
> > which l4protos we already tried).  But I don't like that a lot either.
> 
> Four protocol trackers should be probed, so I'd favour the auto-probing 
> from the generic tracker.

Sure, if you absolutely disagree with me I will go for that and add
autoload to the generic  tracker.

But I don't like it, I feel its like band-aid solution.  And it has
severe side-effects:

No sctp conntracker available (CONFIG_NF_CT_PROTO_SCTP=n) or some
temporary issue when we tried to modprobe?
Too bad, your -p scpt --dport x + ESTABLISHED rule matches all flows.

Exploitable hole in the sctp conntracker?  Great, remote user can cause
it to be loaded automatically now.

Some other protocol where we don't have conntrack support?
Same, you get magic ESTABLISHED state everywhere...

I see no choice other than removing generic tracking.

Cheers,
Florian

Re: [PATCH nf] netfilter: conntrack: disable generic protocol tracking

[Jozsef Kadlecsik]

On Thu, 25 Sep 2014, Florian Westphal wrote:

> > Originally all protocol trackers were included in the conntrack module. 
> > The whole issue comes from the fact that at present four ones can be in 
> > modules and might not be loaded in. If the trackers work, then the whole 
> > issue disappears.
> 
> I don't think so.  You'd actually have to implement trackers for all l4
> protocols, and you'd need kernel where these are then also built.

You mean "deep" or "real" connection trackers instead of the generic one.

> > With disabling generic tracking you force everyone to switch to stateless 
> > filtering, without any other option.
> 
> How is generic tracking different from stateless filtering?
> Its identical to
> 
> -p $proto -j ACCEPT
> 
> since the generic tracking entry doesn't contain any l4 lookup keys?
> So I don't see any benefit from the generic tracker (except the
> backwards compat issue, I agree, it is a problem)

Without conntrack, you don't have NAT.

Without conntrack, you don't have a direction. So how do you controll who 
may initiate the connection for the protocols which are covered by the 
generic tracker? We don't have any match for that.
 
> No sctp conntracker available (CONFIG_NF_CT_PROTO_SCTP=n) or some
> temporary issue when we tried to modprobe?
> Too bad, your -p scpt --dport x + ESTABLISHED rule matches all flows.

The generic tracker can know which protocols have an own tracker and when 
it's not available can refuse to track that flow (like the proposed patch 
but selectively, just in this very case).

> Exploitable hole in the sctp conntracker?  Great, remote user can cause
> it to be loaded automatically now.

Exploitable modules can be blacklisted temporarily, while the fix is not 
available. With a generic tracker which refuses to track protocols with 
own trackers, the current security issue is not opened up.

> Some other protocol where we don't have conntrack support?
> Same, you get magic ESTABLISHED state everywhere...

NEW and ESTABLISHED states, which means directions and thus policies.
Those are all lost without the generic tracker.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH nf] netfilter: conntrack: disable generic protocol tracking

[Florian Westphal]

Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote:
> Without conntrack, you don't have NAT.

Right.

> Without conntrack, you don't have a direction. So how do you controll who 
> may initiate the connection for the protocols which are covered by the 
> generic tracker? We don't have any match for that.

Fair enough,  I'll add autoprobing for the generic tracker instead.

> The generic tracker can know which protocols have an own tracker and when 
> it's not available can refuse to track that flow (like the proposed patch 
> but selectively, just in this very case).

OK.  I think this is acceptable compromise.

> Exploitable modules can be blacklisted temporarily, while the fix is not 
> available. With a generic tracker which refuses to track protocols with 
> own trackers, the current security issue is not opened up.

Right, thanks Jozsef.

> NEW and ESTABLISHED states, which means directions and thus policies.
> Those are all lost without the generic tracker.

Yes, that and loss of NAT are sound arguments.

Cheers,
Florian