[PATCH] memset_s() and talloc_set_secure()

[Andrew Bartlett]

On Thu, 2018-10-11 at 15:26 +0200, Andreas Schneider wrote:
> On Thursday, 11 October 2018 13:15:17 CEST Stefan Metzmacher wrote:
> > Am 11.10.2018 um 13:07 schrieb Andrew Bartlett via samba-technical:
> > > On Thu, 2018-10-11 at 12:17 +0200, Andreas Schneider via samba-
> > > 
> > > technical wrote:
> > > > Hello,
> > > > 
> > > > the attached patch adds memset_s() [1] and talloc_set_secure(). It will
> > > > make sure that memory is zeroed/erased before freeing to not keep
> > > > secrets around.> 
> > > 
> > > Stepping back a moment, how do you handle talloc_realloc()?
> > > 
> > > That either needs to be banned or handled to ensure the old memory is
> > > wiped after a memcpy() to new memory (with performance losss).
> > > 
> > > (And that will all need tests).
> > > 
> > > Sorry this is turning into a can of worms, but if we do this we need to
> > > do it completely.
> > 
> > Yes, I also discussed privately with Andreas that we need to make sure
> > talloc_report() doesn't leak the content.
> > 
> > The current idea is:
> > 
> > #define talloc_keep_secret(ptr) _talloc_keep_secret(ptr, #ptr);
> > void _talloc_keep_secret(const void *ptr, const char *name);
> > 
> > While I may prefer to pass name explicit.
> > I guess talloc_asprintf_append* also needs special handling.
> > We need to decide what to do with talloc_strdup() and even more complex
> > talloc_asprintf(..., "%s", secret_talloc_string).
> 
> What do you mean? That in case we pass a pointer to such a function it should 
> be inherited?
> 
> I think if you do that you should do it like:
> 
> char *password = talloc_strdup(talloc_secret_string);
> talloc_keep_secret(password);
> 
> And not magically to it. I think explicitly calling it is better.
> 
> > Do we force the caller to reuse talloc_keep_secret() or do we want to
> > somehow inherit the secret state.
> 
> I would not inherit it, only talloc_realloc() should inherit it.
> 
> The current state is here:
> 
> https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master-memset_s

This still doesn't handle realloc correctly.  The current code there
only handles the non-pool case when ALWAYS_REALLOC is defined, which
never is.  

You need to force all talloc_realloc() calls on secret memory though
this path, as realloc() won't let you zero the memory after, and unlike
the talloc header (which we overstamp and then restore) you don't do
something similar with the content. 

talloc_realloc() is rare, and talloc_set_secret() is even more rare.  I
suggest just banning the combination and checking for things like
talloc_realloc() and talloc_asprintf_append() in the small number of
impacted code paths manually. 

A key (pardon the pun) use case for this on the AD DC will be secrets
in the DB, after decryption.  These are pretty narrowly used and I
can't think of any reason why these calls would be used with them, I
could certainly live with this restriction.

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba