Re: [musl] [PATCH] network/: strlcpy instead of strncpy

[Rich Felker <dalias@libc.org>]

On Mon, Apr 12, 2021 at 09:41:33PM +0000, Guilherme Janczak wrote:
> I'm replying to myself to say that this is actually a bug and that 
> my patches fix it.
> 
> 
> If you call if_nametoindex(name) and strlen(name) is >= IF_NAMESIZE, 
> then the function doesn't null terminate and a buffer overread happens.
> if_indextoname() on the other hand seems to be copying a string which
> is always smaller than IF_NAMESIZE, but I've already been wrong once so
> don't quote me on that.
> 
> I wrote a toy program to trigger the overread:
> 
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <net/if.h>
> 
> int
> main()
> {
> 	char *name = "buffer overread, string's length is over IF_NAMESIZE";
> 	if_nametoindex(name);
> }
> 
> 
> Here's what Valgrind outputs after running the program above:
> ==5180== Memcheck, a memory error detector
> ==5180== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==5180== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
> ==5180== Command: ./a.out
> ==5180==
> ==5180== Syscall param ioctl(SIOCGIFINDEX) points to uninitialised byte(s)
> ==5180==    at 0x403E852: ioctl (ioctl.c:35)
> ==5180==    by 0x4044AB5: if_nametoindex (if_nametoindex.c:15)
> ==5180==    by 0x109223: main (in /tmp/a.out)
> ==5180==  Address 0x1fff000810 is on thread 1's stack
> ==5180==  in frame #1, created by if_nametoindex (if_nametoindex.c:9)
> ==5180==  Uninitialised value was created by a stack allocation
> ==5180==    at 0x109020: ??? (in /tmp/a.out)
> ==5180==
> ==5180==
> ==5180== HEAP SUMMARY:
> ==5180==     in use at exit: 468 bytes in 4 blocks
> ==5180==   total heap usage: 5 allocs, 1 frees, 508 bytes allocated
> ==5180==
> ==5180== LEAK SUMMARY:
> ==5180==    definitely lost: 0 bytes in 0 blocks
> ==5180==    indirectly lost: 0 bytes in 0 blocks
> ==5180==      possibly lost: 0 bytes in 0 blocks
> ==5180==    still reachable: 468 bytes in 4 blocks
> ==5180==         suppressed: 0 bytes in 0 blocks
> ==5180== Rerun with --leak-check=full to see details of leaked memory
> ==5180==
> ==5180== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
> ==5180==
> ==5180== 1 errors in context 1 of 1:
> ==5180== Syscall param ioctl(SIOCGIFINDEX) points to uninitialised byte(s)
> ==5180==    at 0x403E852: ioctl (ioctl.c:35)
> ==5180==    by 0x4044AB5: if_nametoindex (if_nametoindex.c:15)
> ==5180==    by 0x109223: main (in /tmp/a.out)
> ==5180==  Address 0x1fff000810 is on thread 1's stack
> ==5180==  in frame #1, created by if_nametoindex (if_nametoindex.c:9)
> ==5180==  Uninitialised value was created by a stack allocation
> ==5180==    at 0x109020: ??? (in /tmp/a.out)
> ==5180==
> ==5180== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Valgrind is wrong here. The kernel ioctl takes a fixed-size
null-padded buffer, not a null-terminated string.

> On 21/04/12 07:42PM, Guilherme Janczak wrote:
> > Hello, I'm sending 2 patches that do the same thing to 2 functions.
> > The patches replace strncpy with strlcpy.
> > As you can see neither function ensures null termination after strncpy,
> > but there doesn't seem to be an actual bug in their usage.
> > 
> > 
> > diff --git a/src/network/if_indextoname.c b/src/network/if_indextoname.c
> > index 3b368bf0..603204cc 100644
> > --- a/src/network/if_indextoname.c
> > +++ b/src/network/if_indextoname.c
> > @@ -19,5 +19,6 @@ char *if_indextoname(unsigned index, char *name)
> >  		if (errno == ENODEV) errno = ENXIO;
> >  		return 0;
> >  	}
> > -	return strncpy(name, ifr.ifr_name, IF_NAMESIZE);
> > +	strlcpy(name, ifr.ifr_name, IF_NAMESIZE);
> > +	return name;
> >  }
> > diff --git a/src/network/if_nametoindex.c b/src/network/if_nametoindex.c
> > index 331413c6..75af31f0 100644
> > --- a/src/network/if_nametoindex.c
> > +++ b/src/network/if_nametoindex.c
> > @@ -11,7 +11,7 @@ unsigned if_nametoindex(const char *name)
> >  	int fd, r;
> >  
> >  	if ((fd = socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0)) < 0) return 0;
> > -	strncpy(ifr.ifr_name, name, sizeof ifr.ifr_name);
> > +	strlcpy(ifr.ifr_name, name, sizeof ifr.ifr_name);
> >  	r = ioctl(fd, SIOCGIFINDEX, &ifr);
> >  	__syscall(SYS_close, fd);
> >  	return r < 0 ? 0 : ifr.ifr_ifindex;

strlcpy is not usable here because it is not in the namespace, and can
make the syscall with part of the structure uninitialized, since it
does not null pad. Historically these were null-padded fields where
all of the bytes were significant. Nowadays the kernel ignores
everything past the first null byte and rejects names that don't have
room for one. However, semantically strncpy here is very intentional,
and being used for exactly the type of historical
fixed-size/null-padded buffer it was designed for.

Rich