ima-avm-utils: Convert sign v2 from RSA to EVP_PKEY API

PATCH Superseded Vitaly Chikunov 2 participants 12 messages Jun 14, 2019 – Jun 17, 2019 Keys-trusted

Share & tools

Vitaly Chikunov

author

2019-06-14 01:54 UTC #0 lore raw

v4 cover ima-avm-utils: Convert sign v2 from RSA to EVP_PKEY API

Changes since v3:
- As suggested by Mimi Zohar this is v3 splitted into several patches to simplify review. No code changes.

Changes since v2:
- Just rebase over newer commits.

Changes since v1:
- More key neutral code in calc_keyid_v1().
- Fix uninitialized sigsize for EVP_PKEY_sign().
- Fix memory leaks for openssl types.

Vitaly Chikunov (5):
ima-avm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE
ima-avm-utils: Change read_pub_key to use EVP_PKEY API
ima-avm-utils: Change read_priv_key to use EVP_PKEY API
ima-evm-utils: Convert sign v2 from RSA to EVP_PKEY API
ima-avm-utils: Remove RSA_ASN1_templates

 src/evmctl.c    |  29 +++---
 src/imaevm.h    |   4 +-
 src/libimaevm.c | 269 ++++++++++++++++++++++++++------------------------------
 3 files changed, 146 insertions(+), 156 deletions(-)

Signature · 1 line

2.11.0

Vitaly Chikunov

author

2019-06-14 01:54 UTC #1 lore raw

PATCH v4 1/5 ima-avm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

Fix off-by-one error of the output buffer passed to sign_hash().

Signed-off-by Vitaly Chikunov <[email protected]>

---
 src/evmctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

src/evmctl.c +2 -2

index 15a7226..b6333bf 100644
@@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 static int sign_evm(const char *file, const char *key)
 {
 	unsigned char hash[MAX_DIGEST_SIZE];
-	unsigned char sig[MAX_SIGNATURE_SIZE];
+	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
 	int len, err;
 
 	len = calc_evm_hash(file, hash);

@@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
 		return len;
 
 	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
-	assert(len < sizeof(sig));
+	assert(len < MAX_SIGNATURE_SIZE);
 	if (len <= 1)
 		return len;

Signature · 1 line

2.11.0

Mimi Zohar

maintainer

2019-06-17 13:33 UTC #2 lore raw

Re: [PATCH v4 1/5] ima-avm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE

Quoted 21 lines · Vitaly Chikunov

Fix off-by-one error of the output buffer passed to sign_hash().

Signed-off-by Vitaly Chikunov <[email protected]>

---
 src/evmctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

src/evmctl.c +1 -1

index 15a7226..b6333bf 100644
@@ -510,7 +510,7 @@ static int calc_evm_hash(const char *file, unsigned char *hash)
 static int sign_evm(const char *file, const char *key)
 {
 	unsigned char hash[MAX_DIGEST_SIZE];
-	unsigned char sig[MAX_SIGNATURE_SIZE];
+	unsigned char sig[MAX_SIGNATURE_SIZE + 1];
 	int len, err;
 
 	len = calc_evm_hash(file, hash);

src/evmctl.c static int sign_evm(const char *file, const char *key) · line 522

Quoted 5 lines

src/evmctl.c +1 -1

@@ -519,7 +519,7 @@ static int sign_evm(const char *file, const char *key)
 		return len;
 
 	len = sign_hash(params.hash_algo, hash, len, key, NULL, sig + 1);
-	assert(len < sizeof(sig));
+	assert(len < MAX_SIGNATURE_SIZE);

Should this be "<="?

Mimi

Quoted 3 lines

src/evmctl.c +0 -0

 	if (len <= 1)
 		return len;

Vitaly Chikunov

author

2019-06-14 01:54 UTC #3 lore raw

PATCH v4 2/5 ima-avm-utils: Change read_pub_key to use EVP_PKEY API

In reply to Vitaly Chikunov

Introduce read_pub_pkey() to read keys using EVP_PKEY, and change read_pub_key() to be wrapper for it.

Signed-off-by Vitaly Chikunov <[email protected]>

---
 src/imaevm.h    |  1 +
 src/libimaevm.c | 33 ++++++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 11 deletions(-)

src/imaevm.h +1 -0

index c81bf21..6d5eabd 100644
@@ -216,6 +216,7 @@ int get_filesize(const char *filename);
 int ima_calc_hash(const char *file, uint8_t *hash);
 int 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);
 
 void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len);
 void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key);

