Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 20 Jun 2015 16:32:48 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] Implement glibc *_chk interfaces for ABI
 compatibility.

On Tue, Jun 16, 2015 at 09:48:11PM -0500, Josiah Worcester wrote:
> diff --git a/src/compat/confstr_chk.c b/src/compat/confstr_chk.c
> new file mode 100644
> index 0000000..61bcd42
> --- /dev/null
> +++ b/src/compat/confstr_chk.c
> @@ -0,0 +1,8 @@
> +#include <unistd.h>
> +#include <atomic.h>
> +
> +size_t __confstr_chk(int name, char *buf, size_t len, size_t buflen)
> +{
> +	if (buflen < len) a_crash();
> +	return confstr(name, buf, len);
> +}

I think this should be something like:

	size_t r = confstr(name, buf, len<buflen ? len : buflen);
	if (buflen < len && r < buflen) a_crash();
	return r;

i.e. you should not crash unless the overflow actually would happen.

> diff --git a/src/compat/posix_chk.c b/src/compat/posix_chk.c
> new file mode 100644
> index 0000000..7e39754
> --- /dev/null
> +++ b/src/compat/posix_chk.c
> @@ -0,0 +1,132 @@
> +#include <poll.h>
> +#include <sys/socket.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <wchar.h>
> +#include "atomic.h"
> +
> +ssize_t __read_chk(int fd, void *buf, size_t count, size_t buflen)
> +{
> +	if (count > buflen) a_crash();
> +	return read(fd, buf, count);
> +}
> +
> +ssize_t __readlink_chk(const char *path, void *buf, size_t bufsize, size_t buflen)
> +{
> +	if (bufsize > buflen) a_crash();
> +	return readlink(path, buf, bufsize);
> +}
> +
> +ssize_t __readlinkat_chk(int fd, const char *path, void *buf, size_t bufsize, size_t buflen)
> +{
> +	if (bufsize > buflen) a_crash();
> +	return readlinkat(fd, path, buf, bufsize);
> +}
> +
> +ssize_t __recv_chk(int fd, void *buf, size_t len, size_t buflen, int flags)
> +{
> +	if (len > buflen) a_crash();
> +	return recv(fd, buf, len, flags);
> +}
> +
> +ssize_t __recvfrom_chk(int fd, void *buf, size_t len, size_t buflen, int flags, struct sockaddr *addr, socklen_t *alen)
> +{
> +	if (len > buflen) a_crash();
> +	return recvfrom(fd, buf, len, flags, addr, alen);
> +}
> +

These all have the same issue as confstr, and need the same fix.

> +ssize_t __pread_chk(int fd, void *buf, size_t size, size_t ofs, size_t buflen)
> +{
> +	if (size > buflen) a_crash();
> +	return pread(fd, buf, size, ofs);
> +}
> +
> +ssize_t __pread64_chk(int fd, void *buf, size_t size, size_t ofs, size_t buflen)
> +{
> +	return __pread_chk(fd, buf, size, ofs, buflen);
> +}
> +

These too.

> +char *__getcwd_chk(char *buf, size_t len, size_t buflen)
> +{
> +	if (len > buflen) a_crash();
> +	return getcwd(buf, len);
> +}

And again. Here there's also the case of !buf in which case len is ignored.

> +int __gethostname_chk(char *name, size_t len, size_t namelen)
> +{
> +	if (len > namelen) a_crash();
> +	return gethostname(name, len);
> +}

Same issue.

> +int __ttyname_r_chk(int fd, char *name, size_t size, size_t namelen)
> +{
> +	if (size > namelen) a_crash();
> +	return ttyname_r(fd, name, size);
> +}

Same.

> +char *__stpcpy_chk(char *d, const char *s, size_t dlen)
> +{
> +	size_t slen = strnlen(s, dlen) + 1;
> +	if (slen > dlen) a_crash();
> +	return stpcpy(d, s);
> +}

This looks like it handles the issue right.

