minimum ICMPv6 message size vs. RPL's DIS

The kernel assumes that any valid ICMPv6 message is at least eight
bytes long, for example in net/ipv6/raw.c:icmpv6_filter

The DIS message of RPL (RFC 6550 section 6.2, from the 6LoWPAN
world), has a minimum length of six bytes, and is thus blocked by
icmpv6_filter.

Contiki OS sends this sort of messages and is thus ignored by an
user-space RPL application on Linux (in my case simpleRPL). Nodes
will eventually join the network regardless, but this can take
hours instead of seconds.
 
RFC 4443 seems to allow even a zero-sized body, making the minimum
allowable size four bytes.

A similar restriction exists in net/ipv6/icmp.c:icmpv6_rcv but
causes no harm there since all potential recipients at that point
expect a larger payload.

Now, while this is easy enough to fix on the Linux side (see
below), I wonder if it is really a Linux bug or if I (and the
authors of RPL in Contiki) may have missed some minimum size
requirement for ICMPv6 messages stated elsewhere.

Opinions ?

- Werner

diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index c45f7a5..539534c 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c

@@ -112,7 +112,7 @@ static int icmpv6_filter(const struct sock *sk, const struct sk_buff *skb)
 	const struct icmp6hdr *hdr;
 
 	hdr = skb_header_pointer(skb, skb_transport_offset(skb),
-				 sizeof(_hdr), &_hdr);
+				 4, &_hdr);
 	if (hdr) {
 		const __u32 *data = &raw6_sk(sk)->filter.data[0];
 		unsigned int type = hdr->icmp6_type;

On Wed, Jul 24, 2013 at 08:28:52PM -0300, Werner Almesberger wrote:

The kernel assumes that any valid ICMPv6 message is at least eight
bytes long, for example in net/ipv6/raw.c:icmpv6_filter

The DIS message of RPL (RFC 6550 section 6.2, from the 6LoWPAN
world), has a minimum length of six bytes, and is thus blocked by
icmpv6_filter.

Contiki OS sends this sort of messages and is thus ignored by an
user-space RPL application on Linux (in my case simpleRPL). Nodes
will eventually join the network regardless, but this can take
hours instead of seconds.
 
RFC 4443 seems to allow even a zero-sized body, making the minimum
allowable size four bytes.

A similar restriction exists in net/ipv6/icmp.c:icmpv6_rcv but
causes no harm there since all potential recipients at that point
expect a larger payload.

Now, while this is easy enough to fix on the Linux side (see
below), I wonder if it is really a Linux bug or if I (and the
authors of RPL in Contiki) may have missed some minimum size
requirement for ICMPv6 messages stated elsewhere.

Opinions ?

Browsing the RFCs, I do think you are correct. Did you check what IPv4 does?
(what a strange errata to RFC 4443...)

Hmm, maybe we should update the icmp header to something like

struct icmp6hdr {
	struct icmp6hdr_head {
		__u8 icmp6_type;
		...;
	} icmpv6_head;
#define icmp6_type icmpv6_head.icmp6_type

and just change the type in icmpv6_filter to icmp6hdr_head.

Hmm, there is a bug in this function, _hdr must not be a pointer.

Greetings,

  Hannes

Hannes Frederic Sowa wrote:

Hmm, maybe we should update the icmp header to something like

That would be quite clean. Is it okay to introduce new names
like that in a uapi/ header (uapi/linux/icmpv6.h) ?

Hmm, there is a bug in this function, _hdr must not be a pointer.

Oh, I didn't even notice that. Very good catch !

So on 32 bit system, it would actually work even with "short"
ICMPv6 messages. Two wrongs sometimes do make a right :-)

I've attached a revised patch that, according to quick testing,
still works and doesn't break anything else.

Thanks,
- Werner

---------------------------------- cut here -----------------------------------

diff --git a/include/uapi/linux/icmpv6.h b/include/uapi/linux/icmpv6.h
index e0133c7..11eb5ff 100644
--- a/include/uapi/linux/icmpv6.h
+++ b/include/uapi/linux/icmpv6.h

@@ -5,11 +5,15 @@
 #include <asm/byteorder.h>
 
 struct icmp6hdr {
-
-	__u8		icmp6_type;
-	__u8		icmp6_code;
-	__sum16		icmp6_cksum;
-
+	struct icmp6hdr_head {
+		__u8		type;
+		__u8		code;
+		__sum16		cksum;
+	} icmpv6_head;
+
+#define	icmp6_type	icmpv6_head.type
+#define	icmp6_code	icmpv6_head.code
+#define	icmp6_cksum	icmpv6_head.cksum
 
 	union {
 		__be32			un_data32[1];

diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index c45f7a5..99ab06f 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c

@@ -108,14 +108,14 @@ found:
  */
 static int icmpv6_filter(const struct sock *sk, const struct sk_buff *skb)
 {
-	struct icmp6hdr *_hdr;
-	const struct icmp6hdr *hdr;
+	struct icmp6hdr_head _head;
+	const struct icmp6hdr_head *head;
 
-	hdr = skb_header_pointer(skb, skb_transport_offset(skb),
-				 sizeof(_hdr), &_hdr);
-	if (hdr) {
+	head = skb_header_pointer(skb, skb_transport_offset(skb),
+				  sizeof(_head), &_head);
+	if (head) {
 		const __u32 *data = &raw6_sk(sk)->filter.data[0];
-		unsigned int type = hdr->icmp6_type;
+		unsigned int type = head->type;
 
 		return (data[type >> 5] & (1U << (type & 31))) != 0;
 	}

On Thu, Jul 25, 2013 at 07:30:49AM -0300, Werner Almesberger wrote:

Hannes Frederic Sowa wrote:

quoted

Hmm, maybe we should update the icmp header to something like

That would be quite clean. Is it okay to introduce new names
like that in a uapi/ header (uapi/linux/icmpv6.h) ?

quoted

Hmm, there is a bug in this function, _hdr must not be a pointer.

Oh, I didn't even notice that. Very good catch !

So on 32 bit system, it would actually work even with "short"
ICMPv6 messages. Two wrongs sometimes do make a right :-)

I've attached a revised patch that, according to quick testing,
still works and doesn't break anything else.

Hm, could you drop the 'v' (we want to stay in the naming convention; I know I
introduced it).

Looks fine, could you do a proper patch submission? (Most simple way, do
a git commit, describe your changes, git format-patch HEAD^ and check it
with checkpatch --strict). Details are in the Documentation/ directory,
especially Submit*. I will do a proper review later, then.

Actually, it would be best to split the pointer error in a seperate patch (has
to be the first one). It may be a candidate for stable.

Thanks,

  Hannes

On Thu, Jul 25, 2013 at 07:30:49AM -0300, Werner Almesberger wrote:

Hannes Frederic Sowa wrote:

quoted

Hmm, maybe we should update the icmp header to something like

That would be quite clean. Is it okay to introduce new names
like that in a uapi/ header (uapi/linux/icmpv6.h) ?

I would say no problem. But as I just realized that it could be a bit
problematic because the new defines have actually pretty common names,
let's cc David Miller. Perhaps he has an advice?

Greetings,

  Hannes

Hannes Frederic Sowa wrote:

I would say no problem. But as I just realized that it could be a bit
problematic because the new defines have actually pretty common names,
let's cc David Miller. Perhaps he has an advice?

Yeah, I'll let the issue sit here for a while so that more people
can comment. The change has numerous implications, including

- there may actually be a minimum size requirement *somewhere*
  and I just didn't find it, in which case Linux would be right,

- name pollution visible to future user space,

- subtly changes kernel API semantics for ICMPv6 receivers, to
  the detriment of those that rely on the kernel to filter
  messages < 8 bytes and misbehave if exposed to them.

So this is definitely not the kind of change I want to rush.

Even the pointer fix changes the API in a way that could break
applications that currently work (if only on 32 bit platforms,
but it's not their fault that things would go wrong on 64 bit),
so we can't apply that one before having a decision on the other
issue as well.

Thanks,
- Werner

On Thu, Jul 25, 2013 at 11:32:23AM -0300, Werner Almesberger wrote:

Hannes Frederic Sowa wrote:

quoted

I would say no problem. But as I just realized that it could be a bit
problematic because the new defines have actually pretty common names,
let's cc David Miller. Perhaps he has an advice?

Yeah, I'll let the issue sit here for a while so that more people
can comment. The change has numerous implications, including

- there may actually be a minimum size requirement *somewhere*
  and I just didn't find it, in which case Linux would be right,

I don't know how they could do this if they want to let other RFCs extend
icmp types. It definitely makes sense that an icmpv6 packet could not have any
payload (only for informational icmpv6 packets, error icmp msgs must have 32
bits of payload, see Apendix A - RFC4443).

- name pollution visible to future user space,

I did a search on codesearch.debian.org and the change seems safe from
a first glimpse.

- subtly changes kernel API semantics for ICMPv6 receivers, to
  the detriment of those that rely on the kernel to filter
  messages < 8 bytes and misbehave if exposed to them.

Yes, that could be an issue. I would be willing to accept this fallout. :)

So this is definitely not the kind of change I want to rush.

Even the pointer fix changes the API in a way that could break
applications that currently work (if only on 32 bit platforms,
but it's not their fault that things would go wrong on 64 bit),
so we can't apply that one before having a decision on the other
issue as well.

Yes, you are right!

Thanks,

  Hannes

Hannes Frederic Sowa wrote:

I don't know how they could do this if they want to let other RFCs extend
icmp types.

Oh, ICMPs can have padding. That's used to enforce "nice" alignment.
Even RFC 6550 (RPL) has that. For example, you could simply pad the
troublesome DIS, message which is

Offset	Value	Description
------	-----	------------------------------------------------
0	0x9b	ICMPv6 Type = RPL (155, section 6)
1	0x00	ICMPv6 Code = DODAG Information Solicitation (0)
2	0x??	Checksum
3	0x??	(continued)

4	0x00	Flags = 0 (section 6.2.1)
5	0x00	Reserved

to eight bytes (i.e., four bytes of body) by adding

6	0x01	Option Type = PadN (section 6.7.3)
7	0x00	Option Length = 0

But if nothing obliges the sender to do so, there's no excuse for
Linux to expect such padding.

Yes, that could be an issue. I would be willing to accept this fallout. :)

I'm kinda curious what sort of policy we have on that. The worst
case would be that there's a bunch of 64 bit Linux machines out
there, doing critical infrastructure things in the Internet (not an
unlikely role, given the API in question), and their user space has
some vulnerability if the kernel lets a "short" ICMPv6 packet
through.

Of course, "The Almesberger-Sowa Internet Meltdown of 2013" does
have a nice ring to it, in an apocalyptic kind of way ...

- Werner

On Thu, Jul 25, 2013 at 06:47:49PM -0300, Werner Almesberger wrote:

Hannes Frederic Sowa wrote:

quoted

I don't know how they could do this if they want to let other RFCs extend
icmp types.

Oh, ICMPs can have padding. That's used to enforce "nice" alignment.
Even RFC 6550 (RPL) has that. For example, you could simply pad the
troublesome DIS, message which is

Offset	Value	Description
------	-----	------------------------------------------------
0	0x9b	ICMPv6 Type = RPL (155, section 6)
1	0x00	ICMPv6 Code = DODAG Information Solicitation (0)
2	0x??	Checksum
3	0x??	(continued)

4	0x00	Flags = 0 (section 6.2.1)
5	0x00	Reserved

to eight bytes (i.e., four bytes of body) by adding

6	0x01	Option Type = PadN (section 6.7.3)
7	0x00	Option Length = 0

But if nothing obliges the sender to do so, there's no excuse for
Linux to expect such padding.

Yes, of course, that's possible but not specified at all in the general
ICMPv6 RFC.  If packets are too short for some medium, I guess, one
would stretch it with extension header paddings before the icmpv6 header.

quoted

Yes, that could be an issue. I would be willing to accept this fallout. :)

I'm kinda curious what sort of policy we have on that. The worst
case would be that there's a bunch of 64 bit Linux machines out
there, doing critical infrastructure things in the Internet (not an
unlikely role, given the API in question), and their user space has
some vulnerability if the kernel lets a "short" ICMPv6 packet
through.

You forgot one critical aspect: Important infrastructure is *never*
going to be updated and definitely never runs IPv6. ;)

I don't think there is a policy, just intuition.

Of course, "The Almesberger-Sowa Internet Meltdown of 2013" does
have a nice ring to it, in an apocalyptic kind of way ...

I would like to avoid such a scenario, but have seen enough patches that
I kind of cooled down a bit. ;)

In summary, I agree, we should get both changes at once into the tree or
none (of course I would still change the pointer to something reasonable
and describe the circumstances in a comment if we don't change the
current behaviour).

Greetings,

  Hannes

Hi Werner!

On Thu, Jul 25, 2013 at 07:30:49AM -0300, Werner Almesberger wrote:

I would go ahead and post this as an offical patch submission now and let
David Miller have a look.

Thanks,

  Hannes

From: Hannes Frederic Sowa <redacted>
Date: Thu, 1 Aug 2013 07:48:44 +0200

I would go ahead and post this as an offical patch submission now
and let David Miller have a look.

FWIW I think this is a safe change.

David Miller wrote:

FWIW I think this is a safe change.

Thanks !

Regarding the change to icmpv6.h, I found that the introduction
of the otherwise very nice and tidy "struct icmp6hdr_head" creates
a conflict with net/bridge/br_multicast.c:br_multicast_ipv6_rcv
which has a variable called "icmp6_type".

While it would be easy enough to rename that variable, this makes
me wonder if the change isn't too intrusive after all, I think
I'd rather go with my original idea and just change the size to 4,
with a comment why and what this is.

I'll post the patches in a bit.

- Werner