[PATCH] memset_s() and talloc_set_secure()

[Simo]

On Wed, 2019-03-27 at 12:02 +1300, Andrew Bartlett wrote:
> On Tue, 2019-03-26 at 18:15 -0400, Simo wrote:
> > 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.
> 
> That has been the argument for every talloc feature added, and between
> them talloc_free() has become quite costly. 
> 
> > > 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 ?
> 
> Yes.  talloc_free() is a very hot path.

Ok, I just wanted to understand the concern, we can measure the change
and see.

> > > 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.
> 
> If we do this, then my condition is that the flag is taken from
> somewhere else, eg unifying the POOL flags. 
> 
> > > > 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.
> 
> It still seems, compared the the vast majority of talloc use that this
> is a niche feature that can be implemented outside libtalloc using
> existing features.  We have found it incredibly difficult to remove
> additional 'helpful' features from talloc and I'm keen to avoid it
> where there is a practical alternative.

If it were possible to do this meaningfully outside of talloc proper I
would have not brought this up. Nut it isn't. The whole point of
zeroization of secrets is to not leave them in memory, and dealing with
reallocation is crucial for this to be working.

> I also dislike the additional complexity in the talloc_realloc() case,
> this is even more code for something that (statistically) just won't
> happen. 

Actually the patch mostly re-purposes code that was already there.

> I hope this clarifies things, and I'm sorry I didn't get the review tag
> to Andreas's work earlier, as I've mentioned elsewhere I've been rather
> swamped. 

No problem,
as long as any "blocking" is based on data and not just "feelings" we
are all good.

Simo.