src/libimaevm.c +22 ~~-11~~

index 3a9ab63..da0f422 100644
@@ -355,10 +355,9 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 	return mdlen;
 }
 
-RSA *read_pub_key(const char *keyfile, int x509)
+EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 {
 	FILE *fp;
-	RSA *key = NULL;
 	X509 *crt = NULL;
 	EVP_PKEY *pkey = NULL;

@@ -375,24 +374,36 @@ RSA *read_pub_key(const char *keyfile, int x509)
 			goto out;
 		}
 		pkey = X509_extract_key(crt);
+		X509_free(crt);
 		if (!pkey) {
 			log_err("X509_extract_key() failed\n");
 			goto out;
 		}
-		key = EVP_PKEY_get1_RSA(pkey);
 	} else {
-		key = PEM_read_RSA_PUBKEY(fp, NULL, NULL, NULL);
+		pkey = PEM_read_PUBKEY(fp, NULL, NULL, NULL);
+		if (!pkey)
+			log_err("PEM_read_PUBKEY() failed\n");
 	}
 
-	if (!key)
-		log_err("PEM_read_RSA_PUBKEY() failed\n");
-
 out:
-	if (pkey)
-		EVP_PKEY_free(pkey);
-	if (crt)
-		X509_free(crt);
 	fclose(fp);
+	return pkey;
+}
+
+RSA *read_pub_key(const char *keyfile, int x509)
+{
+	EVP_PKEY *pkey;
+	RSA *key;
+
+	pkey = read_pub_pkey(keyfile, x509);
+	if (!pkey)
+		return NULL;
+	key = EVP_PKEY_get1_RSA(pkey);
+	EVP_PKEY_free(pkey);
+	if (!key) {
+		log_err("read_pub_key: unsupported key type\n");
+		return NULL;
+	}
 	return key;
 }

Signature · 1 line

2.11.0

Vitaly Chikunov

author

2019-06-14 01:54 UTC #4 lore raw

PATCH v4 3/5 ima-avm-utils: Change read_priv_key to use EVP_PKEY API

In reply to Vitaly Chikunov

Introduce read_priv_pkey() to read keys using EVP_PKEY, and change read_priv_key() to be wrapper for it.

Signed-off-by Vitaly Chikunov <[email protected]>

---
 src/libimaevm.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

src/libimaevm.c +27 -5

index da0f422..c620c1e 100644
@@ -753,10 +753,10 @@ void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key)
 	free(pkey);
 }
 
-static RSA *read_priv_key(const char *keyfile, const char *keypass)
+static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 {
 	FILE *fp;
-	RSA *key;
+	EVP_PKEY *key;
 
 	fp = fopen(keyfile, "r");
 	if (!fp) {

@@ -764,18 +764,40 @@ static RSA *read_priv_key(const char *keyfile, const char *keypass)
 		return NULL;
 	}
 	ERR_load_crypto_strings();
-	key = PEM_read_RSAPrivateKey(fp, NULL, NULL, (void *)keypass);
+	key = PEM_read_PrivateKey(fp, NULL, NULL, (void *)keypass);
 	if (!key) {
 		char str[256];
 
-		ERR_error_string(ERR_get_error(), str);
-		log_err("PEM_read_RSAPrivateKey() failed: %s\n", str);
+		ERR_error_string(ERR_peek_error(), str);
+		log_err("PEM_read_PrivateKey() failed: %s\n", str);
+#ifdef USE_FPRINTF
+		ERR_print_errors_fp(stderr);
+#else
+		ERR_clear_error();
+#endif
 	}
 
 	fclose(fp);
 	return key;
 }
 
+static RSA *read_priv_key(const char *keyfile, const char *keypass)
+{
+	EVP_PKEY *pkey;
+	RSA *key;
+
+	pkey = read_priv_pkey(keyfile, keypass);
+	if (!pkey)
+		return NULL;
+	key = EVP_PKEY_get1_RSA(pkey);
+	EVP_PKEY_free(pkey);
+	if (!key) {
+		log_err("sign_hash_v1: unsupported key type\n");
+		return NULL;
+	}
+	return key;
+}
+
 static int get_hash_algo_v1(const char *algo)
 {

Signature · 1 line

2.11.0

Mimi Zohar

maintainer

2019-06-17 13:33 UTC #5 lore raw

Re: [PATCH v4 3/5] ima-avm-utils: Change read_priv_key to use EVP_PKEY API

Quoted 13 lines · Vitaly Chikunov

Introduce read_priv_pkey() to read keys using EVP_PKEY, and change read_priv_key() to be wrapper for it.
Signed-off-by Vitaly Chikunov <[email protected]>
---
 src/libimaevm.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)
