[PATCH][WIP] Make exploiting talloc harder by using a random talloc_magic

[Andrew Bartlett]

On Wed, 2013-10-16 at 22:32 -0400, Simo wrote:
> On Thu, 2013-10-17 at 14:34 +1300, Andrew Bartlett wrote:
> > This patch is inspired by the exploit in
> > http://blog.csnc.ch/wp-content/uploads/2012/07/sambaexploit_v1.0.pdf‎
> > and is an idea to see if we can make it harder to exploit talloc.  
> > 
> > The re-order is designed to put the flags earlier into the talloc_chunk,
> > where they would have to be overwritten.
> > 
> > The only downsides I see so far are:
> >  - startup needs to select a better random number
> >  - we loose the magic 'different talloc version' detection, it will just
> > abort with wrong magic.  However library .so names and symbol versions
> > will probably avoid this, now we always build with waf. 
> >  - presumably the compiler would have been able to optimise the previous
> > talloc version check
> > 
> > What do folks think, and can I get some help to prove it would disrupts
> > these exploits?
> 
> Well it may make this specific exploit harder, but if they can't use
> talloc() they'll probably find something else.

They might, but the same could be said about -fstack-protector',
FORTIFY_SOURCE etc.  We have a lot of talloc destructors around, and
while lots of other plugin points exist, none exist so predictably in
every memory structure. 

> Ok coming to the code I am not completely positive your change would
> prevent the attack in all cases, you move flags on top, but nothing
> prevents an optimizing compiler to shuffle them around the structure in
> theory.

In theory, perhaps.  Have you ever seen that happen in practice?  Given
how much the 'unsafe' trick of reading via different union arms is done
in practice (and is depended on in Samba), I suggest that this would be
the least of our worries. 

> It is unclear to me why you sprinkle talloc_set_magic() around, why
> isn't it sufficient to just call it only from __talloc_with_prefix() ?

It is, and a revised patch is attached.  I was originally trying not to
need to add the initialisation to that critical codepath.  In the long
term, running this from some other point may be better.  Is there a hook
we can declare to be run on shared library loading?

> You seem to have put it everywhere null_context is tested for non-NULL,
> but that is unnecessary, as the null_context is either NULL, or it has
> been allocated via __talloc_with_prefix() in
> talloc_enable_null_tracking() hence be already called, what am I
> missing ?

Nothing, just my first attempt as fleshing out the idea, because I hoped
expressing it in code would be clearer. 

Andrew Bartlett

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