Re: thoughts on reallocarray, explicit_bzero?

[Rich Felker <dalias@libc.org>]

On Mon, May 19, 2014 at 05:44:59PM +0200, Daniel Cegiełka wrote:
> 2014-05-19 17:31 GMT+02:00 Isaac Dunham <ibid.ag@gmail.com>:
> > Having read up on the LibreSSL fork of OpenSSL and also recently
> > backported a nuber of libXfont CVE fixes for integer overflows,
> > I've seen the risk posed by malloc(n*sizeof(x)) and realloc(ptr,
> > n*sizeof(x)).
> > calloc(n, sizeof(x)) can be used in place of malloc(n * sizeof(x)),
> > but there's no standard function that does overflow checking for
> > realloc(). OpenBSD has provided the extension reallocarray(), which
> > provides for bounds checking like calloc() does.
> >
> > Additionally, there are times when a compiler will optimize away calls
> > to bzero() on areas that are not used before free(); this can result in
> > passwords getting left in memory. OpenBSD uses a wrapper function called
> > explicit_bzero() to keep this from happening, thugh it seems to be possible
> > to use some ugliness with volatile to stop it.
> >
> > Should musl provide reallocarray()?
> 
> In my opinion, yes, we should.

In the long term I'm not strongly decided one way or the other, but
LibreSSL needs to provide its own fallbacks for these to be portable
to any system but OpenBSD. I don't want to be part of their game of
imposing new nonstandard interfaces where they could just as easily
achieve the same via wrapping standard interfaces.

> btw. no bzero()/bcopy() but memset() and memcpy() etc.
> 
> Daniel
> 
> > And what's the best way to ensure that memory gets zeroed out?
> >
> > Thanks,
> > Isaac Dunham

> diff -urN musl.orig/include/string.h musl/include/string.h
> --- musl.orig/include/string.h	Fri May  9 09:49:36 2014
> +++ musl/include/string.h	Fri May  9 09:57:10 2014
> @@ -82,6 +82,7 @@
>  char *strsep(char **, const char *);
>  size_t strlcat (char *, const char *, size_t);
>  size_t strlcpy (char *, const char *, size_t);
> +void explicit_bzero(void *b, size_t len);
>  #endif
>  
>  #ifdef _GNU_SOURCE
> diff -urN musl.orig/src/string/explicit_bzero.c musl/src/string/explicit_bzero.c
> --- musl.orig/src/string/explicit_bzero.c	Thu Jan  1 00:00:00 1970
> +++ musl/src/string/explicit_bzero.c	Fri May  9 09:57:45 2014
> @@ -0,0 +1,8 @@
> +#include <string.h>
> +
> +static void *(*volatile explicit_memset)(void *, int, size_t) = memset;
> +
> +void explicit_bzero(void *b, size_t len)
> +{
> +	(*explicit_memset)(b, 0, len);
> +}

This is a nice trick, but IIRC I actually observed GCC optimizing out
similar code before (instead of your static volatile, I used a
volatile compound literal). At least the concept is right though: you
want to prevent the compiler from being able to do any flow analysis
at compile time, and making the function pointer volatile achieves
this rather well. On the other hand, GCC will put the volatile pointer
(if it even emits it) in non-constant memory, meaning it's an
additional attack vector for function-pointer-overwrite attacks. And
as mentioned in the other email, I don't really like the idea of
making a new variant of a deprecated/removed function (bzero). The
name "explicit" is rather unclear what it means too.

> diff -urN musl.orig/include/stdlib.h musl/include/stdlib.h
> --- musl.orig/include/stdlib.h	Thu May  8 09:04:08 2014
> +++ musl/include/stdlib.h	Thu May  8 09:11:06 2014
> @@ -44,6 +44,9 @@
>  void *realloc (void *, size_t);
>  void free (void *);
>  void *aligned_alloc(size_t alignment, size_t size);
> +#ifdef _BSD_SOURCE
> +void *reallocarray(void *, size_t, size_t);
> +#endif
>  
>  _Noreturn void abort (void);
>  int atexit (void (*) (void));
> diff -urN musl.orig/src/stdlib/reallocarray.c musl/src/stdlib/reallocarray.c
> --- musl.orig/src/stdlib/reallocarray.c	Thu Jan  1 00:00:00 1970
> +++ musl/src/stdlib/reallocarray.c	Thu May  8 09:06:30 2014
> @@ -0,0 +1,17 @@
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <errno.h>
> +
> +/* this is sqrt(SIZE_MAX+1), as s1*s2 <= SIZE_MAX
> + * if both s1 < MUL_NO_OVERFLOW and s2 < MUL_NO_OVERFLOW */
> +#define MUL_NO_OVERFLOW	(1UL << (sizeof(size_t) * 4))
> +
> +void *reallocarray(void *optr, size_t nmemb, size_t size)
> +{
> +	if ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) &&
> +	    nmemb > 0 && SSIZE_MAX / nmemb < size) {
> +		errno = ENOMEM;
> +		return NULL;
> +	}
> +	return realloc(optr, size * nmemb);
> +}

While it's a bit ugly, if your goal is efficiency, it makes a lot more
sense to special-case 32-bit systems and do a 32x32 -> 64 multiply.
This makes it so you don't need division code or any extra branches.
And for 64-bit systems, either nmemb or size being >32bit would be a
pathological corner case (and very slow already anyway), so your check
is efficient.

Also, is there a reason you're using SSIZE_MAX? SIZE_MAX should work
just as well here, but functionally it makes no difference.

Rich