[ima-evm-utils PATCH 00/12] Address non concurrency-safe libimaevm global variables

[ima-evm-utils PATCH 00/12] Address non concurrency-safe libimaevm global variables

[Mimi Zohar]

The libimaevm global variables are not concurrency-safe.  Instead of
relying on global variables, define new functions with these variables
as parameters, update static functions definitions with these variables,
and deprecate existing functions.  Limit change to public keys, hash
algorithm, and key password.

To avoid library incompatability, make the existing functions wrappers
for the new function versions.

Mimi Zohar (12):
  Rename "public_keys" to "g_public_keys"
  Free public keys list
  Update library function definitions to include a "public_keys"
    parameter
  Update a library function definition to include a "hash_algo"
    parameter
  Update cmd_verify_ima() to define and use a local list of public keys
  Update cmd_verify_evm to define and use a local list of public keys
  Update ima_measurements to define and use a local list of public keys
  Define library ima_calc_hash2() function with a hash algorithm
    parameter
  Use a local hash algorithm variable when verifying file signatures
  Update EVM signature verification to use a local hash algorithm
    variable
  Use a file specific hash algorithm variable for signing files
  Define and use a file specific "keypass" variable

 src/evmctl.c    |  99 +++++++++++++++++-------------
 src/imaevm.h    |   5 ++
 src/libimaevm.c | 159 ++++++++++++++++++++++++++++++++++++------------
 3 files changed, 181 insertions(+), 82 deletions(-)

-- 
2.39.3

[ima-evm-utils PATCH 01/12] Rename "public_keys" to "g_public_keys"

[Mimi Zohar]

In preparation for replacing the library global public_keys variable,
which is not concurrency-safe, with a local variable, rename public_keys
to g_public_keys.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/libimaevm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 5b224625644e..117a1a72b60c 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -370,14 +370,14 @@ struct public_key_entry {
 	char name[9];
 	EVP_PKEY *key;
 };
-static struct public_key_entry *public_keys = NULL;
+static struct public_key_entry *g_public_keys = NULL;
 
 static EVP_PKEY *find_keyid(uint32_t keyid)
 {
-	struct public_key_entry *entry, *tail = public_keys;
+	struct public_key_entry *entry, *tail = g_public_keys;
 	int i = 1;
 
-	for (entry = public_keys; entry != NULL; entry = entry->next) {
+	for (entry = g_public_keys; entry != NULL; entry = entry->next) {
 		if (entry->keyid == keyid)
 			return entry->key;
 		i++;
@@ -394,7 +394,7 @@ static EVP_PKEY *find_keyid(uint32_t keyid)
 	if (tail)
 		tail->next = entry;
 	else
-		public_keys = entry;
+		g_public_keys = entry;
 	log_err("key %d: %x (unknown keyid)\n", i, __be32_to_cpup(&keyid));
 	return 0;
 }
@@ -429,8 +429,8 @@ void init_public_keys(const char *keyfiles)
 		calc_keyid_v2(&entry->keyid, entry->name, entry->key);
 		sprintf(entry->name, "%x", __be32_to_cpup(&entry->keyid));
 		log_info("key %d: %s %s\n", i++, entry->name, keyfile);
-		entry->next = public_keys;
-		public_keys = entry;
+		entry->next = g_public_keys;
+		g_public_keys = entry;
 	}
 	free(keyfiles_free);
 }
-- 
2.39.3

[ima-evm-utils PATCH 02/12] Free public keys list

[Mimi Zohar]

On failure to allocate memory, free the public keys list.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/imaevm.h    |  1 +
 src/libimaevm.c | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/src/imaevm.h b/src/imaevm.h
index 18d7b0e447e1..828976e52881 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -249,6 +249,7 @@ uint32_t imaevm_read_keyid(const char *certfile);
 int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);
 int verify_hash(const char *file, const unsigned char *hash, int size, unsigned char *sig, int siglen);
 int ima_verify_signature(const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
+void free_public_keys(void *public_keys);
 void init_public_keys(const char *keyfiles);
 int imaevm_hash_algo_from_sig(unsigned char *sig);
 const char *imaevm_hash_algo_by_id(int algo);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 117a1a72b60c..74e9d09b1f05 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -399,11 +399,25 @@ static EVP_PKEY *find_keyid(uint32_t keyid)
 	return 0;
 }
 
+void free_public_keys(void *public_keys)
+{
+	struct public_key_entry *entry = public_keys, *next;
+
+	while (entry != NULL) {
+		next = entry->next;
+		if (entry->key)
+			free(entry->key);
+		free(entry);
+		entry = next;
+	}
+}
+
 void init_public_keys(const char *keyfiles)
 {
 	struct public_key_entry *entry;
 	char *tmp_keyfiles, *keyfiles_free;
 	char *keyfile;
+	int err = 0;
 	int i = 1;
 
 	tmp_keyfiles = strdup(keyfiles);
@@ -417,6 +431,7 @@ void init_public_keys(const char *keyfiles)
 		entry = malloc(sizeof(struct public_key_entry));
 		if (!entry) {
 			perror("malloc");
+			err = -ENOMEM;
 			break;
 		}
 
@@ -433,6 +448,8 @@ void init_public_keys(const char *keyfiles)
 		g_public_keys = entry;
 	}
 	free(keyfiles_free);
+	if (err < 0)
+		free_public_keys(g_public_keys);
 }
 
 /*
-- 
2.39.3

[ima-evm-utils PATCH 03/12] Update library function definitions to include a "public_keys" parameter

[Mimi Zohar]

Instead of relying on a global static "public_keys" variable, which is
not concurrency-safe, update static library function definitions to
include it as a parameter, define new library functions with it as
a parameter, and deprecate existing functions.

Define init_public_keys2(), verify_hash2(), and ima_verify_signature2()
functions. Update static function defintions to include "public_keys".

To avoid library incompatablity, make the existing functions -
init_public_keys(), verify_hash(), ima_verify_signature() - wrappers
for the new function versions.

Deprecate init_public_keys(), verify_hash(), ima_verify_signature()
functions.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/imaevm.h    |  2 ++
 src/libimaevm.c | 94 +++++++++++++++++++++++++++++++++++++------------
 2 files changed, 74 insertions(+), 22 deletions(-)

diff --git a/src/imaevm.h b/src/imaevm.h
index 828976e52881..146123ba5c42 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -249,8 +249,10 @@ uint32_t imaevm_read_keyid(const char *certfile);
 int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);
 int verify_hash(const char *file, const unsigned char *hash, int size, unsigned char *sig, int siglen);
 int ima_verify_signature(const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
+int ima_verify_signature2(void *public_keys, const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
 void free_public_keys(void *public_keys);
 void init_public_keys(const char *keyfiles);
+int init_public_keys2(const char *keyfiles, void **public_keys);
 int imaevm_hash_algo_from_sig(unsigned char *sig);
 const char *imaevm_hash_algo_by_id(int algo);
 int calc_hash_sigv3(enum evm_ima_xattr_type type, const char *algo, const unsigned char *in_hash, unsigned char *out_hash);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 74e9d09b1f05..bf8c99770ddc 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -372,12 +372,12 @@ struct public_key_entry {
 };
 static struct public_key_entry *g_public_keys = NULL;
 
-static EVP_PKEY *find_keyid(uint32_t keyid)
+static EVP_PKEY *find_keyid(void *public_keys, uint32_t keyid)
 {
-	struct public_key_entry *entry, *tail = g_public_keys;
+	struct public_key_entry *entry, *tail = public_keys;
 	int i = 1;
 
-	for (entry = g_public_keys; entry != NULL; entry = entry->next) {
+	for (entry = public_keys; entry != NULL; entry = entry->next) {
 		if (entry->keyid == keyid)
 			return entry->key;
 		i++;
@@ -394,7 +394,7 @@ static EVP_PKEY *find_keyid(uint32_t keyid)
 	if (tail)
 		tail->next = entry;
 	else
-		g_public_keys = entry;
+		public_keys = (void *) entry;
 	log_err("key %d: %x (unknown keyid)\n", i, __be32_to_cpup(&keyid));
 	return 0;
 }
@@ -412,7 +412,7 @@ void free_public_keys(void *public_keys)
 	}
 }
 
-void init_public_keys(const char *keyfiles)
+int init_public_keys2(const char *keyfiles, void **public_keys)
 {
 	struct public_key_entry *entry;
 	char *tmp_keyfiles, *keyfiles_free;
@@ -420,6 +420,11 @@ void init_public_keys(const char *keyfiles)
 	int err = 0;
 	int i = 1;
 
+	if (!public_keys)
+		return -EINVAL;
+
+	*public_keys = NULL;
+
 	tmp_keyfiles = strdup(keyfiles);
 	keyfiles_free = tmp_keyfiles;
 
@@ -444,12 +449,24 @@ void init_public_keys(const char *keyfiles)
 		calc_keyid_v2(&entry->keyid, entry->name, entry->key);
 		sprintf(entry->name, "%x", __be32_to_cpup(&entry->keyid));
 		log_info("key %d: %s %s\n", i++, entry->name, keyfile);
-		entry->next = g_public_keys;
-		g_public_keys = entry;
+		entry->next = (struct public_key_entry *) *public_keys;
+		*public_keys = (void *)entry;
 	}
+
 	free(keyfiles_free);
 	if (err < 0)
-		free_public_keys(g_public_keys);
+		free_public_keys(public_keys);
+	return err;
+}
+
+/*
+ * Global static variables are not concurrency-safe.
+ *
+ * Deprecate init_public_keys() usage.
+ */
+void init_public_keys(const char *keyfiles)
+{
+	init_public_keys2(keyfiles, (void **)&g_public_keys);
 }
 
 /*
@@ -466,7 +483,8 @@ void init_public_keys(const char *keyfiles)
  *
  * (Note: signature_v2_hdr struct does not contain the 'type'.)
  */
