From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/5130 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: thoughts on reallocarray, explicit_bzero? Date: Mon, 19 May 2014 12:16:54 -0400 Message-ID: <20140519161654.GO507@brightrain.aerifal.cx> References: <20140519153130.GA519@muslin> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: ger.gmane.org 1400516237 3930 80.91.229.3 (19 May 2014 16:17:17 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 19 May 2014 16:17:17 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-5135-gllmg-musl=m.gmane.org@lists.openwall.com Mon May 19 18:17:10 2014 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1WmQFP-0004sm-4v for gllmg-musl@plane.gmane.org; Mon, 19 May 2014 18:17:07 +0200 Original-Received: (qmail 17663 invoked by uid 550); 19 May 2014 16:17:06 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 17655 invoked from network); 19 May 2014 16:17:06 -0000 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:5130 Archived-At: On Mon, May 19, 2014 at 05:44:59PM +0200, Daniel Cegiełka wrote: > 2014-05-19 17:31 GMT+02:00 Isaac Dunham : > > 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 > + > +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 > +#include > +#include > + > +/* 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