Stefan,
On Sat, Sep 04, 2021 at 09:10:50PM -0400, Stefan Berger wrote:
>
> On 9/4/21 6:50 AM, Vitaly Chikunov wrote:
> > After CRYPTO_secure_malloc_init OpenSSL will automatically store private
> > keys in secure heap. OPENSSL_secure_malloc(3):
> >
> > If a secure heap is used, then private key BIGNUM values are stored
> > there. This protects long-term storage of private keys, but will not
> > necessarily put all intermediate values and computations there.
> >
> > Additionally, we try to keep user passwords in secure heap too.
> > This facility is only available since OpenSSL_1_1_0-pre1.
> > Signed-off-by: Vitaly Chikunov <vt@xxxxxxxxxxxx>
> > Reviewed-by: Bruno Meneguele <bmeneg@xxxxxxxxxx>
> > Cc: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > ---
> > Change since v3:
> > - Undo secure heap handling from (file2bin and) calc_evm_hmac since this
> > is debugging tool only. Add comment about this.
> > - Since there is only code removals and new comments I keep Reviewed-by
> > tag.
> >
> > src/evmctl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 85 insertions(+), 12 deletions(-)
> >
> > @@ -2596,15 +2616,41 @@ static struct option opts[] = {
> > };
> > +/*
> > + * Copy password from optarg into secure heap, so it could be
> > + * freed in the same way as a result of get_password().
> > + */
> > +static char *optarg_password(char *optarg)
> > +{
>
>
> Mimi applied my patch that takes the password from an environment variable
> to the next-testing branch. The code there (imaevm_params.keypass =
> getenv("..."))would have to change as well calling this function here. Even
> though the man page of getenv says that 'The caller must take care not to
> modify this string, since that would change the environment of the process'
> we should be able to overwrite the env variable's password value just like
> here. Maybe for this purpose rename this function to dup_password() ?
`/proc/<pid>/environ' is never world readable (unlike `cmdline'), so we
don't even need to overwrite it there. But, we can.
(Btw, I found the 'EVMCTL_KEY_PASSWORD' patch.)
> > @@ -2643,6 +2691,7 @@ int main(int argc, char *argv[])
> > ENGINE *eng = NULL;
> > unsigned long keyid;
> > char *eptr;
> > + char *keypass = NULL; /* @secure heap */
> > #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
> > OPENSSL_init_crypto(
> > @@ -2651,6 +2700,17 @@ int main(int argc, char *argv[])
> > #endif
> > OPENSSL_INIT_ENGINE_ALL_BUILTIN, NULL);
> > #endif
> > +#if OPENSSL_VERSION_NUMBER > 0x10100000
> > + /*
> > + * This facility is available since OpenSSL_1_1_0-pre1.
>
>
> Shouldn't the comparison then not include 0x10100000?
>
> #if OPENSSL_VERSION_NUMBER >= 0x10100000
The number is from `include/openssl/opensslv.h' from the time when
relevant commit is already applied. So, I use `>'. It seems good match
for OpenSSL_1_1_0-pre1 too:
$ git grep OPENSSL_VERSION_NUMBER OpenSSL_1_1_0-pre1:include/openssl/opensslv.h
OpenSSL_1_1_0-pre1:include/openssl/opensslv.h:# define OPENSSL_VERSION_NUMBER 0x10100001L
Thanks,
