Bug#1032642: iproute2: ip tunnel change ip6gre to gre crashes with stack smash

[Robin]

Package: iproute2
Version: 5.10.0-4
Severity: normal

Dear Maintainer,

I just came across a "stack smashing detected" crash when changing a gre6 to a gre4 tunnel
To reproduce create an ipv6 gre tunnel:
> ip tunnel add gre1 mode ip6gre local 2001:db8::1 remote 2001:db8::2 ttl 255
And then attempt to change it to an ipv4 one:
> ip tunnel change gre1 mode gre local 192.168.0.0 remote 192.168.0.1 ttl 255
This results in:
> *** stack smashing detected ***: terminated
> Aborted

The inverse (changing v4 to v6) results in:
> add tunnel "gre1" failed: Invalid argument

Which I'm not sure if I should expect or if that's another issue, but it does not crash.

I've reproduced the crash on a few other bullseye servers/vms to rule out a single broken install.

I have also tested this on testing (iproute2 version 6.1.0-2) and the crash also happens there

I hope this is the right place and helpful enough to action on
Thanks!


-- System Information:
Debian Release: 11.6
APT prefers stable
APT policy: (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 5.10.0-20-amd64 (SMP w/24 CPU threads)
Kernel taint flags: TAINT_PROPRIETARY_MODULE, TAINT_OOT_MODULE, TAINT_UNSIGNED_MODULE
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE not set
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages iproute2 depends on:
ii debconf [debconf-2.0] 1.5.77
ii libbpf0 1:0.3-2
ii libbsd0 0.11.3-1
ii libc6 2.31-13+deb11u5
ii libcap2 1:2.44-1
ii libcap2-bin 1:2.44-1
ii libdb5.3 5.3.28+dfsg1-0.8
ii libelf1 0.183-1
ii libmnl0 1.0.4-3
ii libselinux1 3.1-3
ii libxtables12 1.8.7-1

Versions of packages iproute2 recommends:
pn libatm1 <none>

Versions of packages iproute2 suggests:
pn iproute2-doc <none>

-- debconf information:
iproute2/setcaps: false

[Bernhard Übelacker]

Dear Maintainer,
I tried to find out where exactly the stack smashing takes place.
And found the ioctl SIOCCHGTUNNEL did write more than the 52 bytes
allocated in variable old_p, by that overwriting the stack canary.

Kind regards,
Bernhard


(gdb)
0x000055555557589f 62 {
1: x/i $pc
=> 0x55555557589f <parse_args+31>: mov %fs:0x28,%rax
(gdb)
0x00005555555758a8 62 {
1: x/i $pc
=> 0x5555555758a8 <parse_args+40>: mov %rax,0x68(%rsp)
(gdb) print/x $rax
$1 = 0xbf9b77d893accd00
(gdb) print/x $rsp + 0x68
$2 = 0x7fffffffea28


(gdb)
0x00007ffff7e575f5 120 in ../sysdeps/unix/syscall-template.S
1: x/i $pc
=> 0x7ffff7e575f5 <ioctl+5>: syscall
2: /x *(uint64_t*)0x7fffffffea28 = 0xbf9b77d893accd00
(gdb) bt
#0 0x00007ffff7e575f5 in ioctl () at ../sysdeps/unix/syscall-template.S:120
#1 0x0000555555578230 in tnl_get_ioctl (basedev=0x7fffffffee8f "gre1", p=<optimized out>) at tunnel.c:77
#2 0x0000555555576243 in parse_args (argc=9, argv=0x7fffffffec50, cmd=35315, p=0x7fffffffea70) at iptunnel.c:181
#3 0x00005555555762fb in do_add (cmd=35315, argc=<optimized out>, argv=<optimized out>) at iptunnel.c:260
#4 0x000055555556258b in do_cmd (argv0=0x7fffffffee81 "tunnel", argc=11, argv=0x7fffffffec40) at ip.c:133
#5 0x0000555555561fc2 in main (argc=12, argv=0x7fffffffec38) at ip.c:344
(gdb) stepi
0x00007ffff7e575f7 120 in ../sysdeps/unix/syscall-template.S
1: x/i $pc
=> 0x7ffff7e575f7 <ioctl+7>: cmp $0xfffffffffffff001,%rax
2: /x *(uint64_t*)0x7fffffffea28 = 0x200000000000000

(gdb) print &old_p
$7 = (struct ip_tunnel_parm *) 0x7fffffffe9f0
(gdb) print sizeof(old_p)
$8 = 52
(gdb) print/x 0x7fffffffe9f0 + 52
$9 = 0x7fffffffea24

(gdb) list iptunnel.c:181
178 if (cmd == SIOCCHGTUNNEL && count == 0) {
179 struct ip_tunnel_parm old_p = {};
180
181 if (tnl_get_ioctl(*argv, &old_p))
182 return -1;

[Luca Boccassi]

Hi David and Stephen,

To reproduce, build iproute2 with -fstack-protector-strong in CFLAGS, and run:

ip tunnel add gre1 mode ip6gre local 2001:db8::1 remote 2001:db8::2 ttl 255

ip tunnel change gre1 mode gre local 192.168.0.0 remote 192.168.0.1 ttl 255

You'll get:

*** stack smashing detected ***: terminated
Aborted

This happens because iproute2 just assumes the tunnel is ipv4, but the
kernel "knows" it's actually ip6gre so when calling the SIOCGETTUNNEL
ioctl it writes back a struct ip6_tnl_parm2 into the struct
ip_tunnel_parm which is smaller, so the stack gets overwritten. Is
there any way to tell from userspace whether a gre is v4 or v6 before
doing an ioctl? The ioctls don't take/return a size parameter as far
as I can see...

[Stephen Hemminger]

Is setting IPv4 addresses on an IPv6 tunnel even a valid operation?
Assuming the ioctl to get is fixed, is there a reason to allow it?

One more reason netlink is better than ioctl.

[Luca Boccassi]

I don't know, I don't really know anything about GRE, but if it isn't,
it should be rejected earlier, rather than overwriting the stack,
which seems dangerous.

[Stephen Hemminger]

ted
>
> This happens because iproute2 just assumes the tunnel is ipv4, but the
> kernel "knows" it's actually ip6gre so when calling the SIOCGETTUNNEL
> ioctl it writes back a struct ip6_tnl_parm2 into the struct
> ip_tunnel_parm which is smaller, so the stack gets overwritten. Is
> there any way to tell from userspace whether a gre is v4 or v6 before
> doing an ioctl? The ioctls don't take/return a size parameter as far
> as I can see...

Ip uses and IPv4 UDP socket when it thinks it is talking to GRE.
And a IPv6 UDP socket when it is talking to GRE6.

So the kernel could check and error out?

[David Ahern]

Does seem like a kernel bug and a well known design flaw in ioctl
interface (assuming buffer of a specific size). The best iproute2 can do
is have `old_p` be a larger size (e.g., ip6_tnl_parm2) to avoid the
overrun, but then the result is nonsense with no way for it no an ipv6
struct was passed back. The crash at least indicates something is off.

[Stephen Hemminger]

Actually any change tunnel can have similar issues where the tunnel
is of one type and the request wants to change parameters.
The two structs (ip_param and ip6_tunnel_param) are different enough
that getting the incorrect type will be complete garbage.

There doesn't seem to be a good way to identify the tunnel type.
The only way I can see is to look at the link type (ifi_type)
but this is ARPHRD_XXX value and not the ip protocol.

The other way would be to query link info (with netlink)
and make sure that IFLA_INFO_KIND (in IFLA_LINKINFO) matches when
changing.

Or maybe get rid of the ip tunnel command and just use ip link
which is all netlink based. The iptunnel stuff was introduced long ago
when the only way to make tunnels was with ioctl. Now you can do
same operations with ip link.

to

[Stephen Hemminger]

On Mon, 3 Apr 2023 20:47:01 -0600
David Ahern <dsa...@kernel.org> wrote:

I started to look into redoing the whole 'ip tunnel XXX' as just a remapping
of arguments and calling the equivalent 'ip link ... type YYY' and it is doable
for the basic stuff.

Then starting looking at the Potential Router List (PRL) stuff.
Looks like this is only supported through ioctl().
Definitely a dusty dark corner of networking code with rarely used features.

Plus things like, the code to get PRL will allow bigger get if called
from root vs non-root user??