[patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[Jiri Pirko]

In rfc4942 and rfc2460 I cannot find anything which would implicate to
drop packets which have only padding in tlv.

Current behaviour breaks TAHI Test v6LC.1.2.6.

Problem was intruduced in:
9b905fe6843 "ipv6/exthdrs: strict Pad1 and PadN check"

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/ipv6/exthdrs.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 07a7d65..8d67900 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -162,12 +162,6 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb)
 		off += optlen;
 		len -= optlen;
 	}
-	/* This case will not be caught by above check since its padding
-	 * length is smaller than 7:
-	 * 1 byte NH + 1 byte Length + 6 bytes Padding
-	 */
-	if ((padlen == 6) && ((off - skb_network_header_len(skb)) == 8))
-		goto bad;
 
 	if (len == 0)
 		return true;
-- 
1.8.3.1

Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[Eldad Zack]

Hi Jiri,

On Fri, 6 Sep 2013, Jiri Pirko wrote:

> In rfc4942 and rfc2460 I cannot find anything which would implicate to
> drop packets which have only padding in tlv.

NAK from my side.
Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".

While it doesn't specifically discusses this corner case, you can 
understand from "There is no legitimate reason for padding beyond the 
next eight octet..." that there's also no legitimate reason for an 
option header containing only padding.
I can't imagine a sane use-case for this.

> Current behaviour breaks TAHI Test v6LC.1.2.6.

I'm not familiar with this, but IMHO the test should be reversed :)

Cheers,
Eldad

> 
> Problem was intruduced in:
> 9b905fe6843 "ipv6/exthdrs: strict Pad1 and PadN check"
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  net/ipv6/exthdrs.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index 07a7d65..8d67900 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -162,12 +162,6 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb)
>  		off += optlen;
>  		len -= optlen;
>  	}
> -	/* This case will not be caught by above check since its padding
> -	 * length is smaller than 7:
> -	 * 1 byte NH + 1 byte Length + 6 bytes Padding
> -	 */
> -	if ((padlen == 6) && ((off - skb_network_header_len(skb)) == 8))
> -		goto bad;
>  
>  	if (len == 0)
>  		return true;
> -- 
> 1.8.3.1
> 

Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[Jiri Pirko]

Sat, Sep 07, 2013 at 02:31:36PM CEST, eldad@fogrefinery.com wrote:
>
>Hi Jiri,
>
>On Fri, 6 Sep 2013, Jiri Pirko wrote:
>
>> In rfc4942 and rfc2460 I cannot find anything which would implicate to
>> drop packets which have only padding in tlv.
>
>NAK from my side.
>Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".

I did.

>
>While it doesn't specifically discusses this corner case, you can 
>understand from "There is no legitimate reason for padding beyond the 
>next eight octet..." that there's also no legitimate reason for an 
>option header containing only padding.

Okay. I'm glad you agree with me and that we both understand the rfc the
same way. And since the rfc does not say that "here's no legitimate
reason for an option header containing only padding", this should be
possible. I say we respect rfc and do not add stronger restrictions than
it dictates. No need for them.

>I can't imagine a sane use-case for this.

rfcs are not about sanity...

>
>> Current behaviour breaks TAHI Test v6LC.1.2.6.
>
>I'm not familiar with this, but IMHO the test should be reversed :)
>
>Cheers,
>Eldad
>
>> 
>> Problem was intruduced in:
>> 9b905fe6843 "ipv6/exthdrs: strict Pad1 and PadN check"
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  net/ipv6/exthdrs.c | 6 ------
>>  1 file changed, 6 deletions(-)
>> 
>> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
>> index 07a7d65..8d67900 100644
>> --- a/net/ipv6/exthdrs.c
>> +++ b/net/ipv6/exthdrs.c
>> @@ -162,12 +162,6 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb)
>>  		off += optlen;
>>  		len -= optlen;
>>  	}
>> -	/* This case will not be caught by above check since its padding
>> -	 * length is smaller than 7:
>> -	 * 1 byte NH + 1 byte Length + 6 bytes Padding
>> -	 */
>> -	if ((padlen == 6) && ((off - skb_network_header_len(skb)) == 8))
>> -		goto bad;
>>  
>>  	if (len == 0)
>>  		return true;
>> -- 
>> 1.8.3.1
>> 

Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[Eldad Zack]