-static int verify_hash_common(const char *file, const unsigned char *hash,
+static int verify_hash_common(void *public_keys, const char *file,
+			      const unsigned char *hash,
 			      int size, unsigned char *sig, int siglen)
 {
 	int ret = -1;
@@ -481,7 +499,7 @@ static int verify_hash_common(const char *file, const unsigned char *hash,
 		log_dump(hash, size);
 	}
 
-	pkey = find_keyid(hdr->keyid);
+	pkey = find_keyid(public_keys, hdr->keyid);
 	if (!pkey) {
 		uint32_t keyid = hdr->keyid;
 
@@ -543,11 +561,13 @@ err:
  *
  * Return: 0 verification good, 1 verification bad, -1 error.
  */
-static int verify_hash_v2(const char *file, const unsigned char *hash,
+static int verify_hash_v2(void *public_keys, const char *file,
+			  const unsigned char *hash,
 			  int size, unsigned char *sig, int siglen)
 {
 	/* note: signature_v2_hdr does not contain 'type', use sig + 1 */
-	return verify_hash_common(file, hash, size, sig + 1, siglen - 1);
+	return verify_hash_common(public_keys, file, hash, size,
+				  sig + 1, siglen - 1);
 }
 
 /*
@@ -556,7 +576,8 @@ static int verify_hash_v2(const char *file, const unsigned char *hash,
  *
  * Return: 0 verification good, 1 verification bad, -1 error.
  */
-static int verify_hash_v3(const char *file, const unsigned char *hash,
+static int verify_hash_v3(void *public_keys, const char *file,
+			  const unsigned char *hash,
 			  int size, unsigned char *sig, int siglen)
 {
 	unsigned char sigv3_hash[MAX_DIGEST_SIZE];
@@ -567,7 +588,8 @@ static int verify_hash_v3(const char *file, const unsigned char *hash,
 		return ret;
 
 	/* note: signature_v2_hdr does not contain 'type', use sig + 1 */
-	return verify_hash_common(file, sigv3_hash, size, sig + 1, siglen - 1);
+	return verify_hash_common(public_keys, file, sigv3_hash, size,
+				  sig + 1, siglen - 1);
 }
 
 #define HASH_MAX_DIGESTSIZE 64	/* kernel HASH_MAX_DIGESTSIZE is 64 bytes */
@@ -710,8 +732,9 @@ int imaevm_hash_algo_from_sig(unsigned char *sig)
 		return -1;
 }
 
-int verify_hash(const char *file, const unsigned char *hash, int size,
-		unsigned char *sig, int siglen)
+int verify_hash2(void *public_keys, const char *file,
+		 const unsigned char *hash, int size,
+		 unsigned char *sig, int siglen)
 {
 	/* Get signature type from sig header */
 	if (sig[1] == DIGSIG_VERSION_1) {
@@ -730,15 +753,29 @@ int verify_hash(const char *file, const unsigned char *hash, int size,
 		return -1;
 #endif
 	} else if (sig[1] == DIGSIG_VERSION_2) {
-		return verify_hash_v2(file, hash, size, sig, siglen);
+		return verify_hash_v2(public_keys, file, hash, size,
+				      sig, siglen);
 	} else if (sig[1] == DIGSIG_VERSION_3) {
-		return verify_hash_v3(file, hash, size, sig, siglen);
+		return verify_hash_v3(public_keys, file, hash, size,
+				      sig, siglen);
 	} else
 		return -1;
 }
 
-int ima_verify_signature(const char *file, unsigned char *sig, int siglen,
-			 unsigned char *digest, int digestlen)
+/*
+ * Global static variables are not concurrency-safe.
+ *
+ * Deprecate verify_hash() usage.
+ */
+int verify_hash(const char *file, const unsigned char *hash, int size,
+		unsigned char *sig, int siglen)
+{
+	return verify_hash2(g_public_keys, file, hash, size, sig, siglen);
+}
+
+int ima_verify_signature2(void *public_keys, const char *file,
+			  unsigned char *sig, int siglen,
+			  unsigned char *digest, int digestlen)
 {
 	unsigned char hash[MAX_DIGEST_SIZE];
 	int hashlen, sig_hash_algo;
@@ -766,14 +803,27 @@ int ima_verify_signature(const char *file, unsigned char *sig, int siglen,
 	 * measurement list, not by calculating the local file digest.
 	 */
 	if (digest && digestlen > 0)
-		return verify_hash(file, digest, digestlen, sig, siglen);
+		return verify_hash2(public_keys, file, digest, digestlen,
+				   sig, siglen);
 
 	hashlen = ima_calc_hash(file, hash);
 	if (hashlen <= 1)
 		return hashlen;
 	assert(hashlen <= sizeof(hash));
 
-	return verify_hash(file, hash, hashlen, sig, siglen);
+	return verify_hash2(public_keys, file, hash, hashlen, sig, siglen);
+}
+
+/*
+ * Global static variables are not concurrency-safe.
+ *
+ * Deprecate ima_verify_signature() usage.
+ */
+int ima_verify_signature(const char *file, unsigned char *sig, int siglen,
+			 unsigned char *digest, int digestlen)
+{
+	return ima_verify_signature2(g_public_keys, file, sig, siglen,
+				     digest, digestlen);
 }
 
 #if CONFIG_SIGV1
-- 
2.39.3

[ima-evm-utils PATCH 04/12] Update a library function definition to include a "hash_algo" parameter

[Mimi Zohar]

Instead of relying on a global "hash_algo" variable, which is not
concurrency-safe, update the verify_hash2() function definition to
include a "hash_algo" parameter as a place holder.

Export the verify_hash2() definition.

Define verify_hash2().  To avoid library incompatablity, make the existing
function verify_hash() a function wrapper for verify_hash2().

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/imaevm.h    | 1 +
 src/libimaevm.c | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/imaevm.h b/src/imaevm.h
index 146123ba5c42..1ed2c81d510d 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -248,6 +248,7 @@ uint32_t imaevm_read_keyid(const char *certfile);
 
 int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);
 int verify_hash(const char *file, const unsigned char *hash, int size, unsigned char *sig, int siglen);
+int verify_hash2(void *public_keys, const char *file, const char *hash_algo, const unsigned char *hash, int size, unsigned char *sig, int siglen);
 int ima_verify_signature(const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
 int ima_verify_signature2(void *public_keys, const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
 void free_public_keys(void *public_keys);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index bf8c99770ddc..e64d167a2a8a 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -732,7 +732,7 @@ int imaevm_hash_algo_from_sig(unsigned char *sig)
 		return -1;
 }
 
-int verify_hash2(void *public_keys, const char *file,
+int verify_hash2(void *public_keys, const char *file, const char *hash_algo,
 		 const unsigned char *hash, int size,
 		 unsigned char *sig, int siglen)
 {
@@ -770,7 +770,7 @@ int verify_hash2(void *public_keys, const char *file,
 int verify_hash(const char *file, const unsigned char *hash, int size,
 		unsigned char *sig, int siglen)
 {
-	return verify_hash2(g_public_keys, file, hash, size, sig, siglen);
+	return verify_hash2(g_public_keys, file, NULL, hash, size, sig, siglen);
 }
 
 int ima_verify_signature2(void *public_keys, const char *file,
@@ -803,7 +803,7 @@ int ima_verify_signature2(void *public_keys, const char *file,
 	 * measurement list, not by calculating the local file digest.
 	 */
 	if (digest && digestlen > 0)
-		return verify_hash2(public_keys, file, digest, digestlen,
+		return verify_hash2(public_keys, file, NULL, digest, digestlen,
 				   sig, siglen);
 
 	hashlen = ima_calc_hash(file, hash);
@@ -811,7 +811,8 @@ int ima_verify_signature2(void *public_keys, const char *file,
 		return hashlen;
 	assert(hashlen <= sizeof(hash));
 
-	return verify_hash2(public_keys, file, hash, hashlen, sig, siglen);
+	return verify_hash2(public_keys, file, NULL, hash, hashlen,
+			    sig, siglen);
 }
 
 /*
-- 
2.39.3

[ima-evm-utils PATCH 05/12] Update cmd_verify_ima() to define and use a local list of public keys

[Mimi Zohar]

Update the static verify_ima() fucntion definition to include "public_keys".

Replace calling init_public_keys() with the init_public_keys2() version.
Similarly replace ima_verify_signature() with the ima_verify_signature2()
version.

Free the local public keys list.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 4190913f0295..bf1f8f07e9ca 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -972,7 +972,7 @@ static int cmd_verify_evm(struct command *cmd)
 	return err;
 }
 
-static int verify_ima(const char *file)
+static int verify_ima(void *public_keys, const char *file)
 {
 	unsigned char sig[MAX_SIGNATURE_SIZE];
 	int len;
@@ -999,34 +999,37 @@ static int verify_ima(const char *file)
 		}
 	}
 
-	return ima_verify_signature(file, sig, len, NULL, 0);
+	return ima_verify_signature2(public_keys, file, sig, len, NULL, 0);
 }
 
 static int cmd_verify_ima(struct command *cmd)
 {
 	char *file = g_argv[optind++];
+	void *public_keys = NULL;
 	int err, fails = 0;
 
-	if (imaevm_params.x509) {
-		if (imaevm_params.keyfile) /* Support multiple public keys */
-			init_public_keys(imaevm_params.keyfile);
-		else			   /* assume read pubkey from x509 cert */
-			init_public_keys("/etc/keys/x509_evm.der");
-	}
-
 	if (!file) {
 		log_err("Parameters missing\n");
 		print_usage(cmd);
 		return -1;
 	}
 
+	if (imaevm_params.x509) {
+		if (imaevm_params.keyfile) /* Support multiple public keys */
+			init_public_keys2(imaevm_params.keyfile, &public_keys);
+		else			   /* assume read pubkey from x509 cert */
+			init_public_keys2("/etc/keys/x509_evm.der", &public_keys);
+	}
+
 	do {
-		err = verify_ima(file);
+		err = verify_ima(public_keys, file);
 		if (err)
 			fails++;
 		if (!err && imaevm_params.verbose >= LOG_INFO)
 			log_info("%s: verification is OK\n", file);
 	} while ((file = g_argv[optind++]));
+
+	free_public_keys(public_keys);
 	return fails > 0;
 }
 
-- 
2.39.3

[ima-evm-utils PATCH 06/12] Update cmd_verify_evm to define and use a local list of public keys

[Mimi Zohar]

Replace calling init_public_keys() with the init_public_keys2() version.
Similarly replace verify_hash() with the verify_hash2() version.

Update the static function verify_evm() definition to include a
"public_keys" parameter.

Free the local public keys list.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index bf1f8f07e9ca..f796edfce5f1 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -905,7 +905,7 @@ static int cmd_sign_evm(struct command *cmd)
 	return do_cmd(cmd, sign_evm_path);
 }
 
-static int verify_evm(const char *file)
+static int verify_evm(void *public_keys, const char *file)
 {
 	unsigned char hash[MAX_DIGEST_SIZE];
 	unsigned char sig[MAX_SIGNATURE_SIZE];
@@ -945,12 +945,14 @@ static int verify_evm(const char *file)
 		return mdlen;
 	assert(mdlen <= sizeof(hash));
 
-	return verify_hash(file, hash, mdlen, sig, len);
+	return verify_hash2(public_keys, file, imaevm_params.hash_algo,
+			    hash, mdlen, sig, len);
 }
 
 static int cmd_verify_evm(struct command *cmd)
 {
 	char *file = g_argv[optind++];
+	void *public_keys = NULL;
 	int err;
 
 	if (!file) {
@@ -961,14 +963,17 @@ static int cmd_verify_evm(struct command *cmd)
 
 	if (imaevm_params.x509) {
 		if (imaevm_params.keyfile) /* Support multiple public keys */
-			init_public_keys(imaevm_params.keyfile);
+			init_public_keys2(imaevm_params.keyfile, &public_keys);
 		else			   /* assume read pubkey from x509 cert */
-			init_public_keys("/etc/keys/x509_evm.der");
+			init_public_keys2("/etc/keys/x509_evm.der",
+					  &public_keys);
 	}
 
-	err = verify_evm(file);
+	err = verify_evm(public_keys, file);
 	if (!err && imaevm_params.verbose >= LOG_INFO)
 		log_info("%s: verification is OK\n", file);
+
+	free_public_keys(public_keys);
 	return err;
 }
 
-- 
2.39.3

[ima-evm-utils PATCH 07/12] Update ima_measurements to define and use a local list of public keys

[Mimi Zohar]

Replace calling init_public_keys() with the init_public_keys2() version.
Similarly replace ima_verify_signature() with the ima_verify_signature2()
version.

Update the static ima_ng_show() function definition to include a
"public_keys" parameter.

Free the local public keys list.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index f796edfce5f1..ad4565b3ee52 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1614,7 +1614,7 @@ static int lookup_template_name_entry(char *template_name)
 	return 0;
 }
 
-void ima_ng_show(struct template_entry *entry)
+static void ima_ng_show(void *public_keys, struct template_entry *entry)
 {
 	uint8_t *fieldp = entry->template;
 	uint32_t field_len;
@@ -1740,10 +1740,12 @@ void ima_ng_show(struct template_entry *entry)
 		 * the measurement list or calculate the hash.
 		 */
 		if (verify_list_sig)
-			err = ima_verify_signature(path, sig, sig_len,
-						   digest, digest_len);
+			err = ima_verify_signature2(public_keys, path,
+						    sig, sig_len,
+						    digest, digest_len);
 		else
-			err = ima_verify_signature(path, sig, sig_len, NULL, 0);
+			err = ima_verify_signature2(public_keys, path,
+						    sig, sig_len, NULL, 0);
 
 		if (!err && imaevm_params.verbose > LOG_INFO)
 			log_info("%s: verification is OK\n", path);
@@ -2223,6 +2225,7 @@ static int ima_measurement(const char *file)
 	int first_record = 1;
 	unsigned int pseudo_padded_banks_mask, pseudo_banks_mask;
 	unsigned long entry_num = 0;
+	void *public_keys = NULL;
 	int c;
 
 	struct template_entry entry = { .template = NULL };
@@ -2252,9 +2255,9 @@ static int ima_measurement(const char *file)
 	}
 
 	if (imaevm_params.keyfile)	/* Support multiple public keys */
-		init_public_keys(imaevm_params.keyfile);
+		init_public_keys2(imaevm_params.keyfile, &public_keys);
 	else				/* assume read pubkey from x509 cert */
-		init_public_keys("/etc/keys/x509_evm.der");
+		init_public_keys2("/etc/keys/x509_evm.der", &public_keys);
 	if (errno)
 		log_errno_reset(LOG_DEBUG,
 				"Failure in initializing public keys");
@@ -2405,7 +2408,7 @@ static int ima_measurement(const char *file)
 		if (is_ima_template)
 			ima_show(&entry);
 		else
-			ima_ng_show(&entry);
+			ima_ng_show(public_keys, &entry);
 
 		if (!tpmbanks)
 			continue;
@@ -2464,6 +2467,7 @@ out_free:
 	free(pseudo_banks);
 	free(pseudo_padded_banks);
 	free(entry.template);
+	free_public_keys(public_keys);
 
 	return err;
 }
-- 
2.39.3

[ima-evm-utils PATCH 08/12] Define library ima_calc_hash2() function with a hash algorithm parameter

[Mimi Zohar]

Instead of relying on the "imaevm_params.algo" global variable, which
is not concurrency-safe, define a new library ima_calc_hash2() function
with the hash algorithm as a parameter.

To avoid library incompatablity, make the existing ima_calc_hash()
function a wrapper for ima_calc_hash2().

Deprecate ima_calc_hash().

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/imaevm.h    |  1 +
 src/libimaevm.c | 12 ++++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/imaevm.h b/src/imaevm.h
index 1ed2c81d510d..81acd8df41cb 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -237,6 +237,7 @@ extern struct libimaevm_params imaevm_params;
 void imaevm_do_hexdump(FILE *fp, const void *ptr, int len, bool cr);
 void imaevm_hexdump(const void *ptr, int len);
 int ima_calc_hash(const char *file, uint8_t *hash);
+int ima_calc_hash2(const char *file, const char *hash_algo, uint8_t *hash);
 int imaevm_get_hash_algo(const char *algo);
 RSA *read_pub_key(const char *keyfile, int x509);
 EVP_PKEY *read_pub_pkey(const char *keyfile, int x509);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index e64d167a2a8a..4c9da7a2f06b 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -181,7 +181,7 @@ out:
 	return err;
 }
 
-int ima_calc_hash(const char *file, uint8_t *hash)
+int ima_calc_hash2(const char *file, const char *hash_algo, uint8_t *hash)
 {
 	const EVP_MD *md;
 	struct stat st;
@@ -202,10 +202,9 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 		goto err;
 	}
 
-	md = EVP_get_digestbyname(imaevm_params.hash_algo);
+	md = EVP_get_digestbyname(hash_algo);
 	if (!md) {
-		log_err("EVP_get_digestbyname(%s) failed\n",
-			imaevm_params.hash_algo);
+		log_err("EVP_get_digestbyname(%s) failed\n", hash_algo);
 		err = 1;
 		goto err;
 	}
@@ -246,6 +245,11 @@ err:
 	return err;
 }
 
+int ima_calc_hash(const char *file, uint8_t *hash)
+{
+	return ima_calc_hash2(file, imaevm_params.hash_algo, hash);
+}
+
 EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 {
 	FILE *fp;
-- 
2.39.3

[ima-evm-utils PATCH 09/12] Use a local hash algorithm variable when verifying file signatures

[Mimi Zohar]

Instead of relying on the "imaevm_params.algo" global variable, which
is not concurrency-safe, define and use a local variable.

Update static verify_hash_v2(), verify_hash_v3(), and verify_hash_common()
function definitions to include a hash algorithm argument.

Similarly update ima_verify_signature2() and ima_calc_hash2() to define
and use a local hash algorithm variable.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/libimaevm.c | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 4c9da7a2f06b..18b6a6f27237 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -488,6 +488,7 @@ void init_public_keys(const char *keyfiles)
  * (Note: signature_v2_hdr struct does not contain the 'type'.)
  */
 static int verify_hash_common(void *public_keys, const char *file,
+			      const char *hash_algo,
 			      const unsigned char *hash,
 			      int size, unsigned char *sig, int siglen)
 {
@@ -499,7 +500,7 @@ static int verify_hash_common(void *public_keys, const char *file,
 	const char *st;
 
 	if (imaevm_params.verbose > LOG_INFO) {
-		log_info("hash(%s): ", imaevm_params.hash_algo);
+		log_info("hash(%s): ", hash_algo);
 		log_dump(hash, size);
 	}
 
@@ -530,7 +531,7 @@ static int verify_hash_common(void *public_keys, const char *file,
 	if (!EVP_PKEY_verify_init(ctx))
 		goto err;
 	st = "EVP_get_digestbyname";
-	if (!(md = EVP_get_digestbyname(imaevm_params.hash_algo)))
+	if (!(md = EVP_get_digestbyname(hash_algo)))
 		goto err;
 	st = "EVP_PKEY_CTX_set_signature_md";
 	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
@@ -566,11 +567,12 @@ err:
  * Return: 0 verification good, 1 verification bad, -1 error.
  */
 static int verify_hash_v2(void *public_keys, const char *file,
+			  const char *hash_algo,
 			  const unsigned char *hash,
 			  int size, unsigned char *sig, int siglen)
 {
 	/* note: signature_v2_hdr does not contain 'type', use sig + 1 */
-	return verify_hash_common(public_keys, file, hash, size,
+	return verify_hash_common(public_keys, file, hash_algo, hash, size,
 				  sig + 1, siglen - 1);
 }
 
@@ -581,19 +583,20 @@ static int verify_hash_v2(void *public_keys, const char *file,
  * Return: 0 verification good, 1 verification bad, -1 error.
  */
 static int verify_hash_v3(void *public_keys, const char *file,
+			  const char *hash_algo,
 			  const unsigned char *hash,
 			  int size, unsigned char *sig, int siglen)
 {
 	unsigned char sigv3_hash[MAX_DIGEST_SIZE];
 	int ret;
 
-	ret = calc_hash_sigv3(sig[0], NULL, hash, sigv3_hash);
+	ret = calc_hash_sigv3(sig[0], hash_algo, hash, sigv3_hash);
 	if (ret < 0)
 		return ret;
 
 	/* note: signature_v2_hdr does not contain 'type', use sig + 1 */
-	return verify_hash_common(public_keys, file, sigv3_hash, size,
-				  sig + 1, siglen - 1);
+	return verify_hash_common(public_keys, file, hash_algo, sigv3_hash,
+				  size, sig + 1, siglen - 1);
 }
 
 #define HASH_MAX_DIGESTSIZE 64	/* kernel HASH_MAX_DIGESTSIZE is 64 bytes */
@@ -636,8 +639,10 @@ int calc_hash_sigv3(enum evm_ima_xattr_type type, const char *algo,
 		return -EINVAL;
 	}
 
-	if (!algo)
-		algo = imaevm_params.hash_algo;
+	if (!algo) {
+		log_err("Hash algorithm unspecified\n");
+		return -EINVAL;
+	}
 
 	if ((hash_algo = imaevm_get_hash_algo(algo)) < 0) {
 		log_err("Hash algorithm %s not supported\n", algo);
@@ -757,10 +762,10 @@ int verify_hash2(void *public_keys, const char *file, const char *hash_algo,
 		return -1;
 #endif
 	} else if (sig[1] == DIGSIG_VERSION_2) {
-		return verify_hash_v2(public_keys, file, hash, size,
+		return verify_hash_v2(public_keys, file, hash_algo, hash, size,
 				      sig, siglen);
 	} else if (sig[1] == DIGSIG_VERSION_3) {
-		return verify_hash_v3(public_keys, file, hash, size,
+		return verify_hash_v3(public_keys, file, hash_algo, hash, size,
 				      sig, siglen);
 	} else
 		return -1;
@@ -774,7 +779,8 @@ int verify_hash2(void *public_keys, const char *file, const char *hash_algo,
 int verify_hash(const char *file, const unsigned char *hash, int size,
 		unsigned char *sig, int siglen)
 {
-	return verify_hash2(g_public_keys, file, NULL, hash, size, sig, siglen);
+	return verify_hash2(g_public_keys, file, imaevm_params.hash_algo,
+			    hash, size, sig, siglen);
 }
 
 int ima_verify_signature2(void *public_keys, const char *file,
@@ -783,6 +789,7 @@ int ima_verify_signature2(void *public_keys, const char *file,
 {
 	unsigned char hash[MAX_DIGEST_SIZE];
 	int hashlen, sig_hash_algo;
+	const char *hash_algo;
 
 	if (sig[0] != EVM_IMA_XATTR_DIGSIG && sig[0] != IMA_VERITY_DIGSIG) {
 		log_err("%s: xattr ima has no signature\n", file);
@@ -800,22 +807,23 @@ int ima_verify_signature2(void *public_keys, const char *file,
 		return -1;
 	}
 	/* Use hash algorithm as retrieved from signature */
-	imaevm_params.hash_algo = imaevm_hash_algo_by_id(sig_hash_algo);
+	hash_algo = imaevm_hash_algo_by_id(sig_hash_algo);
 
 	/*
 	 * Validate the signature based on the digest included in the
 	 * measurement list, not by calculating the local file digest.
 	 */
 	if (digest && digestlen > 0)
-		return verify_hash2(public_keys, file, NULL, digest, digestlen,
-				   sig, siglen);
+		return verify_hash2(public_keys, file,
+				    hash_algo, digest, digestlen,
+				    sig, siglen);
 
-	hashlen = ima_calc_hash(file, hash);
+	hashlen = ima_calc_hash2(file, hash_algo, hash);
 	if (hashlen <= 1)
 		return hashlen;
 	assert(hashlen <= sizeof(hash));
 
-	return verify_hash2(public_keys, file, NULL, hash, hashlen,
+	return verify_hash2(public_keys, file, hash_algo, hash, hashlen,
 			    sig, siglen);
 }
 
-- 
2.39.3

[ima-evm-utils PATCH 10/12] Update EVM signature verification to use a local hash algorithm variable

[Mimi Zohar]

Instead of relying on the "imaevm_params.algo" global variable, which
is not concurrency-safe, define and use a local file hash algorithm
variable.

Update calc_evm_hash(), verify_hash2().

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index ad4565b3ee52..7ae897d8b8b3 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -340,7 +340,8 @@ err:
  * Returns 0 for EVP_ function failures. Return -1 for other failures.
  * Return hash algorithm size on success.
  */
-static int calc_evm_hash(const char *file, unsigned char *hash)
+static int calc_evm_hash(const char *file, const char *hash_algo,
+			 unsigned char *hash)
 {
         const EVP_MD *md;
 	struct stat st;
@@ -408,10 +409,9 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 	}
 #endif
 
-	md = EVP_get_digestbyname(imaevm_params.hash_algo);
+	md = EVP_get_digestbyname(hash_algo);
 	if (!md) {
-		log_err("EVP_get_digestbyname(%s) failed\n",
-			imaevm_params.hash_algo);
+		log_err("EVP_get_digestbyname(%s) failed\n", hash_algo);
 		err = 0;
 		goto out;
 	}
@@ -570,7 +570,7 @@ static int sign_evm(const char *file, const char *key)
 	unsigned char sig[MAX_SIGNATURE_SIZE];
 	int len, err;
 
-	len = calc_evm_hash(file, hash);
+	len = calc_evm_hash(file, imaevm_params.hash_algo, hash);
 	if (len <= 1)
 		return len;
 	assert(len <= sizeof(hash));
@@ -909,6 +909,7 @@ static int verify_evm(void *public_keys, const char *file)
 {
 	unsigned char hash[MAX_DIGEST_SIZE];
 	unsigned char sig[MAX_SIGNATURE_SIZE];
+	const char *hash_algo = NULL;
 	int sig_hash_algo;
 	int mdlen;
 	int len;
@@ -938,15 +939,15 @@ static int verify_evm(void *public_keys, const char *file)
 		log_err("unknown hash algo: %s\n", file);
 		return -1;
 	}
-	imaevm_params.hash_algo = imaevm_hash_algo_by_id(sig_hash_algo);
+	hash_algo = imaevm_hash_algo_by_id(sig_hash_algo);
 
-	mdlen = calc_evm_hash(file, hash);
+	mdlen = calc_evm_hash(file, hash_algo, hash);
 	if (mdlen <= 1)
 		return mdlen;
 	assert(mdlen <= sizeof(hash));
 
-	return verify_hash2(public_keys, file, imaevm_params.hash_algo,
-			    hash, mdlen, sig, len);
+	return verify_hash2(public_keys, file, hash_algo, hash,
+			    mdlen, sig, len);
 }
 
 static int cmd_verify_evm(struct command *cmd)
-- 
2.39.3

[ima-evm-utils PATCH 11/12] Use a file specific hash algorithm variable for signing files

[Mimi Zohar]

Instead of relying on the library "imaevm_params.algo" global variable,
which is not concurrency-safe, define and use an evmctl file specific
hash algorithm variable.

Propogate using the file specific hash algorithm variable in sign_evm(),
sign_ima(), hash_ima(), and sign_hash() function.

Replace using the library function ima_calc_hash() with ima_calc_hash2().

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 7ae897d8b8b3..b802eeb1bf15 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -140,6 +140,7 @@ static bool evm_immutable;
 static bool evm_portable;
 static bool veritysig;
 static bool hwtpm;
+static char *hash_algo;
 
 #define HMAC_FLAG_NO_UUID	0x0001
 #define HMAC_FLAG_CAPS_SET	0x0002
@@ -570,12 +571,12 @@ static int sign_evm(const char *file, const char *key)
 	unsigned char sig[MAX_SIGNATURE_SIZE];
 	int len, err;
 
-	len = calc_evm_hash(file, imaevm_params.hash_algo, hash);
+	len = calc_evm_hash(file, hash_algo, hash);
 	if (len <= 1)
 		return len;
 	assert(len <= sizeof(hash));
 
-	len = sign_hash(imaevm_params.hash_algo, hash, len, key, NULL, sig + 1);
+	len = sign_hash(hash_algo, hash, len, key, NULL, sig + 1);
 	if (len <= 1)
 		return len;
 	assert(len < sizeof(sig));
@@ -609,10 +610,10 @@ static int hash_ima(const char *file)
 {
 	unsigned char hash[MAX_DIGEST_SIZE + 2]; /* +2 byte xattr header */
 	int len, err, offset;
-	int algo = imaevm_get_hash_algo(imaevm_params.hash_algo);
+	int algo = imaevm_get_hash_algo(hash_algo);
 
 	if (algo < 0) {
-		log_err("Unknown hash algo: %s\n", imaevm_params.hash_algo);
+		log_err("Unknown hash algo: %s\n", hash_algo);
 		return -1;
 	}
 	if (algo > PKEY_HASH_SHA1) {
@@ -624,7 +625,7 @@ static int hash_ima(const char *file)
 		offset = 1;
 	}
 
-	len = ima_calc_hash(file, hash + offset);
+	len = ima_calc_hash2(file, hash_algo, hash + offset);
 	if (len <= 1)
 		return len;
 	assert(len + offset <= sizeof(hash));
@@ -632,7 +633,7 @@ static int hash_ima(const char *file)
 	len += offset;
 
 	if (imaevm_params.verbose >= LOG_INFO)
-		log_info("hash(%s): ", imaevm_params.hash_algo);
+		log_info("hash(%s): ", hash_algo);
 
 	if (sigdump || imaevm_params.verbose >= LOG_INFO)
 		imaevm_hexdump(hash, len);
@@ -656,12 +657,12 @@ static int sign_ima(const char *file, const char *key)
 	unsigned char sig[MAX_SIGNATURE_SIZE];
 	int len, err;
 
-	len = ima_calc_hash(file, hash);
+	len = ima_calc_hash2(file, hash_algo, hash);
 	if (len <= 1)
 		return len;
 	assert(len <= sizeof(hash));
 
-	len = sign_hash(imaevm_params.hash_algo, hash, len, key, NULL, sig + 1);
+	len = sign_hash(hash_algo, hash, len, key, NULL, sig + 1);
 	if (len <= 1)
 		return len;
 	assert(len < sizeof(sig));
@@ -854,7 +855,7 @@ static int cmd_sign_hash(struct command *cmd)
 			assert(hashlen / 2 <= sizeof(hash));
 			hex2bin(hash, line, hashlen / 2);
 
-			siglen = sign_hash(imaevm_params.hash_algo, hash,
+			siglen = sign_hash(hash_algo, hash,
 					   hashlen / 2, key, NULL, sig + 1);
 			sig[0] = EVM_IMA_XATTR_DIGSIG;
 		}
@@ -3077,7 +3078,7 @@ int main(int argc, char *argv[])
 			sigdump = 1;
 			break;
 		case 'a':
-			imaevm_params.hash_algo = optarg;
+			hash_algo = optarg;
 			break;
 		case 'p':
 			if (optarg)
-- 
2.39.3

[ima-evm-utils PATCH 12/12] Define and use a file specific "keypass" variable

[Mimi Zohar]

Instead of relying on the "imaevm_params.keypass" global variable, which
is not concurrency-safe, add keypass as a parameter to the static library
functions definitions.  Update function callers.

To avoid library incompatablity, don't remove imaevm_params.keypass
variable.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/evmctl.c    |  9 +++++----
 src/libimaevm.c | 17 ++++++++---------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index b802eeb1bf15..6d6160159a1f 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -141,6 +141,7 @@ static bool evm_portable;
 static bool veritysig;
 static bool hwtpm;
 static char *hash_algo;
+static char *keypass;
 
 #define HMAC_FLAG_NO_UUID	0x0001
 #define HMAC_FLAG_CAPS_SET	0x0002
@@ -3082,9 +3083,9 @@ int main(int argc, char *argv[])
 			break;
 		case 'p':
 			if (optarg)
-				imaevm_params.keypass = optarg;
+				keypass = optarg;
 			else
-				imaevm_params.keypass = get_password();
+				keypass = get_password();
 			break;
 		case 'f':
 			sigfile = 1;
@@ -3226,8 +3227,8 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if (!imaevm_params.keypass)
-		imaevm_params.keypass = getenv("EVMCTL_KEY_PASSWORD");
+	if (!keypass)
+		keypass = getenv("EVMCTL_KEY_PASSWORD");
 
 	if (imaevm_params.keyfile != NULL &&
 	    imaevm_params.eng == NULL &&
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 18b6a6f27237..10ec847da08a 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -1124,7 +1124,8 @@ static int get_hash_algo_v1(const char *algo)
 }
 
 static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
-			int size, const char *keyfile, unsigned char *sig)
+			int size, const char *keyfile, const char *keypass,
+			unsigned char *sig)
 {
 	int len = -1, hashalgo_idx;
 	SHA_CTX ctx;
@@ -1158,7 +1159,7 @@ static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
 	log_info("hash(%s): ", hashalgo);
 	log_dump(hash, size);
 
-	key = read_priv_key(keyfile, imaevm_params.keypass);
+	key = read_priv_key(keyfile, keypass);
 	if (!key)
 		return -1;
 
@@ -1211,7 +1212,8 @@ out:
  * Return: -1 signing error, >0 length of signature
  */
 static int sign_hash_v2(const char *algo, const unsigned char *hash,
-			int size, const char *keyfile, unsigned char *sig)
+			int size, const char *keyfile, const char *keypass,
+			unsigned char *sig)
 {
 	struct signature_v2_hdr *hdr;
 	int len = -1;
@@ -1246,7 +1248,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 	log_info("hash(%s): ", algo);
 	log_dump(hash, size);
 
-	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
+	pkey = read_priv_pkey(keyfile, keypass);
 	if (!pkey)
 		return -1;
 
@@ -1316,14 +1318,11 @@ err:
 
 int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
 {
-	if (keypass)
-		imaevm_params.keypass = keypass;
-
 	if (imaevm_params.x509)
-		return sign_hash_v2(hashalgo, hash, size, keyfile, sig);
+		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig);
 #if CONFIG_SIGV1
 	else
-		return sign_hash_v1(hashalgo, hash, size, keyfile, sig);
+		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig);
 #endif
 	log_info("Signature version 1 deprecated.");
 	return -1;
-- 
2.39.3

Re: [ima-evm-utils PATCH 01/12] Rename "public_keys" to "g_public_keys"

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> In preparation for replacing the library global public_keys variable,
> which is not concurrency-safe, with a local variable, rename public_keys
> to g_public_keys.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   src/libimaevm.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 5b224625644e..117a1a72b60c 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -370,14 +370,14 @@ struct public_key_entry {
>   	char name[9];
>   	EVP_PKEY *key;
>   };
> -static struct public_key_entry *public_keys = NULL;
> +static struct public_key_entry *g_public_keys = NULL;
>   
>   static EVP_PKEY *find_keyid(uint32_t keyid)
>   {
> -	struct public_key_entry *entry, *tail = public_keys;
> +	struct public_key_entry *entry, *tail = g_public_keys;
>   	int i = 1;
>   
> -	for (entry = public_keys; entry != NULL; entry = entry->next) {
> +	for (entry = g_public_keys; entry != NULL; entry = entry->next) {
>   		if (entry->keyid == keyid)
>   			return entry->key;
>   		i++;
> @@ -394,7 +394,7 @@ static EVP_PKEY *find_keyid(uint32_t keyid)
>   	if (tail)
>   		tail->next = entry;
>   	else
> -		public_keys = entry;
> +		g_public_keys = entry;
>   	log_err("key %d: %x (unknown keyid)\n", i, __be32_to_cpup(&keyid));
>   	return 0;
>   }
> @@ -429,8 +429,8 @@ void init_public_keys(const char *keyfiles)
>   		calc_keyid_v2(&entry->keyid, entry->name, entry->key);
>   		sprintf(entry->name, "%x", __be32_to_cpup(&entry->keyid));
>   		log_info("key %d: %s %s\n", i++, entry->name, keyfile);
> -		entry->next = public_keys;
> -		public_keys = entry;
> +		entry->next = g_public_keys;
> +		g_public_keys = entry;
>   	}
>   	free(keyfiles_free);
>   }

Re: [ima-evm-utils PATCH 02/12] Free public keys list

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> On failure to allocate memory, free the public keys list.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/imaevm.h    |  1 +
>   src/libimaevm.c | 17 +++++++++++++++++
>   2 files changed, 18 insertions(+)
> 
> diff --git a/src/imaevm.h b/src/imaevm.h
> index 18d7b0e447e1..828976e52881 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -249,6 +249,7 @@ uint32_t imaevm_read_keyid(const char *certfile);
>   int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);
>   int verify_hash(const char *file, const unsigned char *hash, int size, unsigned char *sig, int siglen);
>   int ima_verify_signature(const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
> +void free_public_keys(void *public_keys);
>   void init_public_keys(const char *keyfiles);
>   int imaevm_hash_algo_from_sig(unsigned char *sig);
>   const char *imaevm_hash_algo_by_id(int algo);
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 117a1a72b60c..74e9d09b1f05 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -399,11 +399,25 @@ static EVP_PKEY *find_keyid(uint32_t keyid)
>   	return 0;
>   }
>   
> +void free_public_keys(void *public_keys)
> +{
> +	struct public_key_entry *entry = public_keys, *next;
> +
> +	while (entry != NULL) {
> +		next = entry->next;
> +		if (entry->key)
> +			free(entry->key);
> +		free(entry);
> +		entry = next;
> +	}
> +}
> +
>   void init_public_keys(const char *keyfiles)
>   {
>   	struct public_key_entry *entry;
>   	char *tmp_keyfiles, *keyfiles_free;
>   	char *keyfile;
> +	int err = 0;
>   	int i = 1;
>   
>   	tmp_keyfiles = strdup(keyfiles);
> @@ -417,6 +431,7 @@ void init_public_keys(const char *keyfiles)
>   		entry = malloc(sizeof(struct public_key_entry));
>   		if (!entry) {
>   			perror("malloc");
> +			err = -ENOMEM;
>   			break;
>   		}
>   
> @@ -433,6 +448,8 @@ void init_public_keys(const char *keyfiles)
>   		g_public_keys = entry;
>   	}
>   	free(keyfiles_free);
> +	if (err < 0)
> +		free_public_keys(g_public_keys);
>   }
>   
>   /*

Unfortunately you cannot return the err to callers...

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [ima-evm-utils PATCH 03/12] Update library function definitions to include a "public_keys" parameter

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Instead of relying on a global static "public_keys" variable, which is
> not concurrency-safe, update static library function definitions to
> include it as a parameter, define new library functions with it as
> a parameter, and deprecate existing functions.
> 
> Define init_public_keys2(), verify_hash2(), and ima_verify_signature2()
> functions. Update static function defintions to include "public_keys".

-> definitions

> 
> To avoid library incompatablity, make the existing functions -

-> incompatibility

> init_public_keys(), verify_hash(), ima_verify_signature() - wrappers
> for the new function versions.
> 
> Deprecate init_public_keys(), verify_hash(), ima_verify_signature()
> functions.

It may be worth considering adding __attribute__((deprecated)) to those 
functions.

> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/imaevm.h    |  2 ++
>   src/libimaevm.c | 94 +++++++++++++++++++++++++++++++++++++------------
>   2 files changed, 74 insertions(+), 22 deletions(-)
> 
> diff --git a/src/imaevm.h b/src/imaevm.h
> index 828976e52881..146123ba5c42 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -249,8 +249,10 @@ uint32_t imaevm_read_keyid(const char *certfile);
>   int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);
>   int verify_hash(const char *file, const unsigned char *hash, int size, unsigned char *sig, int siglen);
>   int ima_verify_signature(const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
> +int ima_verify_signature2(void *public_keys, const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
>   void free_public_keys(void *public_keys);
>   void init_public_keys(const char *keyfiles);
> +int init_public_keys2(const char *keyfiles, void **public_keys);
>   int imaevm_hash_algo_from_sig(unsigned char *sig);
>   const char *imaevm_hash_algo_by_id(int algo);
>   int calc_hash_sigv3(enum evm_ima_xattr_type type, const char *algo, const unsigned char *in_hash, unsigned char *out_hash);
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 74e9d09b1f05..bf8c99770ddc 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -372,12 +372,12 @@ struct public_key_entry {
>   };
>   static struct public_key_entry *g_public_keys = NULL;
>   
> -static EVP_PKEY *find_keyid(uint32_t keyid)
> +static EVP_PKEY *find_keyid(void *public_keys, uint32_t keyid)
>   {
> -	struct public_key_entry *entry, *tail = g_public_keys;
> +	struct public_key_entry *entry, *tail = public_keys;
>   	int i = 1;
>   
> -	for (entry = g_public_keys; entry != NULL; entry = entry->next) {
> +	for (entry = public_keys; entry != NULL; entry = entry->next) {
>   		if (entry->keyid == keyid)
>   			return entry->key;
>   		i++;
> @@ -394,7 +394,7 @@ static EVP_PKEY *find_keyid(uint32_t keyid)
>   	if (tail)
>   		tail->next = entry;
>   	else
> -		g_public_keys = entry;
> +		public_keys = (void *) entry;
>   	log_err("key %d: %x (unknown keyid)\n", i, __be32_to_cpup(&keyid));
>   	return 0;
>   }
> @@ -412,7 +412,7 @@ void free_public_keys(void *public_keys)
>   	}
>   }
>   
> -void init_public_keys(const char *keyfiles)
> +int init_public_keys2(const char *keyfiles, void **public_keys)

Even though callers of the public APIs may get void *, inside the 
library this can be passed around with its concrete type.

Re: [ima-evm-utils PATCH 04/12] Update a library function definition to include a "hash_algo" parameter

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Instead of relying on a global "hash_algo" variable, which is not
> concurrency-safe, update the verify_hash2() function definition to
> include a "hash_algo" parameter as a place holder.
> 
> Export the verify_hash2() definition.
> 
> Define verify_hash2().  To avoid library incompatablity, make the existing

-> incompatibility

> function verify_hash() a function wrapper for verify_hash2().
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/imaevm.h    | 1 +
>   src/libimaevm.c | 9 +++++----
>   2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/imaevm.h b/src/imaevm.h
> index 146123ba5c42..1ed2c81d510d 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -248,6 +248,7 @@ uint32_t imaevm_read_keyid(const char *certfile);
>   
>   int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);
>   int verify_hash(const char *file, const unsigned char *hash, int size, unsigned char *sig, int siglen);
> +int verify_hash2(void *public_keys, const char *file, const char *hash_algo, const unsigned char *hash, int size, unsigned char *sig, int siglen);
>   int ima_verify_signature(const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
>   int ima_verify_signature2(void *public_keys, const char *file, unsigned char *sig, int siglen, unsigned char *digest, int digestlen);
>   void free_public_keys(void *public_keys);
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index bf8c99770ddc..e64d167a2a8a 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -732,7 +732,7 @@ int imaevm_hash_algo_from_sig(unsigned char *sig)
>   		return -1;
>   }
>   
> -int verify_hash2(void *public_keys, const char *file,
> +int verify_hash2(void *public_keys, const char *file, const char *hash_algo,
>   		 const unsigned char *hash, int size,
>   		 unsigned char *sig, int siglen)
>   {
> @@ -770,7 +770,7 @@ int verify_hash2(void *public_keys, const char *file,
>   int verify_hash(const char *file, const unsigned char *hash, int size,
>   		unsigned char *sig, int siglen)
>   {
> -	return verify_hash2(g_public_keys, file, hash, size, sig, siglen);
> +	return verify_hash2(g_public_keys, file, NULL, hash, size, sig, siglen);
>   }
>   
>   int ima_verify_signature2(void *public_keys, const char *file,
> @@ -803,7 +803,7 @@ int ima_verify_signature2(void *public_keys, const char *file,
>   	 * measurement list, not by calculating the local file digest.
>   	 */
>   	if (digest && digestlen > 0)
> -		return verify_hash2(public_keys, file, digest, digestlen,
> +		return verify_hash2(public_keys, file, NULL, digest, digestlen,
>   				   sig, siglen);
>   
>   	hashlen = ima_calc_hash(file, hash);
> @@ -811,7 +811,8 @@ int ima_verify_signature2(void *public_keys, const char *file,
>   		return hashlen;
>   	assert(hashlen <= sizeof(hash));
>   
> -	return verify_hash2(public_keys, file, hash, hashlen, sig, siglen);
> +	return verify_hash2(public_keys, file, NULL, hash, hashlen,
> +			    sig, siglen);
>   }
>   
>   /*

At this points verify_hash2 becomes a public function (by adding it to 
imaevm.h) and it gets an additional parameter but the parameter is not 
used at all...

Re: [ima-evm-utils PATCH 05/12] Update cmd_verify_ima() to define and use a local list of public keys

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Update the static verify_ima() fucntion definition to include "public_keys".
> 
-> function

> Replace calling init_public_keys() with the init_public_keys2() version.
> Similarly replace ima_verify_signature() with the ima_verify_signature2()
> version.
> 
> Free the local public keys list.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 4190913f0295..bf1f8f07e9ca 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -972,7 +972,7 @@ static int cmd_verify_evm(struct command *cmd)
>   	return err;
>   }
>   
> -static int verify_ima(const char *file)
> +static int verify_ima(void *public_keys, const char *file)
>   {
>   	unsigned char sig[MAX_SIGNATURE_SIZE];
>   	int len;
> @@ -999,34 +999,37 @@ static int verify_ima(const char *file)
>   		}
>   	}
>   
> -	return ima_verify_signature(file, sig, len, NULL, 0);
> +	return ima_verify_signature2(public_keys, file, sig, len, NULL, 0);
>   }
>   
>   static int cmd_verify_ima(struct command *cmd)
>   {
>   	char *file = g_argv[optind++];
> +	void *public_keys = NULL;
>   	int err, fails = 0;
>   
> -	if (imaevm_params.x509) {
> -		if (imaevm_params.keyfile) /* Support multiple public keys */
> -			init_public_keys(imaevm_params.keyfile);
> -		else			   /* assume read pubkey from x509 cert */
> -			init_public_keys("/etc/keys/x509_evm.der");
> -	}
> -
>   	if (!file) {
>   		log_err("Parameters missing\n");
>   		print_usage(cmd);
>   		return -1;
>   	}
>   
> +	if (imaevm_params.x509) {
> +		if (imaevm_params.keyfile) /* Support multiple public keys */
> +			init_public_keys2(imaevm_params.keyfile, &public_keys);
> +		else			   /* assume read pubkey from x509 cert */
> +			init_public_keys2("/etc/keys/x509_evm.der", &public_keys);

You should probably check the return code of this function.

> +	}
> +
>   	do {
> -		err = verify_ima(file);
> +		err = verify_ima(public_keys, file);
>   		if (err)
>   			fails++;
>   		if (!err && imaevm_params.verbose >= LOG_INFO)
>   			log_info("%s: verification is OK\n", file);
>   	} while ((file = g_argv[optind++]));
> +
> +	free_public_keys(public_keys);
>   	return fails > 0;
>   }
>   

Re: [ima-evm-utils PATCH 07/12] Update ima_measurements to define and use a local list of public keys

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Replace calling init_public_keys() with the init_public_keys2() version.
> Similarly replace ima_verify_signature() with the ima_verify_signature2()
> version.
> 
> Update the static ima_ng_show() function definition to include a
> "public_keys" parameter.
> 
> Free the local public keys list.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index f796edfce5f1..ad4565b3ee52 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -1614,7 +1614,7 @@ static int lookup_template_name_entry(char *template_name)
>   	return 0;
>   }
>   
> -void ima_ng_show(struct template_entry *entry)
> +static void ima_ng_show(void *public_keys, struct template_entry *entry)
>   {
>   	uint8_t *fieldp = entry->template;
>   	uint32_t field_len;
> @@ -1740,10 +1740,12 @@ void ima_ng_show(struct template_entry *entry)
>   		 * the measurement list or calculate the hash.
>   		 */
>   		if (verify_list_sig)
> -			err = ima_verify_signature(path, sig, sig_len,
> -						   digest, digest_len);
> +			err = ima_verify_signature2(public_keys, path,
> +						    sig, sig_len,
> +						    digest, digest_len);
>   		else
> -			err = ima_verify_signature(path, sig, sig_len, NULL, 0);
> +			err = ima_verify_signature2(public_keys, path,
> +						    sig, sig_len, NULL, 0);
>   
>   		if (!err && imaevm_params.verbose > LOG_INFO)
>   			log_info("%s: verification is OK\n", path);
> @@ -2223,6 +2225,7 @@ static int ima_measurement(const char *file)
>   	int first_record = 1;
>   	unsigned int pseudo_padded_banks_mask, pseudo_banks_mask;
>   	unsigned long entry_num = 0;
> +	void *public_keys = NULL;
>   	int c;
>   
>   	struct template_entry entry = { .template = NULL };
> @@ -2252,9 +2255,9 @@ static int ima_measurement(const char *file)
>   	}
>   
>   	if (imaevm_params.keyfile)	/* Support multiple public keys */
> -		init_public_keys(imaevm_params.keyfile);
> +		init_public_keys2(imaevm_params.keyfile, &public_keys);
>   	else				/* assume read pubkey from x509 cert */
> -		init_public_keys("/etc/keys/x509_evm.der");
> +		init_public_keys2("/etc/keys/x509_evm.der", &public_keys);
>   	if (errno)

Hm, a failing malloc() sets errno, so this would be ok. I would change 
this to check for return value != 0 from init_public_keys2().

>   		log_errno_reset(LOG_DEBUG,
>   				"Failure in initializing public keys");
> @@ -2405,7 +2408,7 @@ static int ima_measurement(const char *file)
>   		if (is_ima_template)
>   			ima_show(&entry);
>   		else
> -			ima_ng_show(&entry);
> +			ima_ng_show(public_keys, &entry);
>   
>   		if (!tpmbanks)
>   			continue;
> @@ -2464,6 +2467,7 @@ out_free:
>   	free(pseudo_banks);
>   	free(pseudo_padded_banks);
>   	free(entry.template);
> +	free_public_keys(public_keys);
>   
>   	return err;
>   }

Re: [ima-evm-utils PATCH 08/12] Define library ima_calc_hash2() function with a hash algorithm parameter

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Instead of relying on the "imaevm_params.algo" global variable, which
> is not concurrency-safe, define a new library ima_calc_hash2() function
> with the hash algorithm as a parameter.
> 
> To avoid library incompatablity, make the existing ima_calc_hash()

-> incompatibility

> function a wrapper for ima_calc_hash2().
> 
> Deprecate ima_calc_hash().

Same comment about possible __attribute__((deprecated)).

> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/imaevm.h    |  1 +
>   src/libimaevm.c | 12 ++++++++----
>   2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/imaevm.h b/src/imaevm.h
> index 1ed2c81d510d..81acd8df41cb 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -237,6 +237,7 @@ extern struct libimaevm_params imaevm_params;
>   void imaevm_do_hexdump(FILE *fp, const void *ptr, int len, bool cr);
>   void imaevm_hexdump(const void *ptr, int len);
>   int ima_calc_hash(const char *file, uint8_t *hash);
> +int ima_calc_hash2(const char *file, const char *hash_algo, uint8_t *hash);
>   int imaevm_get_hash_algo(const char *algo);
>   RSA *read_pub_key(const char *keyfile, int x509);
>   EVP_PKEY *read_pub_pkey(const char *keyfile, int x509);
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index e64d167a2a8a..4c9da7a2f06b 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -181,7 +181,7 @@ out:
>   	return err;
>   }
>   
> -int ima_calc_hash(const char *file, uint8_t *hash)
> +int ima_calc_hash2(const char *file, const char *hash_algo, uint8_t *hash)
>   {
>   	const EVP_MD *md;
>   	struct stat st;
> @@ -202,10 +202,9 @@ int ima_calc_hash(const char *file, uint8_t *hash)
>   		goto err;
>   	}
>   
> -	md = EVP_get_digestbyname(imaevm_params.hash_algo);
> +	md = EVP_get_digestbyname(hash_algo);
>   	if (!md) {
> -		log_err("EVP_get_digestbyname(%s) failed\n",
> -			imaevm_params.hash_algo);
> +		log_err("EVP_get_digestbyname(%s) failed\n", hash_algo);
>   		err = 1;
>   		goto err;
>   	}
> @@ -246,6 +245,11 @@ err:
>   	return err;
>   }
>   
> +int ima_calc_hash(const char *file, uint8_t *hash)
> +{
> +	return ima_calc_hash2(file, imaevm_params.hash_algo, hash);
> +}
> +
>   EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
>   {
>   	FILE *fp;

Rest looks good to me.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [ima-evm-utils PATCH 09/12] Use a local hash algorithm variable when verifying file signatures

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Instead of relying on the "imaevm_params.algo" global variable, which
> is not concurrency-safe, define and use a local variable.
> 
> Update static verify_hash_v2(), verify_hash_v3(), and verify_hash_common()
> function definitions to include a hash algorithm argument.
> 
> Similarly update ima_verify_signature2() and ima_calc_hash2() to define
> and use a local hash algorithm variable.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/libimaevm.c | 40 ++++++++++++++++++++++++----------------
>   1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 4c9da7a2f06b..18b6a6f27237 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -488,6 +488,7 @@ void init_public_keys(const char *keyfiles)
>    * (Note: signature_v2_hdr struct does not contain the 'type'.)
>    */
>   static int verify_hash_common(void *public_keys, const char *file,
> +			      const char *hash_algo,
>   			      const unsigned char *hash,
>   			      int size, unsigned char *sig, int siglen)
>   {
> @@ -499,7 +500,7 @@ static int verify_hash_common(void *public_keys, const char *file,
>   	const char *st;
>   
>   	if (imaevm_params.verbose > LOG_INFO) {
> -		log_info("hash(%s): ", imaevm_params.hash_algo);
> +		log_info("hash(%s): ", hash_algo);
>   		log_dump(hash, size);
>   	}
>   
> @@ -530,7 +531,7 @@ static int verify_hash_common(void *public_keys, const char *file,
>   	if (!EVP_PKEY_verify_init(ctx))
>   		goto err;
>   	st = "EVP_get_digestbyname";
> -	if (!(md = EVP_get_digestbyname(imaevm_params.hash_algo)))
> +	if (!(md = EVP_get_digestbyname(hash_algo)))
>   		goto err;
>   	st = "EVP_PKEY_CTX_set_signature_md";
>   	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
> @@ -566,11 +567,12 @@ err:
>    * Return: 0 verification good, 1 verification bad, -1 error.
>    */
>   static int verify_hash_v2(void *public_keys, const char *file,
> +			  const char *hash_algo,
>   			  const unsigned char *hash,
>   			  int size, unsigned char *sig, int siglen)
>   {
>   	/* note: signature_v2_hdr does not contain 'type', use sig + 1 */
> -	return verify_hash_common(public_keys, file, hash, size,
> +	return verify_hash_common(public_keys, file, hash_algo, hash, size,
>   				  sig + 1, siglen - 1);
>   }
>   
> @@ -581,19 +583,20 @@ static int verify_hash_v2(void *public_keys, const char *file,
>    * Return: 0 verification good, 1 verification bad, -1 error.
>    */
>   static int verify_hash_v3(void *public_keys, const char *file,
> +			  const char *hash_algo,
>   			  const unsigned char *hash,
>   			  int size, unsigned char *sig, int siglen)
>   {
>   	unsigned char sigv3_hash[MAX_DIGEST_SIZE];
>   	int ret;
>   
> -	ret = calc_hash_sigv3(sig[0], NULL, hash, sigv3_hash);
> +	ret = calc_hash_sigv3(sig[0], hash_algo, hash, sigv3_hash);
>   	if (ret < 0)
>   		return ret;
>   
>   	/* note: signature_v2_hdr does not contain 'type', use sig + 1 */
> -	return verify_hash_common(public_keys, file, sigv3_hash, size,
> -				  sig + 1, siglen - 1);
> +	return verify_hash_common(public_keys, file, hash_algo, sigv3_hash,
> +				  size, sig + 1, siglen - 1);
>   }
>   
>   #define HASH_MAX_DIGESTSIZE 64	/* kernel HASH_MAX_DIGESTSIZE is 64 bytes */
> @@ -636,8 +639,10 @@ int calc_hash_sigv3(enum evm_ima_xattr_type type, const char *algo,
>   		return -EINVAL;
>   	}
>   
> -	if (!algo)
> -		algo = imaevm_params.hash_algo;
> +	if (!algo) {
> +		log_err("Hash algorithm unspecified\n");
> +		return -EINVAL;
> +	}
>   
>   	if ((hash_algo = imaevm_get_hash_algo(algo)) < 0) {
>   		log_err("Hash algorithm %s not supported\n", algo);
> @@ -757,10 +762,10 @@ int verify_hash2(void *public_keys, const char *file, const char *hash_algo,
>   		return -1;
>   #endif
>   	} else if (sig[1] == DIGSIG_VERSION_2) {
> -		return verify_hash_v2(public_keys, file, hash, size,
> +		return verify_hash_v2(public_keys, file, hash_algo, hash, size,
>   				      sig, siglen);
>   	} else if (sig[1] == DIGSIG_VERSION_3) {
> -		return verify_hash_v3(public_keys, file, hash, size,
> +		return verify_hash_v3(public_keys, file, hash_algo, hash, size,
>   				      sig, siglen);
>   	} else
>   		return -1;
> @@ -774,7 +779,8 @@ int verify_hash2(void *public_keys, const char *file, const char *hash_algo,
>   int verify_hash(const char *file, const unsigned char *hash, int size,
>   		unsigned char *sig, int siglen)
>   {
> -	return verify_hash2(g_public_keys, file, NULL, hash, size, sig, siglen);
> +	return verify_hash2(g_public_keys, file, imaevm_params.hash_algo,
> +			    hash, size, sig, siglen);

Now you are passing valid parameters into verify_hash2(). Would it not 
be possible to drop 4/12?

Re: [ima-evm-utils PATCH 10/12] Update EVM signature verification to use a local hash algorithm variable

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Instead of relying on the "imaevm_params.algo" global variable, which
> is not concurrency-safe, define and use a local file hash algorithm
> variable.
> 
> Update calc_evm_hash(), verify_hash2().
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index ad4565b3ee52..7ae897d8b8b3 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -340,7 +340,8 @@ err:
>    * Returns 0 for EVP_ function failures. Return -1 for other failures.
>    * Return hash algorithm size on success.
>    */
> -static int calc_evm_hash(const char *file, unsigned char *hash)
> +static int calc_evm_hash(const char *file, const char *hash_algo,
> +			 unsigned char *hash)
>   {
>           const EVP_MD *md;
>   	struct stat st;
> @@ -408,10 +409,9 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
>   	}
>   #endif
>   
> -	md = EVP_get_digestbyname(imaevm_params.hash_algo);
> +	md = EVP_get_digestbyname(hash_algo);
>   	if (!md) {
> -		log_err("EVP_get_digestbyname(%s) failed\n",
> -			imaevm_params.hash_algo);
> +		log_err("EVP_get_digestbyname(%s) failed\n", hash_algo);
>   		err = 0;
>   		goto out;
>   	}
> @@ -570,7 +570,7 @@ static int sign_evm(const char *file, const char *key)
>   	unsigned char sig[MAX_SIGNATURE_SIZE];
>   	int len, err;
>   
> -	len = calc_evm_hash(file, hash);
> +	len = calc_evm_hash(file, imaevm_params.hash_algo, hash);
>   	if (len <= 1)
>   		return len;
>   	assert(len <= sizeof(hash));
> @@ -909,6 +909,7 @@ static int verify_evm(void *public_keys, const char *file)
>   {
>   	unsigned char hash[MAX_DIGEST_SIZE];
>   	unsigned char sig[MAX_SIGNATURE_SIZE];
> +	const char *hash_algo = NULL;
>   	int sig_hash_algo;
>   	int mdlen;
>   	int len;
> @@ -938,15 +939,15 @@ static int verify_evm(void *public_keys, const char *file)
>   		log_err("unknown hash algo: %s\n", file);
>   		return -1;
>   	}
> -	imaevm_params.hash_algo = imaevm_hash_algo_by_id(sig_hash_algo);
> +	hash_algo = imaevm_hash_algo_by_id(sig_hash_algo);
>   
> -	mdlen = calc_evm_hash(file, hash);
> +	mdlen = calc_evm_hash(file, hash_algo, hash);
>   	if (mdlen <= 1)
>   		return mdlen;
>   	assert(mdlen <= sizeof(hash));
>   
> -	return verify_hash2(public_keys, file, imaevm_params.hash_algo,
> -			    hash, mdlen, sig, len);
> +	return verify_hash2(public_keys, file, hash_algo, hash,
> +			    mdlen, sig, len);
>   }
>   
>   static int cmd_verify_evm(struct command *cmd)

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [ima-evm-utils PATCH 11/12] Use a file specific hash algorithm variable for signing files

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Instead of relying on the library "imaevm_params.algo" global variable,
> which is not concurrency-safe, define and use an evmctl file specific
> hash algorithm variable.
> 
> Propogate using the file specific hash algorithm variable in sign_evm(),

-> Propagate

> sign_ima(), hash_ima(), and sign_hash() function.
> 
> Replace using the library function ima_calc_hash() with ima_calc_hash2().
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 7ae897d8b8b3..b802eeb1bf15 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -140,6 +140,7 @@ static bool evm_immutable;
>   static bool evm_portable;
>   static bool veritysig;
>   static bool hwtpm;
> +static char *hash_algo;

g_hash_algo to avoid shadowing this variable in some cases where there 
is a function parameter of the same name?


>   
>   #define HMAC_FLAG_NO_UUID	0x0001
>   #define HMAC_FLAG_CAPS_SET	0x0002
> @@ -570,12 +571,12 @@ static int sign_evm(const char *file, const char *key)

Add hash_algo to the parameters here and call it sign_evm(file, key, 
g_hash_algo)? It makes sense to pass the hash_algo into this function 
that needs to know this for the signature and pushed the usage of global 
variables further out.

>   	unsigned char sig[MAX_SIGNATURE_SIZE];
>   	int len, err;
>   
> -	len = calc_evm_hash(file, imaevm_params.hash_algo, hash);
> +	len = calc_evm_hash(file, hash_algo, hash);
>   	if (len <= 1)
>   		return len;
>   	assert(len <= sizeof(hash));
>   
> -	len = sign_hash(imaevm_params.hash_algo, hash, len, key, NULL, sig + 1);
> +	len = sign_hash(hash_algo, hash, len, key, NULL, sig + 1);
>   	if (len <= 1)
>   		return len;
>   	assert(len < sizeof(sig));
> @@ -609,10 +610,10 @@ static int hash_ima(const char *file)
>   {
>   	unsigned char hash[MAX_DIGEST_SIZE + 2]; /* +2 byte xattr header */
>   	int len, err, offset;
> -	int algo = imaevm_get_hash_algo(imaevm_params.hash_algo);
> +	int algo = imaevm_get_hash_algo(hash_algo);
>   
>   	if (algo < 0) {
> -		log_err("Unknown hash algo: %s\n", imaevm_params.hash_algo);
> +		log_err("Unknown hash algo: %s\n", hash_algo);
>   		return -1;
>   	}
>   	if (algo > PKEY_HASH_SHA1) {
> @@ -624,7 +625,7 @@ static int hash_ima(const char *file)
>   		offset = 1;
>   	}
>   
> -	len = ima_calc_hash(file, hash + offset);
> +	len = ima_calc_hash2(file, hash_algo, hash + offset);
>   	if (len <= 1)
>   		return len;
>   	assert(len + offset <= sizeof(hash));
> @@ -632,7 +633,7 @@ static int hash_ima(const char *file)
>   	len += offset;
>   
>   	if (imaevm_params.verbose >= LOG_INFO)
> -		log_info("hash(%s): ", imaevm_params.hash_algo);
> +		log_info("hash(%s): ", hash_algo);
>   
>   	if (sigdump || imaevm_params.verbose >= LOG_INFO)
>   		imaevm_hexdump(hash, len);
> @@ -656,12 +657,12 @@ static int sign_ima(const char *file, const char *key)
>   	unsigned char sig[MAX_SIGNATURE_SIZE];
>   	int len, err;
>   
> -	len = ima_calc_hash(file, hash);
> +	len = ima_calc_hash2(file, hash_algo, hash);
>   	if (len <= 1)
>   		return len;
>   	assert(len <= sizeof(hash));
>   
> -	len = sign_hash(imaevm_params.hash_algo, hash, len, key, NULL, sig + 1);
> +	len = sign_hash(hash_algo, hash, len, key, NULL, sig + 1);
>   	if (len <= 1)
>   		return len;
>   	assert(len < sizeof(sig));
> @@ -854,7 +855,7 @@ static int cmd_sign_hash(struct command *cmd)
>   			assert(hashlen / 2 <= sizeof(hash));
>   			hex2bin(hash, line, hashlen / 2);
>   
> -			siglen = sign_hash(imaevm_params.hash_algo, hash,
> +			siglen = sign_hash(hash_algo, hash,
>   					   hashlen / 2, key, NULL, sig + 1);
>   			sig[0] = EVM_IMA_XATTR_DIGSIG;
>   		}
> @@ -3077,7 +3078,7 @@ int main(int argc, char *argv[])
>   			sigdump = 1;
>   			break;
>   		case 'a':
> -			imaevm_params.hash_algo = optarg;
> +			hash_algo = optarg;
>   			break;
>   		case 'p':
>   			if (optarg)

Re: [ima-evm-utils PATCH 09/12] Use a local hash algorithm variable when verifying file signatures

[Mimi Zohar]

On Wed, 2023-11-22 at 08:37 -0500, Stefan Berger wrote:
> 
> On 11/19/23 11:50, Mimi Zohar wrote:
> > Instead of relying on the "imaevm_params.algo" global variable, which
> > is not concurrency-safe, define and use a local variable.
> > 
> > Update static verify_hash_v2(), verify_hash_v3(), and verify_hash_common()
> > function definitions to include a hash algorithm argument.
> > 
> > Similarly update ima_verify_signature2() and ima_calc_hash2() to define
> > and use a local hash algorithm variable.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >   src/libimaevm.c | 40 ++++++++++++++++++++++++----------------
> >   1 file changed, 24 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index 4c9da7a2f06b..18b6a6f27237 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -488,6 +488,7 @@ void init_public_keys(const char *keyfiles)
> >    * (Note: signature_v2_hdr struct does not contain the 'type'.)
> >    */
> >   static int verify_hash_common(void *public_keys, const char *file,
> > +			      const char *hash_algo,
> >   			      const unsigned char *hash,
> >   			      int size, unsigned char *sig, int siglen)
> >   {
> > @@ -499,7 +500,7 @@ static int verify_hash_common(void *public_keys, const char *file,
> >   	const char *st;
> >   
> >   	if (imaevm_params.verbose > LOG_INFO) {
> > -		log_info("hash(%s): ", imaevm_params.hash_algo);
> > +		log_info("hash(%s): ", hash_algo);
> >   		log_dump(hash, size);
> >   	}
> >   
> > @@ -530,7 +531,7 @@ static int verify_hash_common(void *public_keys, const char *file,
> >   	if (!EVP_PKEY_verify_init(ctx))
> >   		goto err;
> >   	st = "EVP_get_digestbyname";
> > -	if (!(md = EVP_get_digestbyname(imaevm_params.hash_algo)))
> > +	if (!(md = EVP_get_digestbyname(hash_algo)))
> >   		goto err;
> >   	st = "EVP_PKEY_CTX_set_signature_md";
> >   	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
> > @@ -566,11 +567,12 @@ err:
> >    * Return: 0 verification good, 1 verification bad, -1 error.
> >    */
> >   static int verify_hash_v2(void *public_keys, const char *file,
> > +			  const char *hash_algo,
> >   			  const unsigned char *hash,
> >   			  int size, unsigned char *sig, int siglen)
> >   {
> >   	/* note: signature_v2_hdr does not contain 'type', use sig + 1 */
> > -	return verify_hash_common(public_keys, file, hash, size,
> > +	return verify_hash_common(public_keys, file, hash_algo, hash, size,
> >   				  sig + 1, siglen - 1);
> >   }
> >   
> > @@ -581,19 +583,20 @@ static int verify_hash_v2(void *public_keys, const char *file,
> >    * Return: 0 verification good, 1 verification bad, -1 error.
> >    */
> >   static int verify_hash_v3(void *public_keys, const char *file,
> > +			  const char *hash_algo,
> >   			  const unsigned char *hash,
> >   			  int size, unsigned char *sig, int siglen)
> >   {
> >   	unsigned char sigv3_hash[MAX_DIGEST_SIZE];
> >   	int ret;
> >   
> > -	ret = calc_hash_sigv3(sig[0], NULL, hash, sigv3_hash);
> > +	ret = calc_hash_sigv3(sig[0], hash_algo, hash, sigv3_hash);
> >   	if (ret < 0)
> >   		return ret;
> >   
> >   	/* note: signature_v2_hdr does not contain 'type', use sig + 1 */
> > -	return verify_hash_common(public_keys, file, sigv3_hash, size,
> > -				  sig + 1, siglen - 1);
> > +	return verify_hash_common(public_keys, file, hash_algo, sigv3_hash,
> > +				  size, sig + 1, siglen - 1);
> >   }
> >   
> >   #define HASH_MAX_DIGESTSIZE 64	/* kernel HASH_MAX_DIGESTSIZE is 64 bytes */
> > @@ -636,8 +639,10 @@ int calc_hash_sigv3(enum evm_ima_xattr_type type, const char *algo,
> >   		return -EINVAL;
> >   	}
> >   
> > -	if (!algo)
> > -		algo = imaevm_params.hash_algo;
> > +	if (!algo) {
> > +		log_err("Hash algorithm unspecified\n");
> > +		return -EINVAL;
> > +	}
> >   
> >   	if ((hash_algo = imaevm_get_hash_algo(algo)) < 0) {
> >   		log_err("Hash algorithm %s not supported\n", algo);
> > @@ -757,10 +762,10 @@ int verify_hash2(void *public_keys, const char *file, const char *hash_algo,
> >   		return -1;
> >   #endif
> >   	} else if (sig[1] == DIGSIG_VERSION_2) {
> > -		return verify_hash_v2(public_keys, file, hash, size,
> > +		return verify_hash_v2(public_keys, file, hash_algo, hash, size,
> >   				      sig, siglen);
> >   	} else if (sig[1] == DIGSIG_VERSION_3) {
> > -		return verify_hash_v3(public_keys, file, hash, size,
> > +		return verify_hash_v3(public_keys, file, hash_algo, hash, size,
> >   				      sig, siglen);
> >   	} else
> >   		return -1;
> > @@ -774,7 +779,8 @@ int verify_hash2(void *public_keys, const char *file, const char *hash_algo,
> >   int verify_hash(const char *file, const unsigned char *hash, int size,
> >   		unsigned char *sig, int siglen)
> >   {
> > -	return verify_hash2(g_public_keys, file, NULL, hash, size, sig, siglen);
> > +	return verify_hash2(g_public_keys, file, imaevm_params.hash_algo,
> > +			    hash, size, sig, siglen);
> 
> Now you are passing valid parameters into verify_hash2(). Would it not 
> be possible to drop 4/12?

Just as we can't modify the library verify_hash() definition, I don't
think we should be modifying the verify_hash2() defintion either. 
04/12 defines and exports the final verify_hash2() definition.

-- 
thanks,

Mimi

Re: [ima-evm-utils PATCH 12/12] Define and use a file specific "keypass" variable

[Stefan Berger]

On 11/19/23 11:50, Mimi Zohar wrote:
> Instead of relying on the "imaevm_params.keypass" global variable, which
> is not concurrency-safe, add keypass as a parameter to the static library
> functions definitions.  Update function callers.
> 
> To avoid library incompatablity, don't remove imaevm_params.keypass
> variable.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   src/evmctl.c    |  9 +++++----
>   src/libimaevm.c | 17 ++++++++---------
>   2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index b802eeb1bf15..6d6160159a1f 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -141,6 +141,7 @@ static bool evm_portable;
>   static bool veritysig;
>   static bool hwtpm;
>   static char *hash_algo;
> +static char *keypass;
>   
>   #define HMAC_FLAG_NO_UUID	0x0001
>   #define HMAC_FLAG_CAPS_SET	0x0002
> @@ -3082,9 +3083,9 @@ int main(int argc, char *argv[])
>   			break;
>   		case 'p':
>   			if (optarg)
> -				imaevm_params.keypass = optarg;
> +				keypass = optarg;
>   			else
> -				imaevm_params.keypass = get_password();
> +				keypass = get_password();
>   			break;
>   		case 'f':
>   			sigfile = 1;
> @@ -3226,8 +3227,8 @@ int main(int argc, char *argv[])
>   		}
>   	}
>   
> -	if (!imaevm_params.keypass)
> -		imaevm_params.keypass = getenv("EVMCTL_KEY_PASSWORD");
> +	if (!keypass)
> +		keypass = getenv("EVMCTL_KEY_PASSWORD");
>   
>   	if (imaevm_params.keyfile != NULL &&
>   	    imaevm_params.eng == NULL &&


The problem at this point in evmctl is that keypass is never passed to 
anywhere. You have to pass it to sign_hash().

> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 18b6a6f27237..10ec847da08a 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -1124,7 +1124,8 @@ static int get_hash_algo_v1(const char *algo)
>   }
>   
>   static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
> -			int size, const char *keyfile, unsigned char *sig)
> +			int size, const char *keyfile, const char *keypass,
> +			unsigned char *sig)
>   {
>   	int len = -1, hashalgo_idx;
>   	SHA_CTX ctx;
> @@ -1158,7 +1159,7 @@ static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
>   	log_info("hash(%s): ", hashalgo);
>   	log_dump(hash, size);
>   
> -	key = read_priv_key(keyfile, imaevm_params.keypass);
> +	key = read_priv_key(keyfile, keypass);
>   	if (!key)
>   		return -1;
>   
> @@ -1211,7 +1212,8 @@ out:
>    * Return: -1 signing error, >0 length of signature
>    */
>   static int sign_hash_v2(const char *algo, const unsigned char *hash,
> -			int size, const char *keyfile, unsigned char *sig)
> +			int size, const char *keyfile, const char *keypass,
> +			unsigned char *sig)
>   {
>   	struct signature_v2_hdr *hdr;
>   	int len = -1;
> @@ -1246,7 +1248,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
>   	log_info("hash(%s): ", algo);
>   	log_dump(hash, size);
>   
> -	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
> +	pkey = read_priv_pkey(keyfile, keypass);
>   	if (!pkey)
>   		return -1;
>   
> @@ -1316,14 +1318,11 @@ err:
>   
>   int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
>   {
> -	if (keypass)
> -		imaevm_params.keypass = keypass;
> -

I hope all library callers, other than evmctl, passed the keypass in 
already, otherwise they could have set the password via 
imaevm_params.keypass global and passed in NULL and for them this will 
be a behavioral change. Anyway, you have no other choice to get rid of 
imaevm_params as much as possible, so I guess it's ok.

It looks like at this point imaevm_params.keypass doesn't have a single 
user anymore so you could rename it to 'unused1' or so in the stucture 
since it will have zero effect for anyone to set it.

>   	if (imaevm_params.x509)
> -		return sign_hash_v2(hashalgo, hash, size, keyfile, sig);
> +		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig);
>   #if CONFIG_SIGV1
>   	else
> -		return sign_hash_v1(hashalgo, hash, size, keyfile, sig);
> +		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig);
>   #endif
>   	log_info("Signature version 1 deprecated.");
>   	return -1;

Re: [ima-evm-utils PATCH 09/12] Use a local hash algorithm variable when verifying file signatures

[Stefan Berger]

On 11/22/23 09:14, Mimi Zohar wrote:
> On Wed, 2023-11-22 at 08:37 -0500, Stefan Berger wrote:
>>

>> Now you are passing valid parameters into verify_hash2(). Would it not
>> be possible to drop 4/12?
> 
> Just as we can't modify the library verify_hash() definition, I don't
> think we should be modifying the verify_hash2() defintion either.
> 04/12 defines and exports the final verify_hash2() definition.
> 

The question is whether verify_hash2() can be only introduced here in 
versus made available in 4/12 with a parameter that it doesn't use at 
all at that point.

Re: [ima-evm-utils PATCH 12/12] Define and use a file specific "keypass" variable

[Mimi Zohar]

On Wed, 2023-11-22 at 09:22 -0500, Stefan Berger wrote:
> 
> On 11/19/23 11:50, Mimi Zohar wrote:
> > Instead of relying on the "imaevm_params.keypass" global variable, which
> > is not concurrency-safe, add keypass as a parameter to the static library
> > functions definitions.  Update function callers.
> > 
> > To avoid library incompatablity, don't remove imaevm_params.keypass
> > variable.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >   src/evmctl.c    |  9 +++++----
> >   src/libimaevm.c | 17 ++++++++---------
> >   2 files changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index b802eeb1bf15..6d6160159a1f 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -141,6 +141,7 @@ static bool evm_portable;
> >   static bool veritysig;
> >   static bool hwtpm;
> >   static char *hash_algo;
> > +static char *keypass;
> >   
> >   #define HMAC_FLAG_NO_UUID	0x0001
> >   #define HMAC_FLAG_CAPS_SET	0x0002
> > @@ -3082,9 +3083,9 @@ int main(int argc, char *argv[])
> >   			break;
> >   		case 'p':
> >   			if (optarg)
> > -				imaevm_params.keypass = optarg;
> > +				keypass = optarg;
> >   			else
> > -				imaevm_params.keypass = get_password();
> > +				keypass = get_password();
> >   			break;
> >   		case 'f':
> >   			sigfile = 1;
> > @@ -3226,8 +3227,8 @@ int main(int argc, char *argv[])
> >   		}
> >   	}
> >   
> > -	if (!imaevm_params.keypass)
> > -		imaevm_params.keypass = getenv("EVMCTL_KEY_PASSWORD");
> > +	if (!keypass)
> > +		keypass = getenv("EVMCTL_KEY_PASSWORD");
> >   
> >   	if (imaevm_params.keyfile != NULL &&
> >   	    imaevm_params.eng == NULL &&
> 
> 
> The problem at this point in evmctl is that keypass is never passed to 
> anywhere. You have to pass it to sign_hash().

Thanks for catching this.  Part of the patch was missing was obviously
missing.

> 
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index 18b6a6f27237..10ec847da08a 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -1124,7 +1124,8 @@ static int get_hash_algo_v1(const char *algo)
> >   }
> >   
> >   static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
> > -			int size, const char *keyfile, unsigned char *sig)
> > +			int size, const char *keyfile, const char *keypass,
> > +			unsigned char *sig)
> >   {
> >   	int len = -1, hashalgo_idx;
> >   	SHA_CTX ctx;
> > @@ -1158,7 +1159,7 @@ static int sign_hash_v1(const char *hashalgo, const unsigned char *hash,
> >   	log_info("hash(%s): ", hashalgo);
> >   	log_dump(hash, size);
> >   
> > -	key = read_priv_key(keyfile, imaevm_params.keypass);
> > +	key = read_priv_key(keyfile, keypass);
> >   	if (!key)
> >   		return -1;
> >   
> > @@ -1211,7 +1212,8 @@ out:
> >    * Return: -1 signing error, >0 length of signature
> >    */
> >   static int sign_hash_v2(const char *algo, const unsigned char *hash,
> > -			int size, const char *keyfile, unsigned char *sig)
> > +			int size, const char *keyfile, const char *keypass,
> > +			unsigned char *sig)
> >   {
> >   	struct signature_v2_hdr *hdr;
> >   	int len = -1;
> > @@ -1246,7 +1248,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
> >   	log_info("hash(%s): ", algo);
> >   	log_dump(hash, size);
> >   
> > -	pkey = read_priv_pkey(keyfile, imaevm_params.keypass);
> > +	pkey = read_priv_pkey(keyfile, keypass);
> >   	if (!pkey)
> >   		return -1;
> >   
> > @@ -1316,14 +1318,11 @@ err:
> >   
> >   int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
> >   {
> > -	if (keypass)
> > -		imaevm_params.keypass = keypass;
> > -
> 
> I hope all library callers, other than evmctl, passed the keypass in 
> already, otherwise they could have set the password via 
> imaevm_params.keypass global and passed in NULL and for them this will 
> be a behavioral change. Anyway, you have no other choice to get rid of 
> imaevm_params as much as possible, so I guess it's ok.

For now we could invert the test and do exactly the opposite, like
this:

+       if (!keypass)   /* Avoid breaking existing libimaevm usage */
+               keypass = imaevm_params.keypass;
+

thanks,

Mimi

> 
> It looks like at this point imaevm_params.keypass doesn't have a single 
> user anymore so you could rename it to 'unused1' or so in the stucture 
> since it will have zero effect for anyone to set it.
> 
> >   	if (imaevm_params.x509)
> > -		return sign_hash_v2(hashalgo, hash, size, keyfile, sig);
> > +		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig);
> >   #if CONFIG_SIGV1
> >   	else
> > -		return sign_hash_v1(hashalgo, hash, size, keyfile, sig);
> > +		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig);
> >   #endif
> >   	log_info("Signature version 1 deprecated.");
> >   	return -1;

Re: [ima-evm-utils PATCH 09/12] Use a local hash algorithm variable when verifying file signatures

[Mimi Zohar]

On Wed, 2023-11-22 at 09:33 -0500, Stefan Berger wrote:
> 
> On 11/22/23 09:14, Mimi Zohar wrote:
> > On Wed, 2023-11-22 at 08:37 -0500, Stefan Berger wrote:
> >>
> 
> >> Now you are passing valid parameters into verify_hash2(). Would it not
> >> be possible to drop 4/12?
> > 
> > Just as we can't modify the library verify_hash() definition, I don't
> > think we should be modifying the verify_hash2() defintion either.
> > 04/12 defines and exports the final verify_hash2() definition.
> > 
> 
> The question is whether verify_hash2() can be only introduced here in 
> versus made available in 4/12 with a parameter that it doesn't use at 
> all at that point.

Correct, however 6/12 "Update cmd_verify_evm to define and use a local
list of public keys" calls verify_hash2().

-- 
thanks,

Mimi