> +char *__stpncpy_chk(char *d, const char *s, size_t n, size_t dlen)
> +{
> +	if (n > dlen) a_crash();
> +	return stpncpy(d, s, n);
> +}

But this does it wrong again.

> +wchar_t *__wcpcpy_chk(wchar_t *restrict d, const wchar_t *restrict s, size_t dlen)
> +{
> +	size_t slen = strnlen(s, dlen) + 1;
> +	if (slen > dlen) a_crash();
> +	return wcpcpy(d, s);
> +}

strnlen is obviously wrong for wchar_t * input. Please try compiling
with the -Werror options musl uses by default which will catch invalid
C like this. The check looks right though with that fixed.

> +wchar_t *__wcpncpy_chk(wchar_t *restrict d, const wchar_t *restrict s, size_t n, size_t dlen)
> +{
> +	if (n > dlen) a_crash();
> +	return wcpncpy(d, s, n);
> +}

Same issue again.

> +int __vsnprintf_chk(char *s, size_t n, int flag, size_t slen, const char *fmt, va_list ap)
> +{
> +	if (n > slen) a_crash();
> +	return vsnprintf(s, n, fmt, ap);
> +}

And again here.

> +int __vsprintf_chk(char *s, int flag, size_t slen, const char *fmt, va_list ap)
> +{
> +	int ret;
> +	if (slen == 0) a_crash();
> +	ret = vsnprintf(s, slen, fmt, ap);
> +	if (ret > slen) a_crash();
> +	return ret;
> +}

That should be ret>=slen, because ret does not include the null
termination. But you also need to avoid crashing when ret<0, which is
a valid error return. The implicit conversion to size_t would make all
errors crash because -1 is > any size_t except SIZE_MAX.

> diff --git a/src/compat/realpath_chk.c b/src/compat/realpath_chk.c
> new file mode 100644
> index 0000000..cc0089e
> --- /dev/null
> +++ b/src/compat/realpath_chk.c
> @@ -0,0 +1,9 @@
> +#include <stdlib.h>
> +#include <limits.h>
> +#include "atomics.h"
> +
> +char *__realpath_chk(const char *filename, char *resolved, size_t resolved_len)
> +{
> +	if (resolved_len < PATH_MAX) a_crash();
> +	return realpath(filename, resolved);
> +}

Aside from having the same issue as above, this ignores the case of
passing a null pointer, which is always valid. I think the only way to
make this function safe is to use a temp buffer of size PATH_MAX on
the stack and memcpy the result or crash if strlen is > PATH_MAX.

> diff --git a/src/compat/stdio_chk.c b/src/compat/stdio_chk.c
> new file mode 100644
> index 0000000..53e514c
> --- /dev/null
> +++ b/src/compat/stdio_chk.c
> @@ -0,0 +1,50 @@
> +#include <stdio.h>
> +#include <wchar.h>
> +#include "atomic.h"
> +#include "libc.h"
> +#include "stdio_impl.h"
> +
> +char *__fgets_chk(char *s, size_t size, int n, FILE *f)
> +{
> +	if ((size_t)n > size) a_crash();
> +	return fgets(s, n, f);
> +}
> +
> +wchar_t *__fgetws_chk(wchar_t *s, size_t size, int n, FILE *f)
> +{
> +	if ((size_t)n > size) a_crash();
> +	return fgetws(s, n, f);
> +}
> +
> +size_t __fread_chk(void *restrict destv, size_t destvlen, size_t size, size_t nmemb, FILE *restrict f)
> +{
> +	if (size != 0 && (size * nmemb) / size != nmemb) a_crash();
> +	if (size * nmemb > destvlen) a_crash();
> +	return fread(destv, size, nmemb, f);
> +}

Same issue in these.

