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.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FROM,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 14525 invoked from network); 12 Apr 2021 21:42:00 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 12 Apr 2021 21:42:00 -0000 Received: (qmail 30024 invoked by uid 550); 12 Apr 2021 21:41:57 -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 30003 invoked from network); 12 Apr 2021 21:41:56 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.com; s=mail; t=1618263705; bh=93UJ/ne5E6bJtdNC9iaJHOda4ergWMtQcqWI9x3AoX4=; h=In-Reply-To:Subject:To:From:References:Date:Message-ID; b=R6nt1ABzDhjxmvqB1UvpevCy7UyRCr0wI04FPjiUW0LIOvpOV2FBPN4yOBRR5lDm8 jRVZzYOVQNihDP0bVFWtHOsO0GyvumrT/weaJzZRkNkUir42zTdlIIAFr1ltIpR6YL 8DlhFfEafbZouNgKZtOLnHmmNtrx7ITBuZIIbXeE= Authentication-Results: vla1-3387bfe79154.qloud-c.yandex.net; dkim=pass header.i=@yandex.com Date: Mon, 12 Apr 2021 21:41:33 +0000 From: Guilherme Janczak To: musl@lists.openwall.com Message-ID: <20210412214133.GG37941@jan-obsd-z87.my.domain> References: <20210412194207.GF37941@jan-obsd-z87.my.domain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210412194207.GF37941@jan-obsd-z87.my.domain> Subject: Re: [musl] [PATCH] network/: strlcpy instead of strncpy 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) 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;