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

Andrew Bartlett abartlet at samba.org
Tue Jan 16 18:23:59 UTC 2018


On Tue, 2018-01-16 at 09:20 -0800, Jeremy Allison wrote:
> 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:
> > > 
> > > 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.

Yes.

> I don't see how anyone can get to DEBUG() statements via
> this method. 

The printing is done via DEBUG() in Samba, and of course that backs of
to to printf(), but my point was more that we print untrusted user-
supplied strings via DEBUG() all the time.  

Gary started to filter some untrusted strings when doing the auth audit
work (the *new* messages get the usernames etc encoded if not ASCII)
but what I'm saying is that every other DEBUG(0, ...), DEBUG(1, ...) et
al that uses %s on untrusted user-supplied data is the same threat.

> 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.

Sure.  (I was going to insert a pun about speculative execution, but it
cut too deep). 

Andrew Bartlett

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




More information about the samba-technical mailing list