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

[Guilherme Janczak <guilherme.janczak@yandex.com>]

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)


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;