On Sat, 7 Sep 2013, Jiri Pirko wrote:

> Sat, Sep 07, 2013 at 02:31:36PM CEST, eldad@fogrefinery.com wrote:
> >
> >Hi Jiri,
> >
> >On Fri, 6 Sep 2013, Jiri Pirko wrote:
> >
> >> In rfc4942 and rfc2460 I cannot find anything which would implicate to
> >> drop packets which have only padding in tlv.
> >
> >NAK from my side.
> >Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".
> 
> I did.
> 
> >
> >While it doesn't specifically discusses this corner case, you can 
> >understand from "There is no legitimate reason for padding beyond the 
> >next eight octet..." that there's also no legitimate reason for an 
> >option header containing only padding.
> 
> Okay. I'm glad you agree with me and that we both understand the rfc the
> same way. And since the rfc does not say that "here's no legitimate
> reason for an option header containing only padding", this should be
> possible. I say we respect rfc and do not add stronger restrictions than
> it dictates. No need for them.

Strictly speaking, this RFC is informational, so it is doesn't dictate 
per se. I hope you don't suggest removing the other checks as well...

> >I can't imagine a sane use-case for this.
> 
> rfcs are not about sanity...

Great, we're in agreement again :) But I think the networking code is 
or should be.
What IPv6 stack would generate such a packet, given that the only usage 
of the padding options is to align other options?
Why should I accept a packet which is most likely an artificially 
crafted packet (RFC 3514)?

Cheers,
Eldad

Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[Jiri Pirko]

Sat, Sep 07, 2013 at 06:46:12PM CEST, eldad@fogrefinery.com wrote:
>
>
>On Sat, 7 Sep 2013, Jiri Pirko wrote:
>
>> Sat, Sep 07, 2013 at 02:31:36PM CEST, eldad@fogrefinery.com wrote:
>> >
>> >Hi Jiri,
>> >
>> >On Fri, 6 Sep 2013, Jiri Pirko wrote:
>> >
>> >> In rfc4942 and rfc2460 I cannot find anything which would implicate to
>> >> drop packets which have only padding in tlv.
>> >
>> >NAK from my side.
>> >Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".
>> 
>> I did.
>> 
>> >
>> >While it doesn't specifically discusses this corner case, you can 
>> >understand from "There is no legitimate reason for padding beyond the 
>> >next eight octet..." that there's also no legitimate reason for an 
>> >option header containing only padding.
>> 
>> Okay. I'm glad you agree with me and that we both understand the rfc the
>> same way. And since the rfc does not say that "here's no legitimate
>> reason for an option header containing only padding", this should be
>> possible. I say we respect rfc and do not add stronger restrictions than
>> it dictates. No need for them.
>
>Strictly speaking, this RFC is informational, so it is doesn't dictate 
>per se. I hope you don't suggest removing the other checks as well...

If there are some that just plainly assumes something and does
restrictions without any solid argument, yes remove them.

>
>> >I can't imagine a sane use-case for this.
>> 
>> rfcs are not about sanity...
>
>Great, we're in agreement again :) But I think the networking code is 
>or should be.
>What IPv6 stack would generate such a packet, given that the only usage 
>of the padding options is to align other options?

But why not? I do not see anything evil about that. And rfc allows it.

>Why should I accept a packet which is most likely an artificially 
>crafted packet (RFC 3514)?
>
>Cheers,
>Eldad
>

Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[Hannes Frederic Sowa]

Hello!

On Sat, Sep 07, 2013 at 11:34:45PM +0200, Jiri Pirko wrote:
> Sat, Sep 07, 2013 at 06:46:12PM CEST, eldad@fogrefinery.com wrote:
> >
> >
> >On Sat, 7 Sep 2013, Jiri Pirko wrote:
> >
> >> Sat, Sep 07, 2013 at 02:31:36PM CEST, eldad@fogrefinery.com wrote:
> >> >
> >> >Hi Jiri,
> >> >
> >> >On Fri, 6 Sep 2013, Jiri Pirko wrote:
> >> >
> >> >> In rfc4942 and rfc2460 I cannot find anything which would implicate to
> >> >> drop packets which have only padding in tlv.
> >> >
> >> >NAK from my side.
> >> >Please read RFC4942 2.1.9.5 "Misuse of Pad1 and PadN Options".
> >> 
> >> I did.
> >> 
> >> >
> >> >While it doesn't specifically discusses this corner case, you can 
> >> >understand from "There is no legitimate reason for padding beyond the 
> >> >next eight octet..." that there's also no legitimate reason for an 
> >> >option header containing only padding.