src/libimaevm.c +0 -0
index da0f422..c620c1e 100644

src/libimaevm.c void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key) · line 759

Quoted 9 lines

src/libimaevm.c +2 -2

@@ -753,10 +753,10 @@ void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key)
 	free(pkey);
 }
 
-static RSA *read_priv_key(const char *keyfile, const char *keypass)
+static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 {
 	FILE *fp
-	RSA *key;
+	EVP_PKEY *key;

In read_pub_pkey() EVP_PKEY is named pkey, not key.

Quoted 11 lines

src/libimaevm.c +1 -1

 	fp = fopen(keyfile, "r");
 	if (!fp) {

@@ -764,18 +764,40 @@ static RSA *read_priv_key(const char *keyfile, const char *keypass)
 		return NULL;
 	}
 	ERR_load_crypto_strings();
-	key = PEM_read_RSAPrivateKey(fp, NULL, NULL, (void *)keypass);
+	key = PEM_read_PrivateKey(fp, NULL, NULL, (void *)keypass);
 	if (!key) {
 		char str[256];

src/libimaevm.c static RSA *read_priv_key(const char *keyfile, const char *keypass) · line 777

Quoted 10 lines

src/libimaevm.c +7 -2

 
-		ERR_error_string(ERR_get_error(), str);
-		log_err("PEM_read_RSAPrivateKey() failed: %s\n", str);
+		ERR_error_string(ERR_peek_error(), str);
+		log_err("PEM_read_PrivateKey() failed: %s\n", str);
+#ifdef USE_FPRINTF
+		ERR_print_errors_fp(stderr);
+#else
+		ERR_clear_error();
+#endif

Why is this additional print needed? Are you expecting multiple errors? By calling both log_err() and ERR_print_errors_fp() won't the same error message be duplicated? If calling "ERR_print_errors_fp()" is indeed needed, please make this change as a separate patch, with an appropriate patch description.

Quoted 10 lines

src/libimaevm.c +4 -0

 	}
 
 	fclose(fp);
 	return key;
 }
 
+static RSA *read_priv_key(const char *keyfile, const char *keypass)
+{
+	EVP_PKEY *pkey;
+	RSA *key;

src/libimaevm.c static RSA *read_priv_key(const char *keyfile, const char *keypass) · line 797

Quoted 10 lines

src/libimaevm.c +10 -0

+
+	pkey = read_priv_pkey(keyfile, keypass);
+	if (!pkey)
+		return NULL;
+	key = EVP_PKEY_get1_RSA(pkey);
+	EVP_PKEY_free(pkey);
+	if (!key) {
+		log_err("sign_hash_v1: unsupported key type\n");
+		return NULL;
+	}

At least at this point in the patch series, failing to get the private key isn't limited to sign_hash_v1. Perhaps that might be true later.

Mimi

Quoted 6 lines

src/libimaevm.c +3 -0

+	return key;
+}
+
 static int get_hash_algo_v1(const char *algo)
 {

Vitaly Chikunov

author

2019-06-14 01:54 UTC #6 lore raw

PATCH v4 4/5 ima-evm-utils: Convert sign v2 from RSA to EVP_PKEY API

In reply to Vitaly Chikunov

Convert sign_v2 and related to using more generic EVP_PKEY API instead of RSA API. This enables more signatures to work out of the box.

Signed-off-by Vitaly Chikunov <[email protected]>

---
 src/evmctl.c    |  25 ++++++----
 src/imaevm.h    |   2 +-
 src/libimaevm.c | 145 ++++++++++++++++++++++++++++++--------------------------
 3 files changed, 93 insertions(+), 79 deletions(-)

src/evmctl.c +15 ~~-10~~

index b6333bf..7c398f8 100644
@@ -891,7 +891,6 @@ static int cmd_import(struct command *cmd)
 	int id, len, err = 0;
 	char name[20];
 	uint8_t keyid[8];
-	RSA *key;
 
 	inkey = g_argv[optind++];
 	if (!inkey) {

@@ -925,18 +924,26 @@ static int cmd_import(struct command *cmd)
 		}
 	}
 
