[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210412214133.GG37941@jan-obsd-z87.my.domain>
Date: Mon, 12 Apr 2021 21:41:33 +0000
From: Guilherme Janczak <guilherme.janczak@...dex.com>
To: musl@...ts.openwall.com
Subject: Re: [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 <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;
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
