Re: [PATCH ima-evm-utils v2 07/12] Add missing EVP_MD_CTX_free() call in calc_evm_hash()

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

On 9/6/22 15:50, Mimi Zohar wrote:
> When EVP_MD_CTX_new() call was added, the corresponding EVP_MD_CTX_free()
> was never called.  Properly free it.
> 
> Fixes: 81010f0d87ef ("ima-evm-utils: Add backward compatible support for openssl 1.1")
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 57 ++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index a9b2f1040787..b89e74e06c3d 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -331,11 +331,17 @@ err:
>   	return -1;
>   }
>   
> +/*
> + * calc_evm_hash - calculate the file metadata hash
> + *
> + * Returns 0 for EVP_ function failures. Return -1 for other failures.
> + * Return hash algorithm size on success.
> + */
>   static int calc_evm_hash(const char *file, unsigned char *hash)
>   {
>           const EVP_MD *md;
>   	struct stat st;
> -	int err;
> +	int err = -1;
>   	uint32_t generation = 0;
>   	EVP_MD_CTX *pctx;
>   	unsigned int mdlen;
> @@ -349,12 +355,11 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   #if OPENSSL_VERSION_NUMBER < 0x10100000
>   	EVP_MD_CTX ctx;
>   	pctx = &ctx;
> -#else
> -	pctx = EVP_MD_CTX_new();
>   #endif
>   
>   	if (lstat(file, &st)) {
>   		log_err("Failed to stat: %s\n", file);
> +		errno = 0;
>   		return -1;
>   	}
>   
> @@ -391,20 +396,30 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   	list_size = llistxattr(file, list, sizeof(list));
>   	if (list_size < 0) {
>   		log_err("llistxattr() failed\n");
> +		errno = 0;
>   		return -1;
>   	}
>   
> +#if OPENSSL_VERSION_NUMBER >= 0x10100000
> +	pctx = EVP_MD_CTX_new();
> +	if (!pctx) {
> +		log_err("EVP_MD_CTX_new() failed\n");
> +		return 0;
> +	}
> +#endif
> +
>   	md = EVP_get_digestbyname(imaevm_params.hash_algo);
>   	if (!md) {
>   		log_err("EVP_get_digestbyname(%s) failed\n",
>   			imaevm_params.hash_algo);
> -		return 1;
> +		err = 0;
> +		goto out;
>   	}
>   
>   	err = EVP_DigestInit(pctx, md);
>   	if (!err) {
>   		log_err("EVP_DigestInit() failed\n");
> -		return 1;
> +		goto out;
>   	}
>   
>   	for (xattrname = evm_config_xattrnames; *xattrname != NULL; xattrname++) {
> @@ -415,7 +430,8 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   			if (err > sizeof(xattr_value)) {
>   				log_err("selinux[%u] value is too long to fit into xattr[%zu]\n",
>   					err, sizeof(xattr_value));
> -				return -1;
> +				err = -1;
> +				goto out;
>   			}
>   			strcpy(xattr_value, selinux_str);
>   		} else if (!strcmp(*xattrname, XATTR_NAME_IMA) && ima_str) {
> @@ -423,7 +439,8 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   			if (err > sizeof(xattr_value)) {
>   				log_err("ima[%u] value is too long to fit into xattr[%zu]\n",
>   					err, sizeof(xattr_value));
> -				return -1;
> +				err = -1;
> +				goto out;
>   			}
>   			hex2bin(xattr_value, ima_str, err);
>   		} else if (!strcmp(*xattrname, XATTR_NAME_IMA) && evm_portable){
> @@ -432,7 +449,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   			if (err < 0) {
>   				log_err("EVM portable sig: %s required\n",
>   					xattr_ima);
> -				return -1;
> +				goto out;
>   			}
>   			use_xattr_ima = 1;
>   		} else if (!strcmp(*xattrname, XATTR_NAME_CAPS) && (hmac_flags & HMAC_FLAG_CAPS_SET)) {
> @@ -442,7 +459,8 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   			if (err >= sizeof(xattr_value)) {
>   				log_err("caps[%u] value is too long to fit into xattr[%zu]\n",
>   					err + 1, sizeof(xattr_value));
> -				return -1;
> +				err = -1;
> +				goto out;
>   			}
>   			strcpy(xattr_value, caps_str);
>   		} else {
> @@ -463,7 +481,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   		err = EVP_DigestUpdate(pctx, xattr_value, err);
>   		if (!err) {
>   			log_err("EVP_DigestUpdate() failed\n");
> -			return 1;
> +			goto out;
>   		}
>   	}
>   
> @@ -517,29 +535,33 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   	err = EVP_DigestUpdate(pctx, &hmac_misc, hmac_size);
>   	if (!err) {
>   		log_err("EVP_DigestUpdate() failed\n");
> -		return 1;
> +		goto out;
>   	}
>   
>   	if (!evm_immutable && !evm_portable &&
>   	    !(hmac_flags & HMAC_FLAG_NO_UUID)) {
>   		err = get_uuid(&st, uuid);
>   		if (err)
> -			return -1;
> +			goto out;
>   
>   		err = EVP_DigestUpdate(pctx, (const unsigned char *)uuid, sizeof(uuid));
>   		if (!err) {
>   			log_err("EVP_DigestUpdate() failed\n");
> -			return 1;
> +			goto out;
>   		}
>   	}
>   
>   	err = EVP_DigestFinal(pctx, hash, &mdlen);
> -	if (!err) {
> +	if (!err)
>   		log_err("EVP_DigestFinal() failed\n");
> -		return 1
> -	}
>   
> -	return mdlen;
> +out:
> +#if OPENSSL_VERSION_NUMBER >= 0x10100000
> +	EVP_MD_CTX_free(pctx);
> +#endif
> +	if (err == 1)
> +		return mdlen;
> +	return err;
>   }
>   
>   static int sign_evm(const char *file, const char *key)
> @@ -575,6 +597,7 @@ static int sign_evm(const char *file, const char *key)
>   		err = lsetxattr(file, xattr_evm, sig, len, 0);
>   		if (err < 0) {
>   			log_err("setxattr failed: %s\n", file);
> +			errno = 0;
>   			return err;
>   		}
>   	}

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>