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

[Stefan Berger <stefanb@linux.ibm.com>]

On 9/5/21 4:11 AM, Vitaly Chikunov wrote:
> 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.

Or you could add an option for whether to overwrite the source...


>
> (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

You are right. I had checkout out this commit here:

commit abd30777cc72029e8a44e4b67201cae8ed3d19c1 (HEAD -> OpenSSL_1_1_0, 
tag: OpenSSL_1_1_0)
Author: Matt Caswell <matt@openssl.org>
Date:   Thu Aug 25 16:29:18 2016 +0100

     Prepare for 1.1.0 release

     Reviewed-by: Richard Levitte <levitte@openssl.org


It shows this here:

$ grep -r OPENSSL_VERSION_NUM ./include/
./include/openssl/crypto.h:#  define SSLEAY_VERSION_NUMBER 
OPENSSL_VERSION_NUMBER
./include/openssl/opensslv.h:# define OPENSSL_VERSION_NUMBER 0x1010000fL

There's an 'f' at the 'end'.

   Stefan


>
> Thanks,
>