[openssl-dev] [openssl.org #4116] [PATCH] Reimplement non-asm OPENSSL_cleanse()

[Alessandro Ghedini via RT]

(sorry for the delay, but I've been travelling and moving)

On Sat, Oct 31, 2015 at 11:01:22pm +0000, Brian Smith via RT wrote:
> On Sat, Oct 31, 2015 at 11:50 AM, Alessandro Ghedini via RT <rt at openssl.org>
> The point is to let the person building OPENSSL say "I want the build to
> fail if there isn't a secure way to zero memory, because I'm expecting
> there to always be one in my configuration." Alternatively, there could be
> an OPENSSL_USE_MEMSET_S flag that says "always use memset_s and never
> anything else."

So, I added memset_s support guarded by the OPENSSL_USE_MEMSET_S ifdef, but it
needs to be specified manually during configuration for now. Ideally it should
be added to the configration of the platforms that support memset_s. I also
added support for explicit_bzero() on OpenBSD. None of this has been tested
though.

However, the thing about OpenSSL's build system is that the asm implementations
will *always* take priority, so you'll also need to specify "no-asm" or this
whole thing won't be built. This is another reason for dropping the assembly
implementations of OPENSSL_cleanse btw.

Also, FTR, apparently SecureZeroMemory() doesn't work on the mingw version that
Travis installs, so that's currently failing to build.

> > > The primary purpose of the assembly language implementations is to
> > reduce the
> > > possibility that the C compiler will do the weird things that C
> > compilers love
> > > to do.
> >
> > According to the changelog and git log, the primary purpose of the asm
> > implementations was to improve performance (see commit b2dba9b). Using the
> > volatile pointer implementation would IMO make these optimizations useless,
> > hence the proposal to drop them and make things simpler.
> 
> 
> Sorry. Instead of "primary purpose" (which implies intent), I should have
> said 'primary advantage".  An assembly language implementation is more
> likely to work than a C implementation because the C compiler generally
> won't analyze externally-assembled code

That's also valid for my implementation IMO. The whole point of "volatile" is
to prevent the compiler from trying to guess the content of the variable and do
weird things with it. In any case we still need to have support for the no-asm 
case where neither memset_s or SecureZeroMemory are present. And as explained
above, having the asm functions prevents people from actually using the safer
alternatives so I still think it'd be better to just drop them.

In order to avoid having builds that silently optimize away OPENSSL_cleanse
it'd be nice to have a test case to check for that. OpenBSD/LibreSSL have
somthing along those lines [0], but it doesn't seem to work as expected (well,
at least not with OpenSSL), so alternative ideas are welcome.

In any case, I added another commit to my PR that removes the asm functions,
but I left it as a separate commit so it can be easily dropped.

Cheers

[0] https://github.com/libressl-portable/openbsd/blob/master/src/regress/lib/libc/explicit_bzero/explicit_bzero.c