There could be a legitimate reason: if a firewall wants to discard a HBH
or DOH it could easily zero out the option space instead of rearranging
the segment. But to make this happen in the general case all limits
regarding padding options would have to be dropped, too. That said,
I don't see this as an valid argument.

> >> 
> >> Okay. I'm glad you agree with me and that we both understand the rfc the
> >> same way. And since the rfc does not say that "here's no legitimate
> >> reason for an option header containing only padding", this should be
> >> possible. I say we respect rfc and do not add stronger restrictions than
> >> it dictates. No need for them.
> >
> >Strictly speaking, this RFC is informational, so it is doesn't dictate 
> >per se. I hope you don't suggest removing the other checks as well...
> 
> If there are some that just plainly assumes something and does
> restrictions without any solid argument, yes remove them.
> 
> >
> >> >I can't imagine a sane use-case for this.
> >> 
> >> rfcs are not about sanity...
> >
> >Great, we're in agreement again :) But I think the networking code is 
> >or should be.
> >What IPv6 stack would generate such a packet, given that the only usage 
> >of the padding options is to align other options?
> 
> But why not? I do not see anything evil about that. And rfc allows it.

It is easier to generate valid ipv6 frames which are fragmented in the
header chain. This makes it harder for firewalls to match packets etc.

This was a known attack vector with e.g. router advertisments and
fragmentation. Filters implemented in switches could be circumvented
because they did not match the fragmented RA but the host correctly did
reassemble the packet.

> >Why should I accept a packet which is most likely an artificially 
> >crafted packet (RFC 3514)?

Of course, dropping this check won't do big harm. But I actually like
the strictness of the ipv6 header chain checks and dropping this just
for the IPv6 Ready Logo doesn't seem right. Perhaps one could talk to
the people of tahi.org first?

Greetings,

  Hannes

Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[David Miller]

From: Eldad Zack <eldad@fogrefinery.com>
Date: Sat, 7 Sep 2013 14:31:36 +0200 (CEST)

> On Fri, 6 Sep 2013, Jiri Pirko wrote:
> 
>> Current behaviour breaks TAHI Test v6LC.1.2.6.
> 
> I'm not familiar with this, but IMHO the test should be reversed :)

I think I agree with Eldad on this, and that TAHI should be adjusted
to have more reasonable expectations of an implementation.

Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[YOSHIFUJI Hideaki]

Hi.

David Miller wrote:
> From: Eldad Zack <eldad@fogrefinery.com>
> Date: Sat, 7 Sep 2013 14:31:36 +0200 (CEST)
> 
>> On Fri, 6 Sep 2013, Jiri Pirko wrote:
>>
>>> Current behaviour breaks TAHI Test v6LC.1.2.6.
>>
>> I'm not familiar with this, but IMHO the test should be reversed :)
> 
> I think I agree with Eldad on this, and that TAHI should be adjusted
> to have more reasonable expectations of an implementation.
> 

The tests (including packet formats) are based on the test specification:
 https://www.ipv6ready.org/?page=documents&tag=ipv6-core-protocols

If we want to change the tests, I think we need to consult with IPv6
Forum.

Regards,

--yoshfuji

Re: [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri,  6 Sep 2013 16:02:25 +0200

> In rfc4942 and rfc2460 I cannot find anything which would implicate to
> drop packets which have only padding in tlv.
> 
> Current behaviour breaks TAHI Test v6LC.1.2.6.
> 
> Problem was intruduced in:
> 9b905fe6843 "ipv6/exthdrs: strict Pad1 and PadN check"
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Ok I've changed my position, applied and queued up for -stable.

Thanks.