Linux-Integrity Archive on lore.kernel.org
 help / Atom feed
* [PATCH] ima-evm-utils: simplify digest alias handling
@ 2019-02-12  0:14 Vitaly Chikunov
  2019-02-12 16:23 ` Mimi Zohar
  0 siblings, 1 reply; 5+ messages in thread
From: Vitaly Chikunov @ 2019-02-12  0:14 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin, linux-integrity

- 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ima-evm-utils: simplify digest alias handling
  2019-02-12  0:14 [PATCH] ima-evm-utils: simplify digest alias handling Vitaly Chikunov
@ 2019-02-12 16:23 ` Mimi Zohar
  2019-02-12 17:23   ` Vitaly Chikunov
  0 siblings, 1 reply; 5+ messages in thread
From: Mimi Zohar @ 2019-02-12 16:23 UTC (permalink / raw)
  To: Vitaly Chikunov, Mimi Zohar, Dmitry Kasatkin, linux-integrity

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ima-evm-utils: simplify digest alias handling
  2019-02-12 16:23 ` Mimi Zohar
@ 2019-02-12 17:23   ` Vitaly Chikunov
  2019-02-12 19:17     ` Mimi Zohar
  0 siblings, 1 reply; 5+ messages in thread
From: Vitaly Chikunov @ 2019-02-12 17:23 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: Mimi Zohar, Dmitry Kasatkin, linux-integrity

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ima-evm-utils: simplify digest alias handling
  2019-02-12 17:23   ` Vitaly Chikunov
@ 2019-02-12 19:17     ` Mimi Zohar
  2019-02-12 19:31       ` Vitaly Chikunov
  0 siblings, 1 reply; 5+ messages in thread
From: Mimi Zohar @ 2019-02-12 19:17 UTC (permalink / raw)
  To: Vitaly Chikunov; +Cc: Mimi Zohar, Dmitry Kasatkin, linux-integrity

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
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ima-evm-utils: simplify digest alias handling
  2019-02-12 19:17     ` Mimi Zohar
@ 2019-02-12 19:31       ` Vitaly Chikunov
  0 siblings, 0 replies; 5+ messages in thread
From: Vitaly Chikunov @ 2019-02-12 19:31 UTC (permalink / raw)
  To: Mimi Zohar; +Cc: Mimi Zohar, Dmitry Kasatkin, linux-integrity

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.



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-12  0:14 [PATCH] ima-evm-utils: simplify digest alias handling Vitaly Chikunov
2019-02-12 16:23 ` Mimi Zohar
2019-02-12 17:23   ` Vitaly Chikunov
2019-02-12 19:17     ` Mimi Zohar
2019-02-12 19:31       ` Vitaly Chikunov

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org linux-integrity@archiver.kernel.org
	public-inbox-index linux-integrity


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/ public-inbox