-	key = read_pub_key(inkey, params.x509);
-	if (!key)
-		return 1;
-
 	if (params.x509) {
+		EVP_PKEY *pkey = read_pub_pkey(inkey, params.x509);
+
+		if (!pkey)
+			return 1;
 		pub = file2bin(inkey, NULL, &len);
-		if (!pub)
-			goto out;
-		calc_keyid_v2((uint32_t *)keyid, name, key);
+		if (!pub) {
+			EVP_PKEY_free(pkey);
+			return 1;
+		}
+		calc_keyid_v2((uint32_t *)keyid, name, pkey);
+		EVP_PKEY_free(pkey);
 	} else {
+		RSA *key = read_pub_key(inkey, params.x509);
+
+		if (!key)
+			return 1;
 		len = key2bin(key, pub);
 		calc_keyid_v1(keyid, name, pub, len);
+		RSA_free(key);
 	}
 
 	log_info("Importing public key %s from file %s into keyring %d\n", name, inkey, id);

@@ -951,8 +958,6 @@ static int cmd_import(struct command *cmd)
 	}
 	if (params.x509)
 		free(pub);
-out:
-	RSA_free(key);
 	return err;
 }

src/imaevm.h +1 -1

index 6d5eabd..8c6aeea 100644
@@ -219,7 +219,7 @@ RSA *read_pub_key(const char *keyfile, int x509);
 EVP_PKEY *read_pub_pkey(const char *keyfile, int x509);
 
 void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len);
-void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key);
+void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *key);
 int key2bin(RSA *key, unsigned char *pub);
 
 int sign_hash(const char *algo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig);

src/libimaevm.c +77 ~~-68~~

228 lines not on this page · raw message

Signature · 1 line

2.11.0

Mimi Zohar

maintainer

2019-06-17 13:35 UTC #7 lore raw

Re: [PATCH v4 4/5] ima-evm-utils: Convert sign v2 from RSA to EVP_PKEY API

Quoted 2 lines · Vitaly Chikunov

Convert sign_v2 and related to using more generic EVP_PKEY API instead of RSA API. This enables more signatures to work out of the box.

Please elaborate on "enables more signatures to work out of the box", perhaps with example(s).

I like simple stories, where everything is laid out. Think of a patch set as a simple story, not a mystery, explaining how you go from point A to point B. Each patch in the series explains the next step.

The changes in cmd_import() are nice, straight forward, and easy to read. Other than the "calc_keyid_v2()" change from passing an RSA key to pkey key" dependency, this could be a separate patch. Please see if you could further break this patch up to simplify review.

Mimi

Vitaly Chikunov

author

2019-06-17 14:42 UTC #8 lore raw

Re: [PATCH v4 4/5] ima-evm-utils: Convert sign v2 from RSA to EVP_PKEY API

Mimi,

Quoted 6 lines · Mimi Zohar

Quoted 2 lines · Vitaly Chikunov
Convert sign_v2 and related to using more generic EVP_PKEY API instead of RSA API. This enables more signatures to work out of the box.
Please elaborate on "enables more signatures to work out of the box", perhaps with example(s).

Yes, message is split between next commit where it's mentioned that MD4 got fixed. I will also mention EC-RDSA. Basically, anything that OpenSSL supports will work.

Quoted 8 lines

I like simple stories, where everything is laid out. Think of a patch set as a simple story, not a mystery, explaining how you go from point A to point B. Each patch in the series explains the next step.
The changes in cmd_import() are nice, straight forward, and easy to read. Other than the "calc_keyid_v2()" change from passing an RSA key to pkey key" dependency, this could be a separate patch. Please see if you could further break this patch up to simplify review.

I thought about splitting as much as possible, but didn't not find what to split more.

- If we add read_pub_pkey into cmd_import then we can not use old calc_keyid_v2 (which wants RSA).
- If we add new calc_keyid_v2 into cmd_import it would require calling read_pub_pkey (and have sign_hash_v2 updated too).
- We can not put cmd_import change before 'rsa to pkey' change, because calc_keyid_v2 is also used in new sign_hash_v2.
- We can not put cmd_import change after 'rsa to pkey' change, because cmd_import will fail for v2 signatures (which should use PKEY).

