diff mbox series

[v2,ima-evm-utils,2/6] Pass ENGINE and keyid through to function using them

Message ID 20240223153014.4048133-3-stefanb@linux.ibm.com (mailing list archive)
State New
Headers show
Series Deprecate sign_hash and add provider support | expand

Commit Message

Stefan Berger Feb. 23, 2024, 3:30 p.m. UTC 
Pass the ENGINE and keyid all the way through to the function that is
using them and deprecate sign_hash since it needs to pass these parameters
from the global imaevm_params. Define a new API call imaevm_signhash that
takes all necessary variables as parameters.

In preparation of support for OpenSSL providers, wrap the ENGINE in a
union inside a struct imaevm_ossl_access and add a type for the selection
of the ENGINE or provider later on.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 src/imaevm.h    |  26 ++++++++-
 src/libimaevm.c | 144 ++++++++++++++++++++++++++++++++++++------------
 2 files changed, 134 insertions(+), 36 deletions(-)

Comments

Mimi Zohar Feb. 26, 2024, 4:06 p.m. UTC | #1 
Hi Stefan,

"Pass ENGINE and keyid through to function using them" describes what the patch
does, but not the reason for the patch.  Please update the patch description
Subject line and similarly the cover letter Subject line.


On Fri, 2024-02-23 at 10:30 -0500, Stefan Berger wrote:
> Pass the ENGINE and keyid all the way through to the function that is
> using them and deprecate sign_hash since it needs to pass these parameters
> from the global imaevm_params. Define a new API call imaevm_signhash that
> takes all necessary variables as parameters.

The motiviation for this patch is missing.

Instead of relying on imaevm_params.engine and imaevm_params.keyid global
variables, which are not concurrency-safe, define a new library function
imaevm_signhash() function with the engine and keyid as parameters.

Pass the ENGINE and keyid ...

> 
> In preparation of support for OpenSSL providers, wrap the ENGINE in a
> union inside a struct imaevm_ossl_access and add a type for the selection
> of the ENGINE or provider later on.

(If possible) Make sign_hash() a wrapper for imaevm_signhash().

Deprecate sign_hash().

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

[...]

>  int sign_hash(const char *hashalgo, const unsigned char *hash, int size,
> const char *keyfile, const char *keypass, unsigned char *sig)
>  {
> +	const struct imaevm_ossl_access access_info = {
> +		.type = IMAEVM_OSSL_ACCESS_TYPE_ENGINE,
> +		.u.engine = imaevm_params.eng,
> +	};
>  	if (!keypass)	/* Avoid breaking existing libimaevm usage */
>  		keypass = imaevm_params.keypass;
>  
>  	if (imaevm_params.x509)
> -		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass,
> sig);
> +		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig,
> +				    &access_info, imaevm_params.keyid);
>  #if CONFIG_SIGV1
>  	else
> -		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass,
> sig);
> +		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig,
> +				    &access_info, imaevm_params.keyid);

If possible sign_hash() should become a wrapper for imaevm_signhash().

>  #endif
>  	log_info("Signature version 1 deprecated.");

>  	return -1;


thanks,

Mimi
diff mbox series

Patch

diff --git a/src/imaevm.h b/src/imaevm.h
index 6a52afb..6764604 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -51,7 +51,10 @@ 
 #include <openssl/opensslconf.h>
 
 #if !defined(OPENSSL_NO_ENGINE) && !defined(OPENSSL_NO_DYNAMIC_ENGINE)
-#include <openssl/engine.h>
+# include <openssl/engine.h>
+#else
+struct engine_st;
+typedef struct engine_st ENGINE; /* unused when no engine support */
 #endif
 
 #ifdef USE_FPRINTF
@@ -250,7 +253,9 @@  void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey);
 int key2bin(RSA *key, unsigned char *pub);
 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);
+IMAEVM_DEPRECATED int sign_hash(const char *algo, const unsigned char *hash,
+				int size, const char *keyfile, const char *keypass,
+				unsigned char *sig);
 IMAEVM_DEPRECATED int ima_calc_hash(const char *file, uint8_t *hash);
 IMAEVM_DEPRECATED int verify_hash(const char *file, const unsigned char *hash,
 				  int size, unsigned char *sig, int siglen);
