[PATCH] memset_s() and talloc_set_secure()

Simo simo at samba.org
Tue Mar 26 15:23:06 UTC 2019


On Fri, 2018-10-12 at 09:14 -0700, Jeremy Allison via samba-technical
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 ?
> 
> I can see the need for memset_s(), but why should this be inside
> talloc rather than being called from specific talloc destructors ?

Sorry to revive this old thread, but Andreas asked me to review his new
implementation[1] and I suggested he does it with a talloc flag
instead.
At which point he pointed me back to this thread :-)

So with my security hat on let me comment on why this needs to be in
talloc and with a flag instead of a destructor.

This functionality is hardening, and hardening is useful only if it is
complete and as error free as possible.

As I commented in [1], "The reason I see it as a problem is that there
can be only one destructor on a piece of memory, so you can end up with
a conflict to deal with (if the programmer is careful), or worse, a
silent replacement, where the memsetting destructor is simply
overwritten with another."

But this is just one reason why using a destructor is not a good idea.
There is an even bigger reason, and that is that the destructor is not
called when memory is changed around internally.
That is where a flag is necessary. Reallocation in particular can only
be dealt with by using a flag that tells the code to zero memory that
has to be returned to the system when the chunk needs to grow but does
not fit the current allocation. But also for memory shrinking, should
it happen, as otherwise the destructor may zero only partial
information, leaving part of secret material in memory.

I really do not see any way but a flag to do this job properly.

HTH,
Simo.

[1] https://gitlab.com/samba-team/samba/merge_requests/310




More information about the samba-technical mailing list