[PATCH] netfilter: don't track ICMPv6 negotiation message.

[PATCH] netfilter: don't track ICMPv6 negotiation message.

[Eric Leblond]

This patch removes connection tracking handling for ICMPv6 messages
related to Stateless Address Autoconfiguration, MLD, and MLDv2. They
can not be tracked because they are massively using multicast (on
pre-defined address). But they are not invalid and should not be
detected as such.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
index 6f859c1..94ace19 100644
--- a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
@@ -53,6 +53,17 @@ static const u_int8_t invmap[] = {
 	[ICMPV6_NI_REPLY - 128]		= ICMPV6_NI_QUERY +1
 };
 
+static const u_int8_t noct_valid_new[] = {
+	[ICMPV6_MGM_QUERY - 130] = 1,
+	[ICMPV6_MGM_REPORT -130] = 1,
+	[ICMPV6_MGM_REDUCTION - 130] = 1,
+	[NDISC_ROUTER_SOLICITATION - 130] = 1,
+	[NDISC_ROUTER_ADVERTISEMENT - 130] = 1,
+	[NDISC_NEIGHBOUR_SOLICITATION - 130] = 1,
+	[NDISC_NEIGHBOUR_ADVERTISEMENT - 130] = 1,
+	[ICMPV6_MLD2_REPORT - 130] = 1
+};
+
 static bool icmpv6_invert_tuple(struct nf_conntrack_tuple *tuple,
 				const struct nf_conntrack_tuple *orig)
 {
@@ -182,6 +193,7 @@ icmpv6_error(struct net *net, struct sk_buff *skb, unsigned int dataoff,
 {
 	const struct icmp6hdr *icmp6h;
 	struct icmp6hdr _ih;
+	int type;
 
 	icmp6h = skb_header_pointer(skb, dataoff, sizeof(_ih), &_ih);
 	if (icmp6h == NULL) {
@@ -199,6 +211,15 @@ icmpv6_error(struct net *net, struct sk_buff *skb, unsigned int dataoff,
 		return -NF_ACCEPT;
 	}
 
+	type = icmp6h->icmp6_type - 130;
+	if (type >= 0 && type < sizeof(noct_valid_new)
+	    && noct_valid_new[type]) {
+		skb->nfct = &nf_conntrack_untracked.ct_general;
+		skb->nfctinfo = IP_CT_NEW;
+		nf_conntrack_get(skb->nfct);
+		return NF_ACCEPT;
+	}
+
 	/* is not error message ? */
 	if (icmp6h->icmp6_type >= 128)
 		return NF_ACCEPT;
-- 
1.6.1

Re: [PATCH] netfilter: don't track ICMPv6 negotiation message.

[Patrick McHardy]

Eric Leblond wrote:
> This patch removes connection tracking handling for ICMPv6 messages
> related to Stateless Address Autoconfiguration, MLD, and MLDv2. They
> can not be tracked because they are massively using multicast (on
> pre-defined address). But they are not invalid and should not be
> detected as such.

Applied, thanks Eric.

> +	type = icmp6h->icmp6_type - 130;
> +	if (type >= 0 && type < sizeof(noct_valid_new)
> +	    && noct_valid_new[type]) {

I've made a small cosmetic change here and moved the && to the
first line. I'm slowly trying to get rid of the inconsistent
operator placement whenever we touch some code.

Re: [PATCH] netfilter: don't track ICMPv6 negotiation message.

[Christoph Paasch]

Hi,

I have two questions regarding this patch.

On Tue January 27 2009, Eric Leblond wrote:
> +	type = icmp6h->icmp6_type - 130;
> +	if (type >= 0 && type < sizeof(noct_valid_new)
> +	    && noct_valid_new[type]) {
> +		skb->nfct = &nf_conntrack_untracked.ct_general;
> +		skb->nfctinfo = IP_CT_NEW;
> +		nf_conntrack_get(skb->nfct);
> +		return NF_ACCEPT;
> +	}

Why do you set skb->nfctinfo = IP_CT_NEW?
Because in xt_state.c, at state_mt(...) : 
	if it is in front of an untracked packet (using nf_ct_is_untracked(skb)) it
	automatically sets the statebit to UNTRACKED and so the IP_CT_NEW isn't used.

Why do you return NF_ACCEPT and not -NF_ACCEPT?
By returning a positiv value, the packet will continue it's way through the 
connection tracker.


I hope that I was clear. 
Please correct me if I'm wrong...

Greetings,

--
Christoph Paasch

École Polytechnique de Louvain
Département d'ingénierie informatique

www.rollerbulls.be
--
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: don't track ICMPv6 negotiation message.

[Eric Leblond]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Christoph Paasch a écrit :
> Hi,
> 
> I have two questions regarding this patch.
> 
> On Tue January 27 2009, Eric Leblond wrote:
>> +	type = icmp6h->icmp6_type - 130;
>> +	if (type >= 0 && type < sizeof(noct_valid_new)
>> +	    && noct_valid_new[type]) {
>> +		skb->nfct = &nf_conntrack_untracked.ct_general;
>> +		skb->nfctinfo = IP_CT_NEW;
>> +		nf_conntrack_get(skb->nfct);
>> +		return NF_ACCEPT;
>> +	}
> 
> Why do you set skb->nfctinfo = IP_CT_NEW?
> Because in xt_state.c, at state_mt(...) : 
> 	if it is in front of an untracked packet (using nf_ct_is_untracked(skb)) it
> 	automatically sets the statebit to UNTRACKED and so the IP_CT_NEW isn't used.

Not much to say on that point. I wanted to be homogeneous with what is
done in xt_NOTRACK.c.

> Why do you return NF_ACCEPT and not -NF_ACCEPT?
> By returning a positiv value, the packet will continue it's way through the 
> connection tracker.

If I understand well, icmpv6_error will be called in nf_conntrack_core.c
as l4proto->error :

	if (l4proto->error != NULL) {
		ret = l4proto->error(net, skb, dataoff, &ctinfo, pf, hooknum);
		if (ret <= 0) {
			NF_CT_STAT_INC_ATOMIC(net, error);
			NF_CT_STAT_INC_ATOMIC(net, invalid);
			return -ret;
		}
	}

It will thus increment error counters if return is -NF_ACCEPT. As the
packets we deal with are not error I don't think it is correct to return
- -NF_ACCEPT.

But I agree with the fact, that returning NF_ACCEPT leads to some
useless work inside the kernel.

BR,
- --
Eric Leblond <eric@inl.fr>
INL: http://www.inl.fr/
NuFW: http://www.nufw.org/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFJjxfknxA7CdMWjzIRAn0aAKCQCN4JpW8eae4isbeSA/77Fl0vagCfdfhJ
7n/HGloTFIT9V+mAKnG4oPs=
=65y5
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: don't track ICMPv6 negotiation message.

[Christoph Paasch]

Hi,

thanks for your reply.
On Sun February 8 2009, Eric Leblond wrote:
> Hi,
>
> Christoph Paasch a écrit :
> > Why do you set skb->nfctinfo = IP_CT_NEW?
> > Because in xt_state.c, at state_mt(...) :
> > 	if it is in front of an untracked packet (using nf_ct_is_untracked(skb))
> > it automatically sets the statebit to UNTRACKED and so the IP_CT_NEW
> > isn't used.
>
> Not much to say on that point. I wanted to be homogeneous with what is
> done in xt_NOTRACK.c.
OK.

But then I'm asking me, why does the UNTRACKED state isn't documented in the 
iptables manpage, even if this one exists for the iptables state module? As 
those ICMPv6 messages will be recognized as UNTRACKED by xt_state.c, and so 
the firewall administrator needs to allow those kind of packets.

But well, this is more an issue of the iptables manpage, ...

>
> > Why do you return NF_ACCEPT and not -NF_ACCEPT?
> > By returning a positiv value, the packet will continue it's way through
> > the connection tracker.
>
> If I understand well, icmpv6_error will be called in nf_conntrack_core.c
> as l4proto->error :
>
> 	if (l4proto->error != NULL) {
> 		ret = l4proto->error(net, skb, dataoff, &ctinfo, pf, hooknum);
> 		if (ret <= 0) {
> 			NF_CT_STAT_INC_ATOMIC(net, error);
> 			NF_CT_STAT_INC_ATOMIC(net, invalid);
> 			return -ret;
> 		}
> 	}
>
> It will thus increment error counters if return is -NF_ACCEPT. As the
> packets we deal with are not error I don't think it is correct to return
> -NF_ACCEPT.
>
> But I agree with the fact, that returning NF_ACCEPT leads to some
> useless work inside the kernel.

The packet will then go up to the call to icmpv6_new (as the connection 
tracker will still look for a connection matching the tuple):

	if (type < 0 || type >= sizeof(valid_new) || !valid_new[type]) {
		/* Can't create a new ICMPv6 `conn' with this. */
		pr_debug("icmpv6: can't create new conn with type %u\n",
			 type + 128);
		nf_ct_dump_tuple_ipv6(&ct->tuplehash[0].tuple);
		return false;
	}

And then the invalid counter will get incremented in nf_conntrack_in(...):

	ct = resolve_normal_ct(net, skb, dataoff, pf, protonum,
			       l3proto, l4proto, &set_reply, &ctinfo);
	if (!ct) {
		/* Not valid part of a connection */
		NF_CT_STAT_INC_ATOMIC(net, invalid);
		return NF_ACCEPT;
	}

So, maybe the handling of the ICMPv6 negotiation messages should get moved to 
icmpv6_new, with a more descriptive pr_debug message than the one who is 
present.

--
Christoph Paasch

www.rollerbulls.be
--
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] IPv6 conntrack support for neighbour discovery

[Yasuyuki KOZAKAI]

Hi,

From: Eric Leblond <eric@inl.fr>
Date: Fri, 23 Jan 2009 11:51:30 +0100

> > >  static bool icmpv6_invert_tuple(struct nf_conntrack_tuple *tuple,
> > >  				const struct nf_conntrack_tuple *orig)
> > >  {
> > >  	int type = orig->dst.u.icmp.type - 128;
> > > -	if (type < 0 || type >= sizeof(invmap) || !invmap[type])
> > > +
> > > +	if (type < 0 || type >= sizeof(invmap) || !invmap[type]) {
> > >  		return false;
> > > +	}
> > 
> > Really is this change necessary ?
> 
> I've resend the patch without this change. It was forgotten during a
> merge of previous work. Sorry for that.

Ah, sorry & thank you for noticing.


> > > @@ -198,6 +212,17 @@ icmpv6_error(struct net *net, struct sk_buff *skb, unsigned int dataoff,
> > >  		return -NF_ACCEPT;
> > >  	}
> > >  
> > > +	/* autoconf message handling */
> > > +	if (nf_ct_icmpv6_autoconf) {
> > > +		int type = icmp6h->icmp6_type - 130;
> > > +		if (type >= 0 && type < sizeof(noct_valid_new)
> > > +		    && noct_valid_new[type]) {
> > > +			skb->nfct = &nf_conntrack_untracked.ct_general;
> > > +			skb->nfctinfo = IP_CT_NEW;
> > > +			nf_conntrack_get(skb->nfct);
> > > +			return -NF_ACCEPT;
> > > +		}
> > > +	}
> > 
> > I prefer 'NEW' rather than 'UNTRACKED' as other protocols which
> > validation is unclear. So another solution is to let the connection
> > tracking subsystem to create a new conntrack and to make
> > nf_contrack_proto_icmpv6 assign 0 as timeout. How do you think ?
> 
> If we do that, we can have nfnetlink messages (NEW, DESTROY) send to
> userspace. Personnaly, I don't think they are necessary. But there is an
> other issue: as we can't invert the tuple, the information provided to
> userspace will be false.
>
> Once we agree on this last point, I will send a reworked patchset (with
> at least the removal of sysctl stuff).

Thank you. I understand why ICMPv6 packets are special here and
I agree to assign UNTRACKED to them. Indeed non-invertible tuple might
bring issues.

-- Yasuyuki Kozakai

[PATCH] netfilter: don't track ICMPv6 negotiation message.

[Eric Leblond]

This patch removes connection tracking handling for ICMPv6 messages
related to autoconfiguration. They can be tracked because they are
massively using multicast (on pre-defined address). But they are not
invalid.

Signed-off-by: Eric Leblond <eric@inl.fr>
---
 net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
index 6f859c1..94ace19 100644
--- a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
@@ -53,6 +53,17 @@ static const u_int8_t invmap[] = {
 	[ICMPV6_NI_REPLY - 128]		= ICMPV6_NI_QUERY +1
 };
 
+static const u_int8_t noct_valid_new[] = {
+	[ICMPV6_MGM_QUERY - 130] = 1,
+	[ICMPV6_MGM_REPORT -130] = 1,
+	[ICMPV6_MGM_REDUCTION - 130] = 1,
+	[NDISC_ROUTER_SOLICITATION - 130] = 1,
+	[NDISC_ROUTER_ADVERTISEMENT - 130] = 1,
+	[NDISC_NEIGHBOUR_SOLICITATION - 130] = 1,
+	[NDISC_NEIGHBOUR_ADVERTISEMENT - 130] = 1,
+	[ICMPV6_MLD2_REPORT - 130] = 1
+};
+
 static bool icmpv6_invert_tuple(struct nf_conntrack_tuple *tuple,
 				const struct nf_conntrack_tuple *orig)
 {
@@ -182,6 +193,7 @@ icmpv6_error(struct net *net, struct sk_buff *skb, unsigned int dataoff,
 {
 	const struct icmp6hdr *icmp6h;
 	struct icmp6hdr _ih;
+	int type;
 
 	icmp6h = skb_header_pointer(skb, dataoff, sizeof(_ih), &_ih);
 	if (icmp6h == NULL) {
@@ -199,6 +211,15 @@ icmpv6_error(struct net *net, struct sk_buff *skb, unsigned int dataoff,
 		return -NF_ACCEPT;
 	}
 
+	type = icmp6h->icmp6_type - 130;
+	if (type >= 0 && type < sizeof(noct_valid_new)
+	    && noct_valid_new[type]) {
+		skb->nfct = &nf_conntrack_untracked.ct_general;
+		skb->nfctinfo = IP_CT_NEW;
+		nf_conntrack_get(skb->nfct);
+		return NF_ACCEPT;
+	}
+
 	/* is not error message ? */
 	if (icmp6h->icmp6_type >= 128)
 		return NF_ACCEPT;
-- 
1.5.6.3

Re: [PATCH] netfilter: don't track ICMPv6 negotiation message.

[Yasuyuki KOZAKAI]

Can you s/autoconfiguration/Stateless Address Autoconfiguration, MLD, and MLDv2/
in commit log ?

Others are fine to me.

-- Yasuyuki Kozakai

From: Eric Leblond <eric@inl.fr>
Date: Sat, 24 Jan 2009 11:32:58 +0100

> This patch removes connection tracking handling for ICMPv6 messages
> related to autoconfiguration. They can be tracked because they are
> massively using multicast (on pre-defined address). But they are not
> invalid.
> 
> Signed-off-by: Eric Leblond <eric@inl.fr>
> ---
>  net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
> index 6f859c1..94ace19 100644
> --- a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
> +++ b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
> @@ -53,6 +53,17 @@ static const u_int8_t invmap[] = {
>  	[ICMPV6_NI_REPLY - 128]		= ICMPV6_NI_QUERY +1
>  };
>  
> +static const u_int8_t noct_valid_new[] = {
> +	[ICMPV6_MGM_QUERY - 130] = 1,
> +	[ICMPV6_MGM_REPORT -130] = 1,
> +	[ICMPV6_MGM_REDUCTION - 130] = 1,
> +	[NDISC_ROUTER_SOLICITATION - 130] = 1,
> +	[NDISC_ROUTER_ADVERTISEMENT - 130] = 1,
> +	[NDISC_NEIGHBOUR_SOLICITATION - 130] = 1,
> +	[NDISC_NEIGHBOUR_ADVERTISEMENT - 130] = 1,
> +	[ICMPV6_MLD2_REPORT - 130] = 1
> +};
> +
>  static bool icmpv6_invert_tuple(struct nf_conntrack_tuple *tuple,
>  				const struct nf_conntrack_tuple *orig)
>  {
> @@ -182,6 +193,7 @@ icmpv6_error(struct net *net, struct sk_buff *skb, unsigned int dataoff,
>  {
>  	const struct icmp6hdr *icmp6h;
>  	struct icmp6hdr _ih;
> +	int type;
>  
>  	icmp6h = skb_header_pointer(skb, dataoff, sizeof(_ih), &_ih);
>  	if (icmp6h == NULL) {
> @@ -199,6 +211,15 @@ icmpv6_error(struct net *net, struct sk_buff *skb, unsigned int dataoff,
>  		return -NF_ACCEPT;
>  	}
>  
> +	type = icmp6h->icmp6_type - 130;
> +	if (type >= 0 && type < sizeof(noct_valid_new)
> +	    && noct_valid_new[type]) {
> +		skb->nfct = &nf_conntrack_untracked.ct_general;
> +		skb->nfctinfo = IP_CT_NEW;
> +		nf_conntrack_get(skb->nfct);
> +		return NF_ACCEPT;
> +	}
> +
>  	/* is not error message ? */
>  	if (icmp6h->icmp6_type >= 128)
>  		return NF_ACCEPT;
> -- 
> 1.5.6.3
>