Re: [PATCH 1/2] Fix sign_hash not observing the hashalgo argument

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

On Thu, 2021-01-07 at 16:15 +0300, Vitaly Chikunov wrote:
> On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote:
> > Patrick, Mimi,
> > 
> > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote:
> > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote:
> > > > This fixes sign_hash not using the correct algorithm for creating the
> > > > signature, by ensuring it uses the passed in variable value.
> > > > 
> > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>
> > > 
> > > Thank you.  This is a regression first introduced in commit
> > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API").
> > 
> > Ah, when sign_hash() is used not via evmctl, but as a library
> > imaevm_params may be not set.
> 
> Thinking about imaevm_params -- it's still belong to a library and
> extensively used in libimaevm.c, so I wonder if the fix is perfect.
> Since, now there is hash_algo and algo duplication which one to prefer
> and why?
> 
> Maybe, it should be [also] set like keypass in sign_hash?
> 
>   int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
>   {
> 	  if (keypass)
> 		  imaevm_params.keypass = keypass;
> 
> +	  if (hashalgo)
> +		  imaevm_params.hash_algo = hashalgo;
> 
> 	  return imaevm_params.x509 ?
> 		  sign_hash_v2(hashalgo, hash, size, keyfile, sig) :
> 		  sign_hash_v1(hashalgo, hash, size, keyfile, sig);
>   }
> 
> 

As seen above, the main difference between keypass and hashalgo is that
hashalgo is being passed as an argument, while keypass isn't.  Instead
of changing the function arguments, it looks like keypass was stored as
global variable.  For this reason, the priority should be the function
argument, not the global variable.

thanks,

Mimi