ima-evm-utils: simplify digest alias handling

PATCH Superseded Vitaly Chikunov 2 participants 5 messages Feb 12, 2019 Keys-trusted

Share & tools

Vitaly Chikunov

author

2019-02-12 00:14 UTC #0 lore raw

PATCH 1/1 ima-evm-utils: simplify digest alias handling

- 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(-)

src/libimaevm.c +13 ~~-76~~

index d9ffa13..ed77211 100644
@@ -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;

Signature · 1 line

2.11.0

Mimi Zohar

maintainer

2019-02-12 16:23 UTC #1 lore raw

Hi Vitaly,

Definitely a lot better.

src/libimaevm.c line 66

Quoted 10 lines

src/libimaevm.c +1 -1

index d9ffa13..ed77211 100644
@@ -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"?

Quoted 8 lines

src/libimaevm.c +0 -1

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

src/libimaevm.c const char *const pkey_hash_algo[PKEY_HASH__LAST] = { · line 83

Quoted 10 lines

src/libimaevm.c +7 -1

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

Quoted 8 lines

src/libimaevm.c +1 -1

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

src/libimaevm.c int get_hash_algo(const char *algo) · line 564

Quoted 10 lines

src/libimaevm.c +3 -4

 	/* 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

Vitaly Chikunov

author

2019-02-12 17:23 UTC #2 lore raw

Mimi,

Quoted 10 lines · Mimi Zohar

Definitely a lot better.
Quoted 8 lines
src/libimaevm.c +0 -0
index d9ffa13..ed77211 100644
@@ -63,7 +63,7 @@
 #include "imaevm.h"
 #include "hash_info.h"
 

Quoted 4 lines

src/libimaevm.c line 66
Quoted 2 lines
src/libimaevm.c +1 -1
-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.

Quoted 13 lines

src/libimaevm.c +4 -2

 	[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",
+};
+

Quoted 8 lines

src/libimaevm.c const char *const pkey_hash_algo[PKEY_HASH__LAST] = { · line 83
Quoted 5 lines
src/libimaevm.c +3 -0
+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.

Quoted 8 lines

src/libimaevm.c +1 -1

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

Quoted 14 lines

src/libimaevm.c int get_hash_algo(const char *algo) · line 564
Quoted 10 lines
src/libimaevm.c +3 -4
 	/* 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!

Quoted 2 lines

Mimi

Mimi Zohar

maintainer

2019-02-12 19:17 UTC #3 lore raw

Quoted 42 lines · Vitaly Chikunov

Mimi,
Quoted 10 lines · Mimi Zohar
Definitely a lot better.
Quoted 8 lines
src/libimaevm.c +0 -0
index d9ffa13..ed77211 100644
@@ -63,7 +63,7 @@
 #include "imaevm.h"
 #include "hash_info.h"
 
Quoted 4 lines
src/libimaevm.c line 66
Quoted 2 lines
src/libimaevm.c +1 -1
-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.
Quoted 13 lines
Quoted 13 lines
src/libimaevm.c +4 -2
 	[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",
+};
+
Quoted 8 lines
src/libimaevm.c const char *const pkey_hash_algo[PKEY_HASH__LAST] = { · line 83
Quoted 5 lines
src/libimaevm.c +3 -0
+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".

Quoted 7 lines

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

Quoted 37 lines

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.
Quoted 8 lines
Quoted 8 lines
src/libimaevm.c +1 -1
@@ -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;
 
Quoted 14 lines
src/libimaevm.c int get_hash_algo(const char *algo) · line 564
Quoted 10 lines
src/libimaevm.c +3 -4
 	/* 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!
Quoted 2 lines
Mimi

Vitaly Chikunov

author

2019-02-12 19:31 UTC #4 lore raw

Mimi,

Quoted 46 lines · Mimi Zohar

Quoted 45 lines · Vitaly Chikunov
Quoted 10 lines · Mimi Zohar
Definitely a lot better.
Quoted 8 lines
src/libimaevm.c +0 -0
index d9ffa13..ed77211 100644
@@ -63,7 +63,7 @@
 #include "imaevm.h"
 #include "hash_info.h"
 
Quoted 4 lines
src/libimaevm.c line 66
Quoted 2 lines
src/libimaevm.c +1 -1
-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.
Quoted 13 lines
Quoted 13 lines
src/libimaevm.c +4 -2
 	[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",
+};
+
Quoted 8 lines
src/libimaevm.c const char *const pkey_hash_algo[PKEY_HASH__LAST] = { · line 83
Quoted 5 lines
src/libimaevm.c +3 -0
+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.

Quoted 4 lines

Quoted 2 lines
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.