[PATCH] Do not leave random talloc magic in free()'ed memory, fix abort message

[Jeremy Allison]

On Mon, Jan 15, 2018 at 12:38:07PM +1300, Andrew Bartlett wrote:
> On Sat, 2018-01-13 at 10:51 -0800, Jeremy Allison via samba-technical
> wrote:
> > On Sat, Jan 13, 2018 at 07:04:06PM +1300, Andrew Bartlett via samba-technical wrote:
> > > On Fri, 2018-01-12 at 15:02 -0800, Jeremy Allison via samba-technical
> > > wrote:
> > > > On Sat, Jan 13, 2018 at 12:00:19AM +0100, David Disseldorp via samba-technical wrote:
> > > > > On Fri, 12 Jan 2018 13:48:15 +1300, Andrew Bartlett via samba-technical wrote:
> > > > > 
> > > > > > +		talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> > > > > 
> > > > > If the talloc_chunk is potentially invalid here, then I'd prefer to
> > > > > avoid logging any strings off it. Should we make this a "...%p\n", tc)
> > > > > instead, or am I being overly paranoid?
> > > > 
> > > > One can never be too paranoid with these things. Yep, I missed
> > > > that in the review. +1 for this change :-).
> > > 
> > > To be clear, this is not a change (except when it wasn't working), this
> > > has been the behaviour of talloc since inception. 
> > > 
> > > The location of the previous free() has been invaluable for debugging
> > > in the past, I would not like this to be lost so quickly.  
> > > 
> > > While it is 'invalid', it must have been a talloc object, it has
> > > exactly the right magic for a past talloc object.  We are also just
> > > about to abort(), so a sigsegv is no real worry.
> > 
> > Is there a chance an attacker can construct a string
> > that might cause an overflow/security issue on printing ?
> > 
> > That's my paranoia about this.
> 
> I would have thought most of our DEBUG() statements would be easier
> targets, but I get your point.  It could be put though a sanitiser for
> [a-z0-9:/] or so. 

So I'm understanding the threat model here, the problem
is an artificially constructed talloc context created by
another security hole allowing writes into talloc memory
we want to use. This code path is triggered as the attacker
can no longer create a valid talloc context due to the
hardening you've already done, so all the attacker can
do here is to modify tc->name. If they don't set
tc->flags & TALLOC_FLAG_FREE then we just call
talloc_abort_unknown_value() with no ability to
modify the printed string, but if they set TALLOC_FLAG_FREE
then we try and print tc->name via talloc log.

I don't see how anyone can get to DEBUG() statements via
this method. As we're going down at this point there's
no performance harm in going over the string we want to print (range
limited of course) and ensuring all characters are within
the printable range.

That way we get the benefits of the normal location of the previous free()
being printed, with protection against string-based attack.

Jeremy.