[PATCH][WIP] Make exploiting talloc harder by using a random talloc_magic
simo at samba.org
Thu Oct 17 10:43:13 MDT 2013
On Thu, 2013-10-17 at 16:38 +1300, Andrew Bartlett wrote:
> On Wed, 2013-10-16 at 22:32 -0400, Simo wrote:
> > On Thu, 2013-10-17 at 14:34 +1300, Andrew Bartlett wrote:
> > > 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.
I am in favor additional protection do not mistake me.
> > 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.
No, I have not, and it is unlikely, but perhaps we want to use some
pragma directive to make sure ?
> > 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?
I forgot the exact semantics with libraries but you can look at
__attribute__((constructor)), some info here:
A function declared constructor should be run at library load time or
before main() execution.
However I think in this case you have to be careful as it is not clear
to me that it is ok to use rand()/srand() at this time. I wonder if
glibc does any initialization there and running in a constructor could
cause issue. It's unlikely but something to watch for.
More information about the samba-technical