[PATCH 1/3] ima-avm-utils: Fix hash buffer overflow in verify_evm

[PATCH 1/3] ima-avm-utils: Fix hash buffer overflow in verify_evm

[Vitaly Chikunov]

Commit ae1319eeabd6 ("Remove hardcoding of SHA1 in EVM signatures")
introduces overflow of 20 byte buffer on the stack while calculating evm
hash. Also, invalid hash length is passed to the underlying verification
function. This prevents any non-SHA1 hashes from being properly
validated using evmctl.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/evmctl.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 1b46d58..94d7ab1 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -55,6 +55,7 @@
 #include <keyutils.h>
 #include <ctype.h>
 #include <termios.h>
+#include <assert.h>
 
 #include <openssl/sha.h>
 #include <openssl/pem.h>
@@ -760,13 +761,15 @@ static int cmd_sign_evm(struct command *cmd)
 
 static int verify_evm(const char *file)
 {
-	unsigned char hash[20];
+	unsigned char hash[64];
 	unsigned char sig[1024];
+	int mdlen;
 	int len;
 
-	len = calc_evm_hash(file, hash);
-	if (len <= 1)
-		return len;
+	mdlen = calc_evm_hash(file, hash);
+	assert(mdlen <= sizeof(hash));
+	if (mdlen <= 1)
+		return mdlen;
 
 	len = lgetxattr(file, "security.evm", sig, sizeof(sig));
 	if (len < 0) {
@@ -779,7 +782,7 @@ static int verify_evm(const char *file)
 		return -1;
 	}
 
-	return verify_hash(file, hash, sizeof(hash), sig + 1, len - 1);
+	return verify_hash(file, hash, mdlen, sig + 1, len - 1);
 }
 
 static int cmd_verify_evm(struct command *cmd)
-- 
2.11.0

[PATCH 2/3] ima-evm-utils: Add --xattr-user option for testing

[Vitaly Chikunov]

Keep ima/evm attributes in user namespace instead of security namespace.
Would be useful for testing purposes without having root privileges,
easier to understand, and because --sigfile does not work for evm
signatures.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/evmctl.c    | 32 ++++++++++++++++++++------------
 src/libimaevm.c |  2 +-
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 94d7ab1..032ea9d 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -145,6 +145,9 @@ static int find(const char *path, int dts, find_cb_t func);
 struct command cmds[];
 static void print_usage(struct command *cmd);
 
