[PATCH] memset_s() and talloc_set_secure()
Andrew Bartlett
abartlet at samba.org
Tue Mar 26 21:48:39 UTC 2019
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 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.
Also, each bit stolen from the magic makes the magic less secure as a
nonce/cookie defending against overwrite attacks.
> 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!).
Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
More information about the samba-technical
mailing list