[PATCH] memset_s() and talloc_set_secure()

Andrew Bartlett abartlet at samba.org
Tue Mar 26 21:48:39 UTC 2019

On Tue, 2019-03-26 at 14:02 -0700, Jeremy Allison wrote:
> On Tue, Mar 26, 2019 at 11:23:06AM -0400, Simo wrote:
> > 
> > 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.
> Hmmm. OK, that is quite a compelling argument.
> I originally was against adding a flag to talloc as
> I'm concerned about adding complexity which ends
> up not being used (the talloc memory limits which
> you originally coded up being the best example,
> sorry :-) :-).

I'm still quite concerned about the performance cost, and I think the
realloc issue can be worked around without another talloc (mis)
feature, references.  If we are really worried about the tiny number of
cases that would realloc a 'secret' buffer, then the
talloc_set_secret() could make a redundant talloc_reference() to block

I would want to see numbers to show this doesn't make the general case
slower: it turns out that unlikely() != zero cost, as we found out when
we optimised out some of the redundant magic checking. 

Also, each bit stolen from the magic makes the magic less secure as a
nonce/cookie defending against overwrite attacks. 

> Being able to mark an existing talloc memory
> buffer as holding security sensitive data,
> and having it auto-zero'ed out on realloc/free does
> seem like an addition that will certainly
> be used (let's just make this a one-way
> gate to avoid extra complexity).

I'm still not convinced.  To make some progress I've positively
reviewed the MR (now I have my head mostly back above water), my only
concern delaying me was naming (sorry!). 

Andrew Bartlett
Andrew Bartlett
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT   

More information about the samba-technical mailing list