mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] network/: strlcpy instead of strncpy
@ 2021-04-12 19:42 Guilherme Janczak
  2021-04-12 21:41 ` Guilherme Janczak
  0 siblings, 1 reply; 3+ messages in thread
From: Guilherme Janczak @ 2021-04-12 19:42 UTC (permalink / raw)
  To: musl

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;

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [musl] [PATCH] network/: strlcpy instead of strncpy
  2021-04-12 19:42 [musl] [PATCH] network/: strlcpy instead of strncpy Guilherme Janczak
@ 2021-04-12 21:41 ` Guilherme Janczak
  2021-04-12 22:25   ` Rich Felker
  0 siblings, 1 reply; 3+ messages in thread
From: Guilherme Janczak @ 2021-04-12 21:41 UTC (permalink / raw)
  To: musl

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;

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [musl] [PATCH] network/: strlcpy instead of strncpy
  2021-04-12 21:41 ` Guilherme Janczak
@ 2021-04-12 22:25   ` Rich Felker
  0 siblings, 0 replies; 3+ messages in thread
From: Rich Felker @ 2021-04-12 22:25 UTC (permalink / raw)
  To: Guilherme Janczak; +Cc: musl

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-12 22:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12 19:42 [musl] [PATCH] network/: strlcpy instead of strncpy Guilherme Janczak
2021-04-12 21:41 ` Guilherme Janczak
2021-04-12 22:25   ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).