> +char *__gets_chk(char *buf, size_t size)
> +{
> +	char *ret = buf;
> +	int c;
> +	FLOCK(stdin);
> +	if (!size) return NULL;
> +	for(;size;buf++,size--) {
> +		c = getc(stdin);
> +		if ((c == EOF && feof(stdin)) || c == '\n') {
> +			*buf = 0;
> +			FUNLOCK(stdin);
> +			return ret;
> +		}
> +		if (c == EOF) {
> +			FUNLOCK(stdin);
> +			return NULL;
> +		}
> +		*buf = c;
> +	}
> +	a_crash();
> +}

I would mildly prefer using flockfile/funlockfile (POSIX namespace) to
using stdio internals here, but it's not a big deal. gets is no longer
in the C namespace so that seems valid to me (as long as it's moved).

> diff --git a/src/compat/string_chk.c b/src/compat/string_chk.c
> new file mode 100644
> index 0000000..ba77c9c
> --- /dev/null
> +++ b/src/compat/string_chk.c
> @@ -0,0 +1,98 @@
> +#include <string.h>
> +#include <wchar.h>
> +#include "atomic.h"
> +
> +void *__memcpy_chk(void *restrict dest, const void *restrict src, size_t n, size_t destlen)
> +{
> +	if (n > destlen) a_crash();
> +	return memcpy(dest, src, n);
> +}
> +
> +void *__memmove_chk(void *restrict dest, const void *restrict src, size_t n, size_t destlen)
> +{
> +	if (n > destlen) a_crash();
> +	return memmove(dest, src, n);
> +}
> +
> +void *__memset_chk(void *dest, int c, size_t n, size_t destlen)
> +{
> +	if (n > destlen) a_crash();
> +	return memset(dest, c, n);
> +}

These are okay because they write exact-length n.

> +char *__strcat_chk(char *restrict dest, const char *restrict src, size_t destlen)
> +{
> +	size_t tot;
> +	tot = strnlen(src, destlen);
> +	if (tot > SIZE_MAX - strnlen(dest, destlen) - 1) a_crash();
> +	tot += strnlen(dest, destlen) + 1;
> +	if (tot > destlen) a_crash();
> +	return strcat(dest, src);
> +}
> +
> +char *__strncat_chk(char *restrict dest, const char *restrict src, size_t n, size_t destlen)
> +{
> +	size_t tot;
> +	tot = strnlen(dest, destlen);
> +	if (tot > SIZE_MAX - strnlen(src, n) - 1) a_crash();
> +	tot += strnlen(src, n) + 1;
> +	if (tot > destlen) a_crash();
> +	return strncat(dest, src, n);
> +}

I think these are right but I'll review them again before commit.

> +char *__strncpy_chk(char *restrict dest, const char *restrict src, size_t n, size_t destlen)
> +{
> +	if (n > destlen) a_crash();
> +	return strncpy(dest, src, n);
> +}

This is okay because strncpy is an exact-length n write.

> +wchar_t *__wcscat_chk(wchar_t *restrict dest, const wchar_t *src, size_t destlen)
> +{
> +	size_t tot;
> +	tot = wcsnlen(src, destlen);
> +	if (tot > SIZE_MAX - wcsnlen(dest, destlen) - 1) a_crash();
> +	tot += wcsnlen(dest, destlen) + 1;
> +	if (tot > destlen) a_crash();
> +	return wcscat(dest, src);
> +}
> +
> +wchar_t *__wcscpy_chk(wchar_t *restrict dest, const wchar_t *restrict src, size_t destlen)
> +{
> +	size_t srclen = wcsnlen(src, destlen) + 1;
> +	if (srclen > destlen) a_crash();
> +	return wcscpy(dest, src);
> +}
> +
> +wchar_t *__wcsncat_chk(wchar_t *restrict dest, const wchar_t *restrict src, size_t n, size_t destlen)
> +{
> +	size_t tot;
> +	tot = wcsnlen(dest, destlen);
> +	if (tot > SIZE_MAX - wcsnlen(src, n) - 1) a_crash();
> +	tot += wcsnlen(dest, destlen) + 1;
> +	if (tot > destlen) a_crash();
> +	return wcsncat(dest, src, n);
> +}
> +
> +wchar_t *__wcsncpy_chk(wchar_t *restrict dest, const wchar_t *restrict src, size_t n, size_t destlen)
> +{
> +	if (n > destlen) a_crash();
> +	return wcsncpy(dest, src, n);
> +}
> +
> +wchar_t *__wmemcpy_chk(wchar_t *restrict d, const wchar_t *restrict s, size_t n, size_t dlen)
> +{
> +	if (n > dlen) a_crash();
> +	return wmemcpy(d, s, n);
> +}
> +
> +wchar_t *__wmemmove_chk(wchar_t *d, const wchar_t *s, size_t n, size_t dlen)
> +{
> +	if (n > dlen) a_crash();
> +	return wmemmove(d, s, n);
> +}
> +
> +wchar_t *__wmemset_chk(wchar_t *d, wchar_t c, size_t n, size_t dlen)
> +{
> +	if (n > dlen) a_crash();
> +	return wmemset(d, c, n);
> +}

I didn't notice any problem in the rest of these.

> diff --git a/src/compat/ttyname_r_chk.c b/src/compat/ttyname_r_chk.c
> new file mode 100644
> index 0000000..50e70e5
> --- /dev/null
> +++ b/src/compat/ttyname_r_chk.c
> @@ -0,0 +1,7 @@
> +#include <unistd.h>
> +
> +int __ttyname_r_chk(int fd, char *name, size_t size, size_t namelen)
> +{
> +	if (size > namelen) a_crash();
> +	return ttyname_r(fd, name, size);
> +}

Same issue again.

> diff --git a/src/compat/wchar_chk.c b/src/compat/wchar_chk.c
> new file mode 100644
> index 0000000..66e35b1
> --- /dev/null
> +++ b/src/compat/wchar_chk.c
> @@ -0,0 +1,45 @@
> +#include <wchar.h>
> +#include <stdlib.h>
> +#include "atomic.h"
> +
> +size_t __mbsnrtowcs_chk(wchar_t *restrict wcs, const char **restrict src, size_t n, size_t wn, mbstate_t *restrict st, size_t wcslen)
> +{
> +	if (wn > wcslen) a_crash();
> +	return msbnrtowcs(wcs, src, n, wn, st);
> +}
> +
> +size_t __mbsrtowcs_chk(wchar_t *restrict ws, const char **restrict src, size_t wn, mbstate_t *restrict st, size_t wslen)
> +{
> +	if (wn > wslen) a_crash();
> +	return mbsrtowcs(ws, src, wn, st);
> +}
> +
> +size_t __mbstowcs_chk(wchar_t *restrict ws, const char *restrict s, size_t wn, size_t wslen)
> +{
> +	if (wn > wslen) a_crash();
> +	return mbstowcs(ws, s, wn);
> +}
> +
> +size_t __wcrtomb_chk(char *restrict s, wchar_t wc, mbstate_t *restrict st, size_t slen)
> +{
> +	if (slen < MB_CUR_MAX) a_crash();
> +	return wcrtomb(s, wc, st);
> +}
> +
> +size_t __wcsnrtombs_chk(char *restrict dst, const wchar_t **restrict wcs, size_t wn, size_t n, mbstate_t *restrict st, size_t dstlen)
> +{
> +	if (n > dstlen) a_crash();
> +	return wcsnrtombs(dst, wcs, wn, n, st);
> +}
> +
> +size_t __wcstombs_chk(char *restrict s, const wchar_t *restrict ws, size_t n, size_t slen)
> +{
> +	if (n > slen) a_crash();
> +	return wcstombs(s, ws, n);
> +}
> +
> +int __wctomb_chk(char *s, wchar_t wc, size_t slen)
> +{
> +	if (slen < MB_CUR_MAX) a_crash();
> +	return wctomb(s, wc);
> +}

And all of these.

Rich

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.