[PATCH] Improve talloc security

[Andrew Bartlett]

On Mon, 2015-02-23 at 17:30 -0800, Jeremy Allison wrote:
> On Tue, Feb 24, 2015 at 01:54:35PM +1300, Andrew Bartlett wrote:
> > Attached are a few patches to improve the security of our systems while
> > running talloc.  I got back onto this after our recent security issue
> > where again, talloc was the exploit vector, after the initial error.
> > 
> > The first patch set finishes the work I did earlier last year to change
> > the talloc_magic to be a random value.  I found out (via libressl's
> > getentropy_linux) that we can ask the Linux kernel for a random number
> > without opening /dev/urandom.  The AT_RANDOM value appears to have been
> > provided since 2008 for ASLR, which is essentially what we are doing.
> > This is also ideal, as it is designed exactly for the environment during
> > library initialisation. 
> > 
> > (Note, the change makes me think the attempted destructor in
> > nsswich/wb_common.c isn't quite right, I don't think that actually works
> > on a non-static function, but I've not touched the behaviour).
> > 
> > The second patch tries to restrict the talloc destructor function to
> > functions already enrolled.  This is done by recording a whitelist of 0
> > and 1 bits that must be present in the functions.  
> > 
> > I use a bitmask so that it is fast to operate (rather than a sorted
> > table), yet apparently good enough to tell the difference between
> > legitimate functions and (say) system().  
> > 
> > Comments and thoughts most welcome!
> 
> Problem with this I can see is it makes talloc *more* MT-unsafe
> when used without the TALLOC_FREE_FILL environment variable,
> and without talloc_enable_null_tracking().
> 
> I know it's a small race condition on initialization but
> eventually all these things add up.

Indeed, I can see this might be an issue.  libbsd hit the same thing
here trying to play around with the global environ[] to implement
setproctitle:

http://cgit.freedesktop.org/libbsd/commit/?id=c5b959028734ca2281250c85773d9b5e1d259bc8

https://bugs.freedesktop.org/show_bug.cgi?id=66679

In short, we can already be in threads when we are in the library init,
if we are invoke for the first time by dlopen(). 

But the above is racing against other parts of the system that can see
and use environ[].  What I don't really see is how we get init() to run
twice, because dlopen() will only load a library once (otherwise we
would be in bigger trouble).  

In any case, dropping the rand() patch would avoid that part of the
issue, as it would be deterministic.  I still don't see the race, but I
understand why it raised red flags for you.

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba