* [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).