diff mbox series

[ima-evm-utils,v3,06/15] Add missing EVP_MD_CTX_free() call in calc_evm_hash()

Message ID 20220914022956.1359218-7-zohar@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series address deprecated warnings | expand

Commit Message

Mimi Zohar Sept. 14, 2022, 2:29 a.m. UTC 
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>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 57 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 17 deletions(-)

Comments

Vitaly Chikunov Sept. 14, 2022, 2:51 p.m. UTC | #1 
Mimi,

On Tue, Sep 13, 2022 at 10:29:47PM -0400, 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>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 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 27d2061f23be..b8c92aad6b84 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -332,11 +332,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;
> @@ -350,12 +356,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;

Why it clears errno (here and below)?

errno(3) says "The value of errno is never set to zero by any system
call or library function."

Thanks,

>  		return -1;
>  	}
>  
> @@ -392,20 +397,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++) {
> @@ -416,7 +431,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) {
> @@ -424,7 +440,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){
> @@ -433,7 +450,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)) {
> @@ -443,7 +460,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 {
> @@ -464,7 +482,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;
>  		}
>  	}
>  
> @@ -518,29 +536,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)
> @@ -576,6 +598,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;
>  		}
>  	}
> -- 
> 2.31.1
Mimi Zohar Sept. 15, 2022, 11:58 a.m. UTC | #2 
Hi Vitaly,

Thank you for this and the other reviews.  They'll be addressed in the
next version.

On Wed, 2022-09-14 at 17:51 +0300, Vitaly Chikunov wrote:
> > @@ -350,12 +356,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;
> 
> Why it clears errno (here and below)?
> 
> errno(3) says "The value of errno is never set to zero by any system
> call or library function."

evmctl, itself, is not a system call or a library function.  Is this a
generic statement or here in particular as to how evmctl should handle
failed system calls?   Another example is reading the keyfile.  The
existence of which is not required.

thanks,

Mimi
Vitaly Chikunov Sept. 15, 2022, 3:36 p.m. UTC | #3 
On Thu, Sep 15, 2022 at 07:58:51AM -0400, Mimi Zohar wrote:
> Hi Vitaly,
> 
> Thank you for this and the other reviews.  They'll be addressed in the
> next version.
> 
> On Wed, 2022-09-14 at 17:51 +0300, Vitaly Chikunov wrote:
> > > @@ -350,12 +356,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;
> > 
> > Why it clears errno (here and below)?
> > 
> > errno(3) says "The value of errno is never set to zero by any system
> > call or library function."
> 
> evmctl, itself, is not a system call or a library function. 

Ah, I wasn't attentive this is only evmctl. [But there's similar commit
acb19d1 ("Reset 'errno' after failure to open or access a file")
changing libimaevm.c which is wrong.]

Perhaps it should be noted in commit message that errno is cleared
because it's error message is already printed to avoid double printing
at exit of cmd handler.

> Is this a
> generic statement or here in particular as to how evmctl should handle
> failed system calls?   Another example is reading the keyfile.  The
> existence of which is not required.

          log_err("Failed to stat: %s\n", file);

This does not even output errno code, but it could be very informative
to user. I think it's better to print (at least errno or) strerror for
users there (and on other syscall errors log_err instances.

Maybe to add special log function (like log_strerr) just for evmctl
which will print (non "\n"-terminated) error message (similar to
warn(3)) with strerror output appended (and commented in the code why
it) clears errno (so that later handlers do not print it again).

ps. About libimaevm.c--I think errno should not be touched there as this
breaks what coders expect from libraries. If this affects exit of evmctl
then it should be handled in evmctl, not in the library. (Of course it's
better to add strerror(errno) to log_err there too, but not by the
proposed above function.)

Vitaly,

> 
> thanks,
> 
> Mimi
>
Mimi Zohar Sept. 16, 2022, 1:07 p.m. UTC | #4 
On Thu, 2022-09-15 at 18:36 +0300, Vitaly Chikunov wrote:
> On Thu, Sep 15, 2022 at 07:58:51AM -0400, Mimi Zohar wrote:
> > Hi Vitaly,
> > 
> > Thank you for this and the other reviews.  They'll be addressed in the
> > next version.
> > 
> > On Wed, 2022-09-14 at 17:51 +0300, Vitaly Chikunov wrote:
> > > > @@ -350,12 +356,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;
> > > 
> > > Why it clears errno (here and below)?
> > > 
> > > errno(3) says "The value of errno is never set to zero by any system
> > > call or library function."
> > 
> > evmctl, itself, is not a system call or a library function. 
> 
> Ah, I wasn't attentive this is only evmctl. [But there's similar commit
> acb19d1 ("Reset 'errno' after failure to open or access a file")
> changing libimaevm.c which is wrong.]
> 
> Perhaps it should be noted in commit message that errno is cleared
> because it's error message is already printed to avoid double printing
> at exit of cmd handler.
> 
> > Is this a
> > generic statement or here in particular as to how evmctl should handle
> > failed system calls?   Another example is reading the keyfile.  The
> > existence of which is not required.
> 
>           log_err("Failed to stat: %s\n", file);
> 
> This does not even output errno code, but it could be very informative
> to user. I think it's better to print (at least errno or) strerror for
> users there (and on other syscall errors log_err instances.
> 
> Maybe to add special log function (like log_strerr) just for evmctl
> which will print (non "\n"-terminated) error message (similar to
> warn(3)) with strerror output appended (and commented in the code why
> it) clears errno (so that later handlers do not print it again).
> 
> ps. About libimaevm.c--I think errno should not be touched there as this
> breaks what coders expect from libraries. If this affects exit of evmctl
> then it should be handled in evmctl, not in the library. (Of course it's
> better to add strerror(errno) to log_err there too, but not by the
> proposed above function.)

Thank you for the suggestions.  log_errno() is already defined.  Not
sure how I missed that.  So use log_errno() in libimaevm.c.  For
evmctl.c, define a wrapper named log_and_reset_errno(), with your
suggested patch description.

Since none of the changes in next-testing have been released, they can
still be fixed/squashed as needed.

thanks!

Mimi
diff mbox series

Patch

diff --git a/src/evmctl.c b/src/evmctl.c
index 27d2061f23be..b8c92aad6b84 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -332,11 +332,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;
@@ -350,12 +356,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;
 	}
 
@@ -392,20 +397,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++) {
@@ -416,7 +431,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) {
@@ -424,7 +440,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){
@@ -433,7 +450,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)) {
@@ -443,7 +460,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 {
@@ -464,7 +482,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;
 		}
 	}
 
@@ -518,29 +536,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)
@@ -576,6 +598,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;
 		}
 	}