[PATCH][WIP] Make exploiting talloc harder by using a random talloc_magic
abartlet at samba.org
Wed Oct 16 21:38:37 MDT 2013
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
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.
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz
More information about the samba-technical