[PATCH] memset_s() and talloc_set_secure()

[Jeremy Allison]

On Tue, Mar 26, 2019 at 06:15:17PM -0400, Simo wrote:
> 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 ?

Well to address any concerns how about we get some numbers with and without
the code changes ?

A loop with several million random talloc allocate/pool/free/realloc
operations of sizes between 1-5MB should do I think.

Andrew, if that shows no measurable changes between multiple
runs with and without the change, would that satisfy ?