strscpy

[Maxime Villard <max%m00nbsd.net@localhost>]

I see the need to have the strscpy() function:

	https://www.kernel.org/doc/htmldocs/kernel-api/API-strscpy.html

Contrary to strlcpy(), it is safe FOR REAL, because it doesn't attempt a strlen
on the source. It would be good to:

 - Add strscpy() to libkern, as a C function shared across all architectures.

 - Use it to replace copystr(), and subsequently the unsafe strlcpy()s or just
   all of them directly. copystr() is a historical function that has the same
   level of safety as strscpy(), but (1) has a format that is not usual
   (pointer to store the length), (2) is very rarely used, and (3) is often
   implemented in ASM which annoys sanitizers that need wrappers as a result.

strscpy() would be the only advertised string copy function, with arguments
that match the well-known strncpy() and strlcpy() functions.

Maxime



Le 08/02/2020 à 15:54, Maxime Villard a écrit :
> [I am not subscribed to this list, so if you want to answer, make sure to CC me]
>
> Contrary to what it seems, strlcpy is not safe with an untrusted second
> argument, because it does strlen on it, and therefore expects it to be NUL-
> terminated.
>
> It looks like we have some problems in some syscalls, which call strlcpy on
> user buffers that got copyin'ed. If these buffers aren't NUL-terminated,
> strlcpy will read memory until encountering a '\0'.
>
> Example, netbsd32_ioctl.c with kASan:
>
>      #include <stdio.h>
>      #include <stdlib.h>
>      #include <unistd.h>
>      #include <sys/ioctl.h>
>      #include <fcntl.h>
>      #include <net/if.h>
>      #include <sys/sockio.h>
>      #include <string.h>
>      int main() {
>          char buf[256];
>          memset(buf, 0xAA, sizeof(buf));
>          int fd = open("test.c", O_RDONLY);
>          ioctl(fd, SIOCGIFADDRPREF, buf);
>      }
>
>      $ gcc -o test test.c -m32
>      $ ./test
>      ASan: Unauthorized Access In 0x...: Addr 0x... [1 byte, read, RedZonePartial]
>      #0 0x... in strlcpy <netbsd>
>      #1 0x... in netbsd32_ioctl <netbsd>
>      #2 0x... in netbsd32_syscall <netbsd>
>      #3 0x... in handle_syscall <netbsd>
>
> What's worse, in if_pppoe.c, it looks like we're calling strlcpy on a packet,
> with no guarantee that the string in the packet is NUL-terminated. This could
> trigger remote DoS.
>
> I think that strlcpy has a bad design and should be replaced by the safer
> copystr.
>
> In PPPoE I think we should drop the string stuff, calling printf is already a
> bad idea anyway.
>
> Maxime