[PATCH] memset_s() and talloc_set_secure()

[Simo]

	On Wed, 2019-03-27 at 13:56 +1300, Andrew Bartlett wrote:
> On Tue, 2019-03-26 at 20:33 -0400, Simo wrote:
> > On Wed, 2019-03-27 at 12:02 +1300, Andrew Bartlett wrote:
> > > 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.
> 
> To be clear we already have this in _talloc_realloc():
> 
> 	/* don't allow realloc on referenced pointers */
> 	if (unlikely(tc->refs)) {
> 		return NULL;
> 	}
> 
> So I don't take it as proven that talloc_realloc() proves this must be
> done in talloc.  
> 
> Likewise we could abuse the must-maligned talloc_memlimit() and trigger
> on this instead:
> 
> 	if (tc->limit && (size > tc->size)) {
> 		if (!talloc_memlimit_check(tc->limit, (size - tc-
> > size))) {
> 			errno = ENOMEM;
> 			return NULL;
> 		}
> 	}
> 
> I'm hand-waving, I've not tested these and both would clearly be
> categorised as a cludge (and talloc_reference() games probably would
> need talloc_unlink() to free it specifically), and I would still
> seriously suggest we skip both and just clearly document.
> 
> In short I would prefer this outside libtalloc for the first
> implementation, and I still think that is viable.

The above are really gross hacks though.
You are asking to build a security feature to rely on side-effects of
two features both of which has been marked as desired to go by various
people.

That's not a good foundation to build anything on, because then people
will change those side effects without realizing they are compromising
a security feature.

I would rather gut away the memlimit feature, I can propose a patch to
remove most of it w/o technically breaking binary compatibility, if
that's the price to pay to get zeroization in. But security features
should be built on good foundations.