@@ -259,7 +264,24 @@  IMAEVM_DEPRECATED int ima_verify_signature(const char *file, unsigned char *sig,
 					   int digestlen);
 IMAEVM_DEPRECATED void init_public_keys(const char *keyfiles);
 
+struct imaevm_ossl_access {
+	int type;
+#define IMAEVM_OSSL_ACCESS_TYPE_NONE   0
+#define IMAEVM_OSSL_ACCESS_TYPE_ENGINE 1  /* also: engine field exists */
+	union {
+		ENGINE *engine;
+	} u;
+};
+
+#define IMAEVM_SIGFLAG_SIGNATURE_V1	(1 << 0) /* v1 signature; deprecated */
+#define IMAEVM_SIGFLAGS_SUPPORT		(1 << 0) /* mask of all supported flags */
+
 int ima_calc_hash2(const char *file, const char *hash_algo, uint8_t *hash);
+int imaevm_signhash(const char *hashalgo, const unsigned char *hash, int size,
+		    const char *keyfile, const char *keypass,
+		    unsigned char *sig, long sigflags,
+		    const struct imaevm_ossl_access *access_info,
+		    uint32_t keyid);
 int imaevm_verify_hash(struct public_key_entry *public_keys, const char *file,
 		       const char *hash_algo, const unsigned char *hash,
 		       int size, unsigned char *sig, int siglen);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index ce4f6f7..91af613 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -1031,33 +1031,53 @@  uint32_t imaevm_read_keyid(const char *certfile)
 	return ntohl(keyid_be);
 }
 
