Syscall param ioctl(SIOCGIFINDEX) points to uninitialised byte(s) in GetNetworkInterfaceIndexByName about avahi

[lathiat]

This seems strange; I find it curious the current code as an #ifdef VALGRIND_WORKAROUND in this exact area but was done a very long time ago!

The only parameter sent to the ioctl is the interface name, which is returned (and allocated) by dbus_message_get_args

Looking at the glibc if_nametoindex code in glibc, it copies from this buffer into the request buffer using strncpy with sizeof (ifr.ifr_name) which has size IF_NAMESIZE (=16)
From glibc/sysdeps/unix/sysv/linux/if_index.c
strncpy (ifr.ifr_name, ifname, sizeof (ifr.ifr_name));

From strcpy(3):

The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated.

Testing it's definitely the size of this string being >= 16 that is triggering the valgrind error (the smaller than larger size is not required). I haven't tried to use valgrind with gdb much so was struggling to figure out exactly what access is uninitialised as I assumed that ioctl would just be sending this entire struct and not trying to read it in any string-like fashion so I'll need to look into that further to understand what's happening.

In any case we possibly should limit the interface name size being passed into the call in any case to IF_NAMESIZE

[evverx]

Given that valgrind no longer complains about that I think it is a false positive. My guess would be that ioctl(SIOCGIFINDEX) wasn't instrumented back in the day and VALGRIND_WORKAROUND was most likely supposed to get it around by avoiding calling that ioctl indirectly via if_nametoindex and if_indextoname. VALGRIND_WORKAROUND can safely be removed and I'll try to send a PR soon.

In any case we possibly should limit the interface name size being passed into the call in any case to IF_NAMESIZE

if_nametoindex started rejecting names like that back in 2017 in https://sourceware.org/git/?p=glibc.git;a=commit;f=sysdeps/unix/sysv/linux/if_index.c;h=2180fee114b778515b3f560e5ff1e795282e60b0 to fix https://sourceware.org/bugzilla/show_bug.cgi?id=22442.

All in all I think this issue can be closed.

@mikispag just out of curiosity I wonder if those strings were generated by dfuzzer: https://github.com/dbus-fuzzer/dfuzzer?

[evverx]

if_nametoindex started rejecting names like that back in 2017

Interestingly looks like that commit introduced CVE-2018-19591 and it was fully fixed in https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=9f433fc791ca4f9d678903ff45b504b524c886fb in 2018. musl doesn't seem to reject overly long names (judging from https://git.musl-libc.org/cgit/musl/tree/src/network/if_nametoindex.c) so it would probably make sense to check names after all. It has nothing to do with the valgrind false positive though.

[evverx]

I think it can be closed. The valgrind false positive is gone.