netdev - Re: [PATCH] net: ip/tnl: Set iph->id only when don't fragment is not set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sun, 24 Nov 2019 14:02:52 +0100
From:   Oliver Herms <oliver.peter.herms@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, davem@...emloft.net,
        kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] net: ip/tnl: Set iph->id only when don't fragment is not
 set

On 23.11.19 18:53, Eric Dumazet wrote:
> Official sources for this assertion please, so that we can double check if you
> implemented the proper avoidance ?
>From RFC 6864 Section 4.1:
"The IPv4 ID field MUST NOT be used for purposes other than
      fragmentation and reassembly."

>>  net/ipv4/ip_tunnel_core.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
>> index 1452a97914a0..8636c1e0e7b7 100644
>> --- a/net/ipv4/ip_tunnel_core.c
>> +++ b/net/ipv4/ip_tunnel_core.c
>> @@ -73,7 +73,9 @@ void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
>>  	iph->daddr	=	dst;
>>  	iph->saddr	=	src;
>>  	iph->ttl	=	ttl;
>> -	__ip_select_ident(net, iph, skb_shinfo(skb)->gso_segs ?: 1);
>> +
>> +	if (unlikely((iph->frag_off & htons(IP_DF)) == false))
> 
> This unlikely() seems wrong to me.
> 
> You do not know what are the odds of IP_DF being set or not.
Right. I'll send a corrected patch.

>> +		__ip_select_ident(net, iph, skb_shinfo(skb)->gso_segs ?: 1);
> So we are going to send 2 bytes with garbage if we do not call __ip_select_ident()
> 
> This would cause various security threats, since the garbage might reveal a secret.
So we should set it to zero then.