Re: [PATCH] ima-evm-utils: simplify digest alias handling

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

On Tue, 2019-02-12 at 20:23 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Tue, Feb 12, 2019 at 11:23:40AM -0500, Mimi Zohar wrote:
> > Definitely a lot better.
> > 
> > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > index d9ffa13..ed77211 100644
> > > --- a/src/libimaevm.c
> > > +++ b/src/libimaevm.c
> > > @@ -63,7 +63,7 @@
> > >  #include "imaevm.h"
> > >  #include "hash_info.h"
> > > 
> > > -const char *const pkey_hash_algo[PKEY_HASH__LAST] = {
> > > +const char *pkey_hash_algo[PKEY_HASH__LAST] = {
> > 
> > Dropping the "const"?
> 
> I will undo this change.
> 
> > >  	[PKEY_HASH_MD4]		= "md4",
> > >  	[PKEY_HASH_MD5]		= "md5",
> > >  	[PKEY_HASH_SHA1]	= "sha1",
> > > @@ -72,8 +72,13 @@ const char *const pkey_hash_algo[PKEY_HASH__LAST] = {
> > >  	[PKEY_HASH_SHA384]	= "sha384",
> > >  	[PKEY_HASH_SHA512]	= "sha512",
> > >  	[PKEY_HASH_SHA224]	= "sha224",
> > > -	[PKEY_HASH_STREEBOG_256] = "md_gost12_256,streebog256",
> > > -	[PKEY_HASH_STREEBOG_512] = "md_gost12_512,streebog512",
> > > +	[PKEY_HASH_STREEBOG_256] = "md_gost12_256",
> > > +	[PKEY_HASH_STREEBOG_512] = "md_gost12_512",
> > > +};
> > > +
> > > +const char *pkey_hash_algo_alias[PKEY_HASH__LAST] = {
> > > +	[PKEY_HASH_STREEBOG_256] = "streebog256",
> > > +	[PKEY_HASH_STREEBOG_512] = "streebog512",
> > >  };
> > > 
> > 
> > If the upstream kernel name is defined as "streebog", the alias would
> > be "gost".
> 
> To me it seems that meaning of what here is alias is really not
> important.  There is two names in OpenSSL md_gost12_X and streebogX,
> where streebogX is newer and less accessible, and md_gost12_X is
> more accessible in older version. While in the kernel name is streebogX.
> 
> Probably it's better to distinguish as openssl and kernel names, and not
> the name and alias.

The original design matched the kernel haah_info.h.

Mimi

> 
> Since libimaevm primarily works with OpenSSL API, I keep backward compatible
> names md_gost12_X in pkey_hash_algo, so when algo id is resolved to the name
> it resolved to the name which is present in the older OpenSSLs, thus we don't
> need to request EVP_get_digestbyname another time if user specified name is
> not found in OpenSSL.
> 
> > > @@ -615,15 +553,14 @@ int get_hash_algo(const char *algo)
> > >  	/* first iterate over builtin algorithms */
> > >  	for (i = 0; i < PKEY_HASH__LAST; i++)
> > >  		if (pkey_hash_algo[i] &&
> > > -		    !strmatch(algo, pkey_hash_algo[i]))
> > > +		    !strcmp(algo, pkey_hash_algo[i]))
> > >  			return i;
> > > 
> > >  	/* iterate over algorithms provided by kernel-headers */
> > > -	for (i = 0; i < HASH_ALGO__LAST; i++) {
> > > -		if (hash_algo_name[i] &&
> > > -		    !algocmp(algo, hash_algo_name[i]))
> > > +	for (i = 0; i < PKEY_HASH__LAST; i++)
> > > +		if (pkey_hash_algo_alias[i] &&
> > > +		    !strcmp(algo, pkey_hash_algo_alias[i]))
> > >  			return i;
> > > -	}
> > > 
> > 
> > This doesn't look right.  The comments don't reflect the code.
> > Shouldn't there be 3 loops - pkey_hash_algo, pkey_hash_algo_alias, and
> > then the kernel header?
> 
> My mistake. I forgot about this feature.
> 
> Thanks!
> 
> > 
> > Mimi
>