Re: [PATCH ima-evm-utils v4] evmctl: Use secure heap for private keys and passwords

[Vitaly Chikunov <vt@altlinux.org>]

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@altlinux.org>
> > Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> > 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,