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

[Andrew Bartlett]

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. 

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