From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 19596 invoked from network); 12 Apr 2021 22:25:19 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 12 Apr 2021 22:25:19 -0000 Received: (qmail 28478 invoked by uid 550); 12 Apr 2021 22:25:16 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 28456 invoked from network); 12 Apr 2021 22:25:15 -0000 Date: Mon, 12 Apr 2021 18:25:03 -0400 From: Rich Felker To: Guilherme Janczak Cc: musl@lists.openwall.com Message-ID: <20210412222503.GM2546@brightrain.aerifal.cx> References: <20210412194207.GF37941@jan-obsd-z87.my.domain> <20210412214133.GG37941@jan-obsd-z87.my.domain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210412214133.GG37941@jan-obsd-z87.my.domain> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] network/: strlcpy instead of strncpy 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 > #include > #include > > 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