-static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
+static EVP_PKEY *read_priv_pkey_engine(ENGINE *e, const char *keyfile,
+				       const char *keypass, uint32_t keyid)
 {
-	FILE *fp;
-	EVP_PKEY *pkey = NULL;
-
-	if (!strncmp(keyfile, "pkcs11:", 7)) {
 #ifdef CONFIG_IMA_EVM_ENGINE
-		if (!imaevm_params.keyid) {
-			log_err("When using a pkcs11 URI you must provide the keyid with an option\n");
-			return NULL;
-		}
+	EVP_PKEY *pkey;
 
-		if (keypass) {
-			if (!ENGINE_ctrl_cmd_string(imaevm_params.eng, "PIN", keypass, 0)) {
-				log_err("Failed to set the PIN for the private key\n");
-				goto err_engine;
-			}
-		}
-		pkey = ENGINE_load_private_key(imaevm_params.eng, keyfile, NULL, NULL);
-		if (!pkey) {
-			log_err("Failed to load private key %s\n", keyfile);
+	if (!keyid) {
+		log_err("When using a pkcs11 URI you must provide the keyid with an option\n");
+		return NULL;
+	}
+
+	if (keypass) {
+		if (!ENGINE_ctrl_cmd_string(e, "PIN", keypass, 0)) {
+			log_err("Failed to set the PIN for the private key\n");
 			goto err_engine;
 		}
-#else
-		log_err("OpenSSL \"engine\" support is disabled\n");
+	}
+	pkey = ENGINE_load_private_key(e, keyfile, NULL, NULL);
+	if (!pkey) {
+		log_err("Failed to load private key %s\n", keyfile);
 		goto err_engine;
+	}
+	return pkey;
+
+err_engine:
+	output_openssl_errors();
+	return NULL;
+#else
+	log_err("OpenSSL \"engine\" support is disabled\n");
+	return NULL;
 #endif
+}
+
+static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass,
+				const struct imaevm_ossl_access *access_info,
+				uint32_t keyid)
+{
+	FILE *fp;
+	EVP_PKEY *pkey = NULL;
+
+	if (!strncmp(keyfile, "pkcs11:", 7)) {
+		switch (access_info->type) {
+		case IMAEVM_OSSL_ACCESS_TYPE_ENGINE:
+			pkey = read_priv_pkey_engine(access_info->u.engine,
+						     keyfile, keypass, keyid);
+			break;
+		}
 	} else {
 		fp = fopen(keyfile, "r");
 		if (!fp) {
@@ -1076,18 +1096,17 @@  static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 
 	return pkey;
 
-err_engine:
-	output_openssl_errors();
-	return NULL;
 }
 
 #if CONFIG_SIGV1
-static RSA *read_priv_key(const char *keyfile, const char *keypass)
+static RSA *read_priv_key(const char *keyfile, const char *keypass,
+			  const struct imaevm_ossl_access *access_info,
+			  uint32_t keyid)
 {
 	EVP_PKEY *pkey;
 	RSA *key;
 
-	pkey = read_priv_pkey(keyfile, keypass);
+	pkey = read_priv_pkey(keyfile, keypass, access_info, keyid);
 	if (!pkey)
 		return NULL;
 	key = EVP_PKEY_get1_RSA(pkey);
@@ -1113,7 +1132,9 @@  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, const char *keypass,
-			unsigned char *sig)
+			unsigned char *sig,
+			const struct imaevm_ossl_access *access_info,
+			uint32_t keyid)
 {
 	int len = -1, hashalgo_idx;
 	SHA_CTX ctx;
@@ -1147,7 +1168,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, keypass);
+	key = read_priv_key(keyfile, keypass, access_info, keyid);
 	if (!key)
 		return -1;
 
@@ -1201,7 +1222,9 @@  out:
  */
 static int sign_hash_v2(const char *algo, const unsigned char *hash,
 			int size, const char *keyfile, const char *keypass,
-			unsigned char *sig)
+			unsigned char *sig,
+			const struct imaevm_ossl_access *access_info,
+			uint32_t keyid)
 {
 	struct signature_v2_hdr *hdr;
 	int len = -1;
@@ -1211,7 +1234,6 @@  static int sign_hash_v2(const char *algo, const unsigned char *hash,
 	const EVP_MD *md;
 	size_t sigsize;
 	const char *st;
-	uint32_t keyid;
 
 	if (!hash) {
 		log_err("sign_hash_v2: hash is null\n");
@@ -1236,7 +1258,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, keypass);
+	pkey = read_priv_pkey(keyfile, keypass, access_info, keyid);
 	if (!pkey)
 		return -1;
 
@@ -1259,8 +1281,8 @@  static int sign_hash_v2(const char *algo, const unsigned char *hash,
 	}
 #endif
 
-	if (imaevm_params.keyid)
-		keyid = htonl(imaevm_params.keyid);
+	if (keyid)
+		keyid = htonl(keyid);
 	else {
 		int keyid_read_failed = read_keyid_from_cert(&keyid, keyfile, false);
 
@@ -1303,17 +1325,71 @@  err:
 	return len;
 }
 
+static int check_ossl_access(const struct imaevm_ossl_access *access_info)
+{
+	switch (access_info->type) {
+	case IMAEVM_OSSL_ACCESS_TYPE_NONE:
+#ifdef CONFIG_IMA_EVM_ENGINE
+	case IMAEVM_OSSL_ACCESS_TYPE_ENGINE:
+#endif
+		return 0;
+
+	default:
+		errno = EINVAL;
+		return -1;
+	}
+}
+
+int imaevm_signhash(const char *hashalgo, const unsigned char *hash, int size,
+		    const char *keyfile, const char *keypass,
+		    unsigned char *sig, long sigflags,
+		    const struct imaevm_ossl_access *access_info,
+		    uint32_t keyid)
+{
+	int rc;
+
+	if (access_info) {
+		rc = check_ossl_access(access_info);
+		if (rc)
+			return rc;
+	}
+	if (sigflags & ~IMAEVM_SIGFLAGS_SUPPORT) {
+		/* unsupported flag */
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (sigflags & IMAEVM_SIGFLAG_SIGNATURE_V1) {
+#if CONFIG_SIGV1
+		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig,
+				    access_info, keyid);
+#else
+		log_info("Signature version 1 deprecated.");
+		return -1;
+#endif
+	}
+
+	return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig,
+			    access_info, keyid);
+}
+
 
 int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig)
 {
+	const struct imaevm_ossl_access access_info = {
+		.type = IMAEVM_OSSL_ACCESS_TYPE_ENGINE,
+		.u.engine = imaevm_params.eng,
+	};
 	if (!keypass)	/* Avoid breaking existing libimaevm usage */
 		keypass = imaevm_params.keypass;
 
 	if (imaevm_params.x509)
-		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig);
+		return sign_hash_v2(hashalgo, hash, size, keyfile, keypass, sig,
+				    &access_info, imaevm_params.keyid);
 #if CONFIG_SIGV1
 	else
-		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig);
+		return sign_hash_v1(hashalgo, hash, size, keyfile, keypass, sig,
+				    &access_info, imaevm_params.keyid);
 #endif
 	log_info("Signature version 1 deprecated.");
 	return -1;