[PATCH] memset_s() and talloc_set_secure()

Simo simo at samba.org
Tue Mar 26 22:15:17 UTC 2019


On Wed, 2019-03-27 at 10:48 +1300, Andrew Bartlett wrote:
> 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
> that. 

I am not sure what is the concern ?
there is a cost only for the case where you set the flag, for all other
cases there should be no impact.

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

Are you concerned that 1 if statement will make a difference in
practice ?

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

Sure, that's a good rule of thumb that we do not want to eat into flags
for no good reason. But here we have a good reason.

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

What are you not convinced about exactly?
That it will be used?
It most certainly will be as Andreas plans to use the feature to mark
keys being transported over talloc()'d memory.

Simo.





More information about the samba-technical mailing list