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

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

[Vitaly Chikunov]

- Make digest name search work just with simple strcmp() and two arrays.
- Make newer name "streebogX" alias of older and backward compatible name
  "md_gost12_X".
- Remove get_digestbyname() which was doing two hash name resolving
  attempts, reverting to use plain EVP_get_digestbyname(). This will
  force the user to specify the proper hash name depending on what
  OpenSSL provides, allowing to specify older hash name in older
  OpenSSL. (This may be improved later, for example, by resolving
  hash name to a numerical id and the id to a proper hash name. Or
  we could resolve a hash name to the OpenSSL hash NID and resolve
  the NID to a internal hash name. In this case we don't need to keep
  hash alias names at all.)
---
 src/libimaevm.c | 89 +++++++++------------------------------------------------
 1 file changed, 13 insertions(+), 76 deletions(-)

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] = {
 	[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",
 };
 
 /*
@@ -161,8 +166,6 @@ const char *get_hash_algo_by_id(int algo)
 {
 	if (algo < PKEY_HASH__LAST)
 	    return pkey_hash_algo[algo];
-	if (algo < HASH_ALGO__LAST)
-	    return hash_algo_name[algo];
 
 	log_err("digest %d not found\n", algo);
 	return "unknown";
@@ -286,35 +289,6 @@ static int add_dev_hash(struct stat *st, EVP_MD_CTX *ctx)
 	return !EVP_DigestUpdate(ctx, &dev, sizeof(dev));
 }
 
-/* Call EVP_get_digestbyname with provided name and with alias,
- * which is first name in the comma separated list of names
- * in pkey_hash_algo.
- */
-static const EVP_MD *get_digestbyname(const char *name)
-{
-	const EVP_MD *md;
-
-	md = EVP_get_digestbyname(params.hash_algo);
-	if (!md) {
-		char alias[MAX_DIGEST_NAME];
-		int len;
-		int algo_id;
-		const char *algo_alias;
-
-		/* try to get algo by its alias */
-		algo_id = get_hash_algo(params.hash_algo);
-		algo_alias = get_hash_algo_by_id(algo_id);
-		len = strchrnul(algo_alias, ',') - algo_alias;
-		if (len < sizeof(alias)) {
-			memcpy(alias, algo_alias, len);
-			alias[len] = '\0';
-			if (strcmp(params.hash_algo, alias))
-				md = EVP_get_digestbyname(alias);
-		}
-	}
-	return md;
-}
-
 int ima_calc_hash(const char *file, uint8_t *hash)
 {
 	const EVP_MD *md;
@@ -336,7 +310,7 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 		return err;
 	}
 
-	md = get_digestbyname(params.hash_algo);
+	md = EVP_get_digestbyname(params.hash_algo);
 	if (!md) {
 		log_err("EVP_get_digestbyname(%s) failed\n", params.hash_algo);
 		return 1;
@@ -572,42 +546,6 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 	return 0;
 }
 
-/* compare algo names case insensitively and ignoring separators */
-static int algocmp(const char *a, const char *b)
-{
-	while (*a && *b) {
-		int cha, chb;
-
-		cha = tolower((unsigned char)*a++);
-		if (!isalnum(cha))
-			continue;
-		chb = tolower((unsigned char)*b++);
-		if (!isalnum(chb)) {
-			a--;
-			continue;
-		}
-		if (cha != chb)
-			return -1;
-	}
-	return *a || *b;
-}
-
-static int strmatch(const char *needle, const char *haystack)
-{
-	int len = strlen(needle);
-	const char *p = haystack;
-	const char *t;
-
-	for (t = strchrnul(p, ','); *p; p = t, t = strchrnul(p, ',')) {
-		if (t - p == len &&
-		    !strncmp(needle, p, len))
-			return 0;
-		if (!*t++)
-			break;
-	}
-	return 1;
-}
-
 int get_hash_algo(const char *algo)
 {
 	int i;
@@ -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;
-	}
 
 	log_info("digest %s not found, fall back to sha1\n", algo);
 	return PKEY_HASH_SHA1;
-- 
2.11.0

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

[Mimi Zohar]

Hi Vitaly,

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"?

>  	[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".

> @@ -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?

Mimi

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

[Vitaly Chikunov]

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.

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

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

[Mimi Zohar]

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
> 

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

[Vitaly Chikunov]

Mimi,

On Tue, Feb 12, 2019 at 02:17:19PM -0500, Mimi Zohar wrote:
> On Tue, 2019-02-12 at 20:23 +0300, Vitaly Chikunov wrote:
> > 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.

But for libimaevm we have openssl as crypto library not the kernel. That
pkey_hash_algo/hash_algo_name lists names does not need to reflect kernel
names at all for things to work, since only openssl is interfaced. It is
question of user interface and compatibility with older libs.