[PATCH] memset_s() and talloc_set_secure()

[Andreas Schneider]

On Thursday, 11 October 2018 15:58:44 CEST Ralph Böhme wrote:
> On Thu, Oct 11, 2018 at 03:26:33PM +0200, Andreas Schneider via samba-
technical 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-memse
> >t_s
> https://git.samba.org/?p=asn/samba.git;a=commitdiff;h=c95e28c4be2ca057061db2
> 3237ff6303059c1b13 still misses the things I mentioned.

#ifdefs on typedefs don't work.

-- 
Andreas Schneider                      asn at samba.org
Samba Team                             www.samba.org
GPG-ID:     8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D