Re: [PATCH v4 3/5] ima-avm-utils: Change read_priv_key to use EVP_PKEY API

[Mimi Zohar <zohar@linux.ibm.com>]

On Fri, 2019-06-14 at 04:54 +0300, Vitaly Chikunov wrote:
> Introduce read_priv_pkey() to read keys using EVP_PKEY, and change
> read_priv_key() to be wrapper for it.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>  src/libimaevm.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index da0f422..c620c1e 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -753,10 +753,10 @@ void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key)
>  	free(pkey);
>  }
> 
> -static RSA *read_priv_key(const char *keyfile, const char *keypass)
> +static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
>  {
>  	FILE *fp
> -	RSA *key;
> +	EVP_PKEY *key;

In read_pub_pkey() EVP_PKEY is named pkey, not key.

> 
>  	fp = fopen(keyfile, "r");
>  	if (!fp) {
> @@ -764,18 +764,40 @@ static RSA *read_priv_key(const char *keyfile, const char *keypass)
>  		return NULL;
>  	}
>  	ERR_load_crypto_strings();
> -	key = PEM_read_RSAPrivateKey(fp, NULL, NULL, (void *)keypass);
> +	key = PEM_read_PrivateKey(fp, NULL, NULL, (void *)keypass);
>  	if (!key) {
>  		char str[256];
> 
> -		ERR_error_string(ERR_get_error(), str);
> -		log_err("PEM_read_RSAPrivateKey() failed: %s\n", str);
> +		ERR_error_string(ERR_peek_error(), str);
> +		log_err("PEM_read_PrivateKey() failed: %s\n", str);
> +#ifdef USE_FPRINTF
> +		ERR_print_errors_fp(stderr);
> +#else
> +		ERR_clear_error();
> +#endif

Why is this additional print needed?  Are you expecting multiple
errors?  By calling both log_err() and ERR_print_errors_fp() won't the
same error message be duplicated?  If calling "ERR_print_errors_fp()"
is indeed needed, please make this change as a separate patch, with an
appropriate patch description.

>  	}
> 
>  	fclose(fp);
>  	return key;
>  }
> 
> +static RSA *read_priv_key(const char *keyfile, const char *keypass)
> +{
> +	EVP_PKEY *pkey;
> +	RSA *key;
> +
> +	pkey = read_priv_pkey(keyfile, keypass);
> +	if (!pkey)
> +		return NULL;
> +	key = EVP_PKEY_get1_RSA(pkey);
> +	EVP_PKEY_free(pkey);
> +	if (!key) {
> +		log_err("sign_hash_v1: unsupported key type\n");
> +		return NULL;
> +	}

At least at this point in the patch series, failing to get the private
key isn't limited to sign_hash_v1.  Perhaps that might be true later.

Mimi

> +	return key;
> +}
> +
>  static int get_hash_algo_v1(const char *algo)
>  {
>