[PATCH] memset_s() and talloc_set_secure()

Andreas Schneider asn at samba.org
Fri Oct 12 17:22:46 UTC 2018


On Friday, October 12, 2018 6:14:51 PM CEST Jeremy Allison wrote:
> On Fri, Oct 12, 2018 at 02:14:03PM +0200, Andreas Schneider wrote:
> > On Thursday, 11 October 2018 21:06:20 CEST Jeremy Allison wrote:
> > > On Fri, Oct 12, 2018 at 07:57:31AM +1300, Andrew Bartlett via samba-
> > 
> > technical wrote:
> > > > This still doesn't handle realloc correctly.  The current code there
> > > > only handles the non-pool case when ALWAYS_REALLOC is defined, which
> > > > never is.
> > > > 
> > > > You need to force all talloc_realloc() calls on secret memory though
> > > > this path, as realloc() won't let you zero the memory after, and
> > > > unlike
> > > > the talloc header (which we overstamp and then restore) you don't do
> > > > something similar with the content.
> > > > 
> > > > talloc_realloc() is rare, and talloc_set_secret() is even more rare. 
> > > > I
> > > > suggest just banning the combination and checking for things like
> > > > talloc_realloc() and talloc_asprintf_append() in the small number of
> > > > impacted code paths manually.
> > > 
> > > +1 on this design decision. This is a very narrow talloc use
> > > case and it would be better to restrict it to avoid internal
> > > complexity.
> > > 
> > > We've made this mistake before, let's not make it again.
> > > All the talloc code around talloc_set_memlimit() is a
> > > horrible increase in complexity for little (not at all
> > > in Samba) used functionality.
> > 
> > I've improved the code with metze today. talloc_realloc() is also working
> > with talloc_keep_secret() now. I've also added a test for it.
> > 
> > See attached patchset.
> > 
> > Please review, we need more eyes on this.
> 
> Andreas, can you give me the use-case for this being in talloc ?

Whatever secret (session key, passwords) we put in talloc memory, we should 
zero it before it is being freed so that if an attacker is able to read parts 
of memory is not getting secrets. 
 
> I can see the need for memset_s(), but why should this be inside
> talloc rather than being called from specific talloc destructors ?

You could do that with a destructor too, but then you have to write a 
destructor for the secrets and the desctructor needs to call a memset which is 
not optimized out. talloc_keep_secret() just does the right thing (tm) for 
you!


	Andreas


	Andreas





More information about the samba-technical mailing list