netdev - Re: [RFC net-next] ipip: Add room for custom tunnel header

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:	Fri, 16 Aug 2013 15:11:51 +0200
From:	Kristian Evensen <kristian.evensen@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC net-next] ipip: Add room for custom tunnel header

Hi,

Thank you very much for your feedback and sorry for not replying
earlier. I modified the patch and implemented setting a custom header
length as a netlink message attribute instead
(http://patchwork.ozlabs.org/patch/266456/). It was initially rejected
on the basis that I needed to provide some use-cases, which I did, so
lets see what happens. However, your comments apply to both patches,
so it would be interesting to discuss this.

> What happens if hlen is bigger than available headroom ?
>
> hlen comes from userspace and there is no safety check, right ?

Based on my understanding of the code, hlen will not be bigger than
available headroom. dev->needed_headroom is set to the tunnel header
length in ip_tunnel_bind_dev(). hlen is only read when adding a new
link, so this assignment cannot be skipped. However, I see that a
check along the lines of "hlen > 0 && hlen < (ETH_DATA_LEN -
LL_MAX_HDR - sizeof(iphdr)" should be added.

One potential side-effect is that one can overflow the mtu by
providing a large hlen. Will that be considered a security risk or
misconfiguration? Since you have to have the same rights to create a
tunnel as to create a link, a similar behavior can be accomplished by
creating a new link with a large mtu.

> What guarantee do we have _something_ will fill the bytes ?
> (We do not want to leak prior content of those bytes to the wire)

In the current patch, none. I assumed this was ok as inserting the
custom header does not make sense without another module filling in
the content. I do agree though, a memset should be added to zero out
the custom header.

Thanks again,
Kristian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html