+static const char *xattr_ima = "security.ima";
+static const char *xattr_evm = "security.evm";
+
 static int bin2file(const char *file, const char *ext, const unsigned char *data, int len)
 {
 	FILE *fp;
@@ -531,7 +534,7 @@ static int sign_evm(const char *file, const char *key)
 		dump(sig, len);
 
 	if (xattr) {
-		err = lsetxattr(file, "security.evm", sig, len, 0);
+		err = lsetxattr(file, xattr_evm, sig, len, 0);
 		if (err < 0) {
 			log_err("setxattr failed: %s\n", file);
 			return err;
@@ -569,7 +572,7 @@ static int hash_ima(const char *file)
 		dump(hash, len);
 
 	if (xattr) {
-		err = lsetxattr(file, "security.ima", hash, len, 0);
+		err = lsetxattr(file, xattr_ima, hash, len, 0);
 		if (err < 0) {
 			log_err("setxattr failed: %s\n", file);
 			return err;
@@ -604,7 +607,7 @@ static int sign_ima(const char *file, const char *key)
 		bin2file(file, "sig", sig, len);
 
 	if (xattr) {
-		err = lsetxattr(file, "security.ima", sig, len, 0);
+		err = lsetxattr(file, xattr_ima, sig, len, 0);
 		if (err < 0) {
 			log_err("setxattr failed: %s\n", file);
 			return err;
@@ -771,14 +774,14 @@ static int verify_evm(const char *file)
 	if (mdlen <= 1)
 		return mdlen;
 
-	len = lgetxattr(file, "security.evm", sig, sizeof(sig));
+	len = lgetxattr(file, xattr_evm, sig, sizeof(sig));
 	if (len < 0) {
 		log_err("getxattr failed: %s\n", file);
 		return len;
 	}
 
 	if (sig[0] != 0x03) {
-		log_err("security.evm has no signature\n");
+		log_err("%s has no signature\n", xattr_evm);
 		return -1;
 	}
 
@@ -813,7 +816,7 @@ static int verify_ima(const char *file)
 		memcpy(sig, tmp, len);
 		free(tmp);
 	} else {
-		len = lgetxattr(file, "security.ima", sig, sizeof(sig));
+		len = lgetxattr(file, xattr_ima, sig, sizeof(sig));
 		if (len < 0) {
 			log_err("getxattr failed: %s\n", file);
 			return len;
@@ -956,7 +959,7 @@ static int setxattr_ima(const char *file, char *sig_file)
 	if (!sig)
 		return 0;
 
-	err = lsetxattr(file, "security.ima", sig, len, 0);
+	err = lsetxattr(file, xattr_ima, sig, len, 0);
 	if (err < 0)
 		log_err("setxattr failed: %s\n", file);
 	free(sig);
@@ -1152,7 +1155,7 @@ static int hmac_evm(const char *file, const char *key)
 
 	if (xattr) {
 		sig[0] = EVM_XATTR_HMAC;
-		err = lsetxattr(file, "security.evm", sig, len + 1, 0);
+		err = lsetxattr(file, xattr_evm, sig, len + 1, 0);
 		if (err < 0) {
 			log_err("setxattr failed: %s\n", file);
 			return err;
@@ -1208,9 +1211,9 @@ static int ima_fix(const char *path)
 		}
 		for (; size > 0; len++, size -= len, list += len) {
 			len = strlen(list);
-			if (!strcmp(list, "security.ima"))
+			if (!strcmp(list, xattr_ima))
 				ima = 1;
-			else if (!strcmp(list, "security.evm"))
+			else if (!strcmp(list, xattr_evm))
 				evm = 1;
 		}
 		if (ima && evm)
@@ -1287,8 +1290,8 @@ static int cmd_ima_fix(struct command *cmd)
 static int ima_clear(const char *path)
 {
 	log_info("%s\n", path);
-	lremovexattr(path, "security.ima");
-	lremovexattr(path, "security.evm");
+	lremovexattr(path, xattr_ima);
+	lremovexattr(path, xattr_evm);
 
 	return 0;
 }
@@ -1718,6 +1721,7 @@ static struct option opts[] = {
 	{"selinux", 1, 0, 136},
 	{"caps", 2, 0, 137},
 	{"list", 0, 0, 138},
+	{"xattr-user", 0, 0, 140},
 	{}
 
 };
@@ -1869,6 +1873,10 @@ int main(int argc, char *argv[])
 		case 138:
 			measurement_list = 1;
 			break;
+		case 140: /* --xattr-user */
+			xattr_ima = "user.ima";
+			xattr_evm = "user.evm";
+			break;
 		case '?':
 			exit(1);
 			break;
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 6fa0ed4..714f1ac 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -594,7 +594,7 @@ int ima_verify_signature(const char *file, unsigned char *sig, int siglen,
 	int hashlen, sig_hash_algo;
 
 	if (sig[0] != 0x03) {
-		log_err("security.ima has no signature\n");
+		log_err("xattr ima has no signature\n");
 		return -1;
 	}
 
-- 
2.11.0

Re: [PATCH 2/3] ima-evm-utils: Add --xattr-user option for testing

[Mimi Zohar]

Hi Vitaly,

On Mon, 2018-11-26 at 07:39 +0300, Vitaly Chikunov wrote:
> Keep ima/evm attributes in user namespace instead of security namespace.
> Would be useful for testing purposes without having root privileges,
> easier to understand, and because --sigfile does not work for evm
> signatures.

The patch looks fine, but the patch description could use some
rewriting.

The IMA/EVM attributes are currently stored in the "security"
namespace, which requires root privileges.  Storing the ima/evm
attributes in the "user" namespace, instead of the "security"
namespace, would be useful for testing purposes. 

Please expand this: "and because --sigfile does not work for evm
signatures."

Conclude with, this patch defines the "--xattr-user" option for
testing.

Mimi

[PATCH 3/3] ima-evm-utils: Allow to use Streebog hash function

[Vitaly Chikunov]

There are two methods of using GOST algorithms in OpenSSL: via config
extension and via --engine option. Both require gost-engine to be
installed.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/evmctl.c    | 27 ++++++++++++++++++++++++---
 src/imaevm.h    | 13 +++++++++++++
 src/libimaevm.c | 15 +++++++++++----
 3 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 032ea9d..5b4d8d9 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -62,6 +62,7 @@
 #include <openssl/hmac.h>
 #include <openssl/err.h>
 #include <openssl/rsa.h>
+#include <openssl/engine.h>
 
 #ifndef XATTR_APPAARMOR_SUFFIX
 #define XATTR_APPARMOR_SUFFIX "apparmor"
@@ -388,7 +389,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 
 	md = EVP_get_digestbyname(params.hash_algo);
 	if (!md) {
-		log_err("EVP_get_digestbyname() failed\n");
+		log_err("EVP_get_digestbyname(%s) failed\n", params.hash_algo);
 		return 1;
 	}
 
@@ -1056,7 +1057,7 @@ static int calc_evm_hmac(const char *file, const char *keyfile, unsigned char *h
 
 	md = EVP_get_digestbyname(params.hash_algo);
 	if (!md) {
-		log_err("EVP_get_digestbyname() failed\n");
+		log_err("EVP_get_digestbyname(%s) failed\n", params.hash_algo);
 		goto out;
 	}
 
@@ -1643,7 +1644,7 @@ static void usage(void)
 
 	printf(
 		"\n"
-		"  -a, --hashalgo     sha1 (default), sha224, sha256, sha384, sha512\n"
+		"  -a, --hashalgo     sha1 (default), sha224, sha256, sha384, sha512, streebog256, streebog512\n"
 		"  -s, --imasig       make IMA signature\n"
 		"  -d, --imahash      make IMA hash\n"
 		"  -f, --sigfile      store IMA signature in .sig file instead of xattr\n"
@@ -1669,6 +1670,7 @@ static void usage(void)
 		"      --selinux      use custom Selinux label for EVM\n"
 		"      --caps         use custom Capabilities for EVM(unspecified: from FS, empty: do not use)\n"
 		"      --list         measurement list verification\n"
+		"      --engine e     preload OpenSSL engine e\n"
 		"  -v                 increase verbosity level\n"
 		"  -h, --help         display this help and exit\n"
 		"\n");
@@ -1721,6 +1723,7 @@ static struct option opts[] = {
 	{"selinux", 1, 0, 136},
 	{"caps", 2, 0, 137},
 	{"list", 0, 0, 138},
+	{"engine", 1, 0, 139},
 	{"xattr-user", 0, 0, 140},
 	{}
 
@@ -1763,6 +1766,7 @@ static char *get_password(void)
 int main(int argc, char *argv[])
 {
 	int err = 0, c, lind;
+	ENGINE *eng = NULL;
 
 	g_argv = argv;
 	g_argc = argc;
@@ -1873,6 +1877,16 @@ int main(int argc, char *argv[])
 		case 138:
 			measurement_list = 1;
 			break;
+		case 139: /* --engine e */
+			eng = ENGINE_by_id(optarg);
+			if (!eng)
+				log_err("engine %s isn't available\n", optarg);
+			else if (!ENGINE_init(eng)) {
+				log_err("engine %s init failed\n", optarg);
+				ENGINE_free(eng);
+				eng = NULL;
+			}
+			break;
 		case 140: /* --xattr-user */
 			xattr_ima = "user.ima";
 			xattr_evm = "user.evm";
@@ -1903,6 +1917,13 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (eng) {
+		ENGINE_finish(eng);
+		ENGINE_free(eng);
+#if OPENSSL_API_COMPAT < 0x10100000L
+		ENGINE_cleanup();
+#endif
+	}
 	ERR_free_strings();
 	EVP_cleanup();
 	BIO_free(NULL);
diff --git a/src/imaevm.h b/src/imaevm.h
index 1bafaad..1a5ebbe 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -149,6 +149,7 @@ struct signature_hdr {
 	char mpi[0];
 } __packed;
 
+/* reflect enum hash_algo from include/uapi/linux/hash_info.h */
 enum pkey_hash_algo {
 	PKEY_HASH_MD4,
 	PKEY_HASH_MD5,
@@ -158,6 +159,18 @@ enum pkey_hash_algo {
 	PKEY_HASH_SHA384,
 	PKEY_HASH_SHA512,
 	PKEY_HASH_SHA224,
+	PKEY_HASH_RIPE_MD_128,
+	PKEY_HASH_RIPE_MD_256,
+	PKEY_HASH_RIPE_MD_320,
+	PKEY_HASH_WP_256,
+	PKEY_HASH_WP_384,
+	PKEY_HASH_WP_512,
+	PKEY_HASH_TGR_128,
+	PKEY_HASH_TGR_160,
+	PKEY_HASH_TGR_192,
+	PKEY_HASH_SM3_256,
+	PKEY_HASH_STREEBOG_256,
+	PKEY_HASH_STREEBOG_512,
 	PKEY_HASH__LAST
 };
 
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 714f1ac..8f74660 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -50,6 +50,7 @@
 #include <string.h>
 #include <stdio.h>
 
+#include <openssl/crypto.h>
 #include <openssl/pem.h>
 #include <openssl/evp.h>
 #include <openssl/x509.h>
@@ -66,6 +67,8 @@ 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] = "streebog256",
+	[PKEY_HASH_STREEBOG_512] = "streebog512",
 };
 
 /*
@@ -290,7 +293,7 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 
 	md = EVP_get_digestbyname(params.hash_algo);
 	if (!md) {
-		log_err("EVP_get_digestbyname() failed\n");
+		log_err("EVP_get_digestbyname(%s) failed\n", params.hash_algo);
 		return 1;
 	}
 
@@ -508,14 +511,16 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 	asn1 = &RSA_ASN1_templates[hdr->hash_algo];
 
 	if (len < asn1->size || memcmp(out, asn1->data, asn1->size)) {
-		log_err("%s: verification failed: %d\n", file, err);
+		log_err("%s: verification failed: %d (asn1 mismatch)\n",
+			file, err);
 		return -1;
 	}
 
 	len -= asn1->size;
 
 	if (len != size || memcmp(out + asn1->size, hash, len)) {
-		log_err("%s: verification failed: %d\n", file, err);
+		log_err("%s: verification failed: %d (digest mismatch)\n",
+			file, err);
 		return -1;
 	}
 
@@ -527,7 +532,8 @@ int get_hash_algo(const char *algo)
 	int i;
 
 	for (i = 0; i < PKEY_HASH__LAST; i++)
-		if (!strcmp(algo, pkey_hash_algo[i]))
+		if (pkey_hash_algo[i] &&
+		    !strcmp(algo, pkey_hash_algo[i]))
 			return i;
 
 	return PKEY_HASH_SHA1;
@@ -899,5 +905,6 @@ int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const c
 static void libinit()
 {
 	OpenSSL_add_all_algorithms();
+	OPENSSL_add_all_algorithms_conf();
 	ERR_load_crypto_strings();
 }
-- 
2.11.0

Re: [PATCH 3/3] ima-evm-utils: Allow to use Streebog hash function

[Mimi Zohar]

On Mon, 2018-11-26 at 07:39 +0300, Vitaly Chikunov wrote:
> There are two methods of using GOST algorithms in OpenSSL: via config
> extension and via --engine option. Both require gost-engine to be
> installed.

Splitting this patch based on the "--engine" option, will make it
easier to review.

> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>  src/evmctl.c    | 27 ++++++++++++++++++++++++---
>  src/imaevm.h    | 13 +++++++++++++
>  src/libimaevm.c | 15 +++++++++++----
>  3 files changed, 48 insertions(+), 7 deletions(-)

<snip>

> diff --git a/src/imaevm.h b/src/imaevm.h
> index 1bafaad..1a5ebbe 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -149,6 +149,7 @@ struct signature_hdr {
>  	char mpi[0];
>  } __packed;
> 
> +/* reflect enum hash_algo from include/uapi/linux/hash_info.h */
>  enum pkey_hash_algo {
>  	PKEY_HASH_MD4,
>  	PKEY_HASH_MD5,
> @@ -158,6 +159,18 @@ enum pkey_hash_algo {
>  	PKEY_HASH_SHA384,
>  	PKEY_HASH_SHA512,
>  	PKEY_HASH_SHA224,
> +	PKEY_HASH_RIPE_MD_128,
> +	PKEY_HASH_RIPE_MD_256,
> +	PKEY_HASH_RIPE_MD_320,
> +	PKEY_HASH_WP_256,
> +	PKEY_HASH_WP_384,
> +	PKEY_HASH_WP_512,
> +	PKEY_HASH_TGR_128,
> +	PKEY_HASH_TGR_160,
> +	PKEY_HASH_TGR_192,
> +	PKEY_HASH_SM3_256,
> +	PKEY_HASH_STREEBOG_256,
> +	PKEY_HASH_STREEBOG_512,
>  	PKEY_HASH__LAST
>  };
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 714f1ac..8f74660 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -50,6 +50,7 @@
>  #include <string.h>
>  #include <stdio.h>
> 
> +#include <openssl/crypto.h>
>  #include <openssl/pem.h>
>  #include <openssl/evp.h>
>  #include <openssl/x509.h>
> @@ -66,6 +67,8 @@ 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] = "streebog256",
> +	[PKEY_HASH_STREEBOG_512] = "streebog512",
>  };

hash_info.h is now included in kernel-headers package.

Anyone using the hash_algo enumeration defined in hash_info.h, will
probably also want to use an associated algorithm name.  It would make
more sense to keep the hash_algo enumeration, hash_algo_name[], and
perhaps the hash_digest_size[] together.  Maybe using macros to keep
them in sync (eg. kernel_read_file_id/kernel_read_file_str).

As new algorithms are added to hash_info.h, nothing would need to be
done here in ima-evm-utils, other than updating the maximum digest
size.

What do you think?

Mimi

Re: [PATCH 3/3] ima-evm-utils: Allow to use Streebog hash function

[Vitaly Chikunov]

Mimi,

On Tue, Nov 27, 2018 at 06:56:51AM -0500, Mimi Zohar wrote:
> On Mon, 2018-11-26 at 07:39 +0300, Vitaly Chikunov wrote:
> <snip>
> > diff --git a/src/imaevm.h b/src/imaevm.h
> > index 1bafaad..1a5ebbe 100644
> > --- a/src/imaevm.h
> > +++ b/src/imaevm.h
> > @@ -149,6 +149,7 @@ struct signature_hdr {
> >  	char mpi[0];
> >  } __packed;
> > 
> > +/* reflect enum hash_algo from include/uapi/linux/hash_info.h */
> >  enum pkey_hash_algo {
> >  	PKEY_HASH_MD4,
> >  	PKEY_HASH_MD5,
> > @@ -158,6 +159,18 @@ enum pkey_hash_algo {
> >  	PKEY_HASH_SHA384,
> >  	PKEY_HASH_SHA512,
> >  	PKEY_HASH_SHA224,
> > +	PKEY_HASH_RIPE_MD_128,
> > +	PKEY_HASH_RIPE_MD_256,
> > +	PKEY_HASH_RIPE_MD_320,
> > +	PKEY_HASH_WP_256,
> > +	PKEY_HASH_WP_384,
> > +	PKEY_HASH_WP_512,
> > +	PKEY_HASH_TGR_128,
> > +	PKEY_HASH_TGR_160,
> > +	PKEY_HASH_TGR_192,
> > +	PKEY_HASH_SM3_256,
> > +	PKEY_HASH_STREEBOG_256,
> > +	PKEY_HASH_STREEBOG_512,
> >  	PKEY_HASH__LAST
> >  };
> > 
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index 714f1ac..8f74660 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -50,6 +50,7 @@
> >  #include <string.h>
> >  #include <stdio.h>
> > 
> > +#include <openssl/crypto.h>
> >  #include <openssl/pem.h>
> >  #include <openssl/evp.h>
> >  #include <openssl/x509.h>
> > @@ -66,6 +67,8 @@ 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] = "streebog256",
> > +	[PKEY_HASH_STREEBOG_512] = "streebog512",
> >  };
> 
> hash_info.h is now included in kernel-headers package.

I think, first it should not be coincided with the new algo adding and
being follow-up patch (if any).

> Anyone using the hash_algo enumeration defined in hash_info.h, will
> probably also want to use an associated algorithm name.  It would make
> more sense to keep the hash_algo enumeration, hash_algo_name[], and
> perhaps the hash_digest_size[] together.  Maybe using macros to keep
> them in sync (eg. kernel_read_file_id/kernel_read_file_str).

On the first sight this sounds good, but...

It sounds like it will require patching kernel's hash_info.h, so it will
be not possible to transfer smoothly on the new scheme without breaking
some compatibility (of the newer ima-evm-utils with older kernels). Also,
it is possible that ima-evm-utils is used on the older stable box (where some
reliable and stable build system run) which does not have such modification
and/or new algorithms in the kernel but willing to generate signatures.

So, I think code duplication between projects is good in this case.

Some hash algorithms may be wished to be compatible with rsa pkcs1
signature scheme, which is also defined in kernel in
crypto/rsa-pkcs1pad.c and in ima-evm-utils in src/libimaevm.c so code
duplication and adding algorithms in both sources will happen anyway.

Thanks,


> As new algorithms are added to hash_info.h, nothing would need to be
> done here in ima-evm-utils, other than updating the maximum digest
> size.
> 
> What do you think?
> 
> Mimi

Re: [PATCH 3/3] ima-evm-utils: Allow to use Streebog hash function

[Mimi Zohar]

On Tue, 2018-11-27 at 16:08 +0300, Vitaly Chikunov wrote:

> > hash_info.h is now included in kernel-headers package.
> 
> I think, first it should not be coincided with the new algo adding and
> being follow-up patch (if any).
> 
> > Anyone using the hash_algo enumeration defined in hash_info.h, will
> > probably also want to use an associated algorithm name.  It would make
> > more sense to keep the hash_algo enumeration, hash_algo_name[], and
> > perhaps the hash_digest_size[] together.  Maybe using macros to keep
> > them in sync (eg. kernel_read_file_id/kernel_read_file_str).
> 
> On the first sight this sounds good, but...
> 
> It sounds like it will require patching kernel's hash_info.h, so it will
> be not possible to transfer smoothly on the new scheme without breaking
> some compatibility (of the newer ima-evm-utils with older kernels).  Also,
> it is possible that ima-evm-utils is used on the older stable box (where some
> reliable and stable build system run) which does not have such modification
> and/or new algorithms in the kernel but willing to generate signatures.
> 
> So, I think code duplication between projects is good in this case.

The sooner the kernel's hash_info.h is updated, the better.  For the
time being, ima-evm-utils could define these definitions in a separate
file that is/isn't included based on autotools/buildtime option.

> 
> Some hash algorithms may be wished to be compatible with rsa pkcs1
> signature scheme, which is also defined in kernel in
> crypto/rsa-pkcs1pad.c and in ima-evm-utils in src/libimaevm.c so code
> duplication and adding algorithms in both sources will happen anyway.
> 
> Thanks,

There might be multiple problems, but fixing one is better than not
fixing either.

Mimi

Re: [PATCH 1/3] ima-avm-utils: Fix hash buffer overflow in verify_evm

[Vitaly Chikunov]

Excuse me, typo in the commit name, supposed to be

  ima-evm-utils: Fix hash buffer overflow in verify_evm

of course.

On Mon, Nov 26, 2018 at 07:39:51AM +0300, Vitaly Chikunov wrote:
> Commit ae1319eeabd6 ("Remove hardcoding of SHA1 in EVM signatures")
> introduces overflow of 20 byte buffer on the stack while calculating evm
> hash. Also, invalid hash length is passed to the underlying verification
> function. This prevents any non-SHA1 hashes from being properly
> validated using evmctl.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>  src/evmctl.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 1b46d58..94d7ab1 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -55,6 +55,7 @@
>  #include <keyutils.h>
>  #include <ctype.h>
>  #include <termios.h>
> +#include <assert.h>
>  
>  #include <openssl/sha.h>
>  #include <openssl/pem.h>
> @@ -760,13 +761,15 @@ static int cmd_sign_evm(struct command *cmd)
>  
>  static int verify_evm(const char *file)
>  {
> -	unsigned char hash[20];
> +	unsigned char hash[64];
>  	unsigned char sig[1024];
> +	int mdlen;
>  	int len;
>  
> -	len = calc_evm_hash(file, hash);
> -	if (len <= 1)
> -		return len;
> +	mdlen = calc_evm_hash(file, hash);
> +	assert(mdlen <= sizeof(hash));
> +	if (mdlen <= 1)
> +		return mdlen;
>  
>  	len = lgetxattr(file, "security.evm", sig, sizeof(sig));
>  	if (len < 0) {
> @@ -779,7 +782,7 @@ static int verify_evm(const char *file)
>  		return -1;
>  	}
>  
> -	return verify_hash(file, hash, sizeof(hash), sig + 1, len - 1);
> +	return verify_hash(file, hash, mdlen, sig + 1, len - 1);
>  }
>  
>  static int cmd_verify_evm(struct command *cmd)
> -- 
> 2.11.0

Re: [PATCH 1/3] ima-avm-utils: Fix hash buffer overflow in verify_evm

[Mimi Zohar]

On Mon, 2018-11-26 at 07:39 +0300, Vitaly Chikunov wrote:
> Commit ae1319eeabd6 ("Remove hardcoding of SHA1 in EVM signatures")
> introduces overflow of 20 byte buffer on the stack while calculating evm
> hash. Also, invalid hash length is passed to the underlying verification
> function. This prevents any non-SHA1 hashes from being properly
> validated using evmctl.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

Thanks!  To prevent this sort of bug from recurring, it would be nice
if the maximum digest size would be defined once and used.

Mimi

> ---
>  src/evmctl.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 1b46d58..94d7ab1 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -55,6 +55,7 @@
>  #include <keyutils.h>
>  #include <ctype.h>
>  #include <termios.h>
> +#include <assert.h>
> 
>  #include <openssl/sha.h>
>  #include <openssl/pem.h>
> @@ -760,13 +761,15 @@ static int cmd_sign_evm(struct command *cmd)
> 
>  static int verify_evm(const char *file)
>  {
> -	unsigned char hash[20];
> +	unsigned char hash[64];
>  	unsigned char sig[1024];
> +	int mdlen;
>  	int len;
> 
> -	len = calc_evm_hash(file, hash);
> -	if (len <= 1)
> -		return len;
> +	mdlen = calc_evm_hash(file, hash);
> +	assert(mdlen <= sizeof(hash));
> +	if (mdlen <= 1)
> +		return mdlen;
> 
>  	len = lgetxattr(file, "security.evm", sig, sizeof(sig));
>  	if (len < 0) {
> @@ -779,7 +782,7 @@ static int verify_evm(const char *file)
>  		return -1;
>  	}
> 
> -	return verify_hash(file, hash, sizeof(hash), sig + 1, len - 1);
> +	return verify_hash(file, hash, mdlen, sig + 1, len - 1);
>  }
> 
>  static int cmd_verify_evm(struct command *cmd)