So I don't see how this is splittable.

Thanks,

Mimi Zohar

maintainer

2019-06-17 15:10 UTC #9 lore raw

Re: [PATCH v4 4/5] ima-evm-utils: Convert sign v2 from RSA to EVP_PKEY API

Quoted 21 lines · Vitaly Chikunov

Quoted 8 lines
I like simple stories, where everything is laid out. Think of a patch set as a simple story, not a mystery, explaining how you go from point A to point B. Each patch in the series explains the next step.
The changes in cmd_import() are nice, straight forward, and easy to read. Other than the "calc_keyid_v2()" change from passing an RSA key to pkey key" dependency, this could be a separate patch. Please see if you could further break this patch up to simplify review.
I thought about splitting as much as possible, but didn't not find what to split more.
- If we add read_pub_pkey into cmd_import then we can not use old calc_keyid_v2 (which wants RSA).
- If we add new calc_keyid_v2 into cmd_import it would require calling read_pub_pkey (and have sign_hash_v2 updated too).
- We can not put cmd_import change before 'rsa to pkey' change, because calc_keyid_v2 is also used in new sign_hash_v2.
- We can not put cmd_import change after 'rsa to pkey' change, because cmd_import will fail for v2 signatures (which should use PKEY).

Quoted 1 line

So I don't see how this is splittable.

Perhaps it causes more patch churn, but there always is a way. In this case, there are a couple of ways. Instead of replacing "key" with "pkey":

- define pkey as an additional argument to calc_keyid_v2().
- define a new function named, for example, calc_keyid_v2_pkey().

Mimi

Vitaly Chikunov

author

2019-06-14 01:54 UTC #10 lore raw

PATCH v4 5/5 ima-avm-utils: Remove RSA_ASN1_templates

In reply to Vitaly Chikunov

Remove RSA_ASN1_templates[] as it does not needed anymore. OpenSSL sign is doing proper PKCS1 padding automatically (tested to be compatible with previous version, except for MD4). This also fixes bug with MD4 which produced wrong signature because of absence of the appropriate RSA_ASN1_template.

Signed-off-by Vitaly Chikunov <[email protected]>

---
 src/imaevm.h    |  1 -
 src/libimaevm.c | 57 ---------------------------------------------------------
 2 files changed, 58 deletions(-)

src/imaevm.h +0 -1

index 8c6aeea..31d05ae 100644
@@ -207,7 +207,6 @@ struct RSA_ASN1_template {
 #define	NUM_PCRS 20
 #define DEFAULT_PCR 10
 
-extern const struct RSA_ASN1_template RSA_ASN1_templates[PKEY_HASH__LAST];
 extern struct libevm_params params;
 
 void do_dump(FILE *fp, const void *ptr, int len, bool cr);

src/libimaevm.c +0 ~~-57~~

63 lines not on this page · raw message

Signature · 1 line

2.11.0

Vitaly Chikunov

author

2019-06-14 01:59 UTC #11 lore raw

In reply to Vitaly Chikunov

Mimi,

Sorry for the typo in ima-avm-utils instead of ima-evm-utils. Would you correct it if you going to apply the patches, or I can resend.

Thanks,

Quoted 26 lines · Vitaly Chikunov

Changes since v3:
- As suggested by Mimi Zohar this is v3 splitted into several patches to simplify review. No code changes.
Changes since v2:
- Just rebase over newer commits.
Changes since v1:
- More key neutral code in calc_keyid_v1().
- Fix uninitialized sigsize for EVP_PKEY_sign().
- Fix memory leaks for openssl types.
Vitaly Chikunov (5):
ima-avm-utils: Make sure sig buffer is always MAX_SIGNATURE_SIZE
ima-avm-utils: Change read_pub_key to use EVP_PKEY API
ima-avm-utils: Change read_priv_key to use EVP_PKEY API
ima-evm-utils: Convert sign v2 from RSA to EVP_PKEY API
ima-avm-utils: Remove RSA_ASN1_templates
 src/evmctl.c    |  29 +++---
 src/imaevm.h    |   4 +-
 src/libimaevm.c | 269 ++++++++++++++++++++++++++------------------------------
 3 files changed, 146 insertions(+), 156 deletions(-)
Signature · 1 line
2.11.0