[PATCH v4 0/5] ima-avm-utils: Convert sign v2 from RSA to EVP_PKEY API

[PATCH v4 0/5] ima-avm-utils: Convert sign v2 from RSA to EVP_PKEY API

[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(-)

-- 
2.11.0

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

[Vitaly Chikunov]

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

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

diff --git a/src/evmctl.c b/src/evmctl.c
index 15a7226..b6333bf 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -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;
 
-- 
2.11.0

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

[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 <vt@altlinux.org>
---
 src/imaevm.h    |  1 +
 src/libimaevm.c | 33 ++++++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/src/imaevm.h b/src/imaevm.h
index c81bf21..6d5eabd 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -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);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 3a9ab63..da0f422 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -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;
 }
 
-- 
2.11.0

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

[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 <vt@altlinux.org>
---
 src/libimaevm.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index da0f422..c620c1e 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -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)
 {
 
-- 
2.11.0

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

[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 <vt@altlinux.org>
---
 src/evmctl.c    |  25 ++++++----
 src/imaevm.h    |   2 +-
 src/libimaevm.c | 145 ++++++++++++++++++++++++++++++--------------------------
 3 files changed, 93 insertions(+), 79 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index b6333bf..7c398f8 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -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;
 }
 
diff --git a/src/imaevm.h b/src/imaevm.h
index 6d5eabd..8c6aeea 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -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);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index c620c1e..f9bafe3 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -452,11 +452,11 @@ struct public_key_entry {
 	struct public_key_entry *next;
 	uint32_t keyid;
 	char name[9];
-	RSA *key;
+	EVP_PKEY *key;
 };
 static struct public_key_entry *public_keys = NULL;
 
-static RSA *find_keyid(uint32_t keyid)
+static EVP_PKEY *find_keyid(uint32_t keyid)
 {
 	struct public_key_entry *entry;
 
@@ -489,7 +489,7 @@ void init_public_keys(const char *keyfiles)
 			break;
 		}
 
-		entry->key = read_pub_key(keyfile, 1);
+		entry->key = read_pub_pkey(keyfile, 1);
 		if (!entry->key) {
 			free(entry);
 			continue;
@@ -506,11 +506,11 @@ void init_public_keys(const char *keyfiles)
 int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 		   unsigned char *sig, int siglen, const char *keyfile)
 {
-	int err, len;
-	unsigned char out[1024];
-	RSA *key;
+	int err;
+	EVP_PKEY *pkey;
 	struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
-	const struct RSA_ASN1_template *asn1;
+	EVP_PKEY_CTX *ctx;
+	const EVP_MD *md;
 
 	if (params.verbose > LOG_INFO) {
 		log_info("hash: ");
@@ -518,45 +518,39 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 	}
 
 	if (public_keys) {
-		key = find_keyid(hdr->keyid);
-		if (!key) {
+		pkey = find_keyid(hdr->keyid);
+		if (!pkey) {
 			log_err("%s: unknown keyid: %x\n", file,
 				__be32_to_cpup(&hdr->keyid));
 			return -1;
 		}
 	} else {
-		key = read_pub_key(keyfile, 1);
-		if (!key)
+		pkey = read_pub_pkey(keyfile, 1);
+		if (!pkey)
 			return 1;
 	}
 
+	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
+		goto err;
+	if (!EVP_PKEY_verify_init(ctx))
+		goto err;
+	if (!(md = EVP_get_digestbyname(params.hash_algo)))
+		goto err;
+	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
+		goto err;
+	err = EVP_PKEY_verify(ctx, sig + sizeof(*hdr),
+			      siglen - sizeof(*hdr), hash, size);
+	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY_free(pkey);
 
-	err = RSA_public_decrypt(siglen - sizeof(*hdr), sig + sizeof(*hdr),
-				 out, key, RSA_PKCS1_PADDING);
-	if (err < 0) {
-		log_err("%s: RSA_public_decrypt() failed: %d\n", file, err);
-		return 1;
-	}
-
-	len = err;
-
-	asn1 = &RSA_ASN1_templates[hdr->hash_algo];
-
-	if (len < asn1->size || memcmp(out, asn1->data, asn1->size)) {
-		log_err("%s: verification failed: %d (asn1 mismatch)\n",
-			file, err);
-		return -1;
-	}
-
-	len -= asn1->size;
-
-	if (len != size || memcmp(out + asn1->size, hash, len)) {
-		log_err("%s: verification failed: %d (digest mismatch)\n",
-			file, err);
-		return -1;
-	}
-
-	return 0;
+	return err != 1;
+err:
+#ifdef USE_FPRINTF
+	ERR_print_errors_fp(stderr);
+#endif
+	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY_free(pkey);
+	return -1;
 }
 
 int get_hash_algo(const char *algo)
@@ -731,16 +725,25 @@ void calc_keyid_v1(uint8_t *keyid, char *str, const unsigned char *pkey, int len
 		log_info("keyid-v1: %s\n", str);
 }
 
-void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key)
+void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *key)
 {
+	X509_PUBKEY *pk = NULL;
 	uint8_t sha1[SHA_DIGEST_LENGTH];
-	unsigned char *pkey = NULL;
+	const unsigned char *pkey = NULL;
 	int len;
 
-	len = i2d_RSAPublicKey(key, &pkey);
-
+	/* This is more general than i2d_PublicKey() */
+	if (!X509_PUBKEY_set(&pk, key) ||
+	    !X509_PUBKEY_get0_param(NULL, &pkey, &len, NULL, pk) ||
+	    len <= 0) {
+#ifdef USE_FPRINTF
+		ERR_print_errors_fp(stderr);
+#endif
+		/* Produce invalid keyid in case of error. */
+		*keyid = 0;
+		return;
+	}
 	SHA1(pkey, len, sha1);
-
 	/* sha1[12 - 19] is exactly keyid from gpg file */
 	memcpy(keyid, sha1 + 16, 4);
 	log_debug("keyid: ");
@@ -750,7 +753,7 @@ void calc_keyid_v2(uint32_t *keyid, char *str, RSA *key)
 	if (params.verbose > LOG_INFO)
 		log_info("keyid: %s\n", str);
 
-	free(pkey);
+	X509_PUBKEY_free(pk);
 }
 
 static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
@@ -889,14 +892,16 @@ out:
 	return len;
 }
 
+/* @sig is assumed to be of MAX_SIGNATURE_SIZE size */
 int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const char *keyfile, unsigned char *sig)
 {
 	struct signature_v2_hdr *hdr;
 	int len = -1;
-	RSA *key;
+	EVP_PKEY *pkey;
 	char name[20];
-	unsigned char *buf;
-	const struct RSA_ASN1_template *asn1;
+	EVP_PKEY_CTX *ctx = NULL;
+	const EVP_MD *md;
+	size_t sigsize;
 
 	if (!hash) {
 		log_err("sign_hash_v2: hash is null\n");
@@ -921,8 +926,8 @@ int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const ch
 	log_info("hash: ");
 	log_dump(hash, size);
 
-	key = read_priv_key(keyfile, params.keypass);
-	if (!key)
+	pkey = read_priv_pkey(keyfile, params.keypass);
+	if (!pkey)
 		return -1;
 
 	hdr = (struct signature_v2_hdr *)sig;
@@ -930,32 +935,36 @@ int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const ch
 
 	hdr->hash_algo = get_hash_algo(algo);
 
-	calc_keyid_v2(&hdr->keyid, name, key);
-
-	asn1 = &RSA_ASN1_templates[hdr->hash_algo];
-
-	buf = malloc(size + asn1->size);
-	if (!buf)
-		goto out;
-
-	memcpy(buf, asn1->data, asn1->size);
-	memcpy(buf + asn1->size, hash, size);
-	len = RSA_private_encrypt(size + asn1->size, buf, hdr->sig,
-				  key, RSA_PKCS1_PADDING);
-	if (len < 0) {
-		log_err("RSA_private_encrypt() failed: %d\n", len);
-		goto out;
-	}
+	calc_keyid_v2(&hdr->keyid, name, pkey);
+
+	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
+		goto err;
+	if (!EVP_PKEY_sign_init(ctx))
+		goto err;
+	if (!(md = EVP_get_digestbyname(params.hash_algo)))
+		goto err;
+	if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
+		goto err;
+	sigsize = MAX_SIGNATURE_SIZE - sizeof(struct signature_v2_hdr);
+	if (!EVP_PKEY_sign(ctx, hdr->sig, &sigsize, hash, size))
+		goto err;
+	len = (int)sigsize;
 
 	/* we add bit length of the signature to make it gnupg compatible */
 	hdr->sig_size = __cpu_to_be16(len);
 	len += sizeof(*hdr);
 	log_info("evm/ima signature: %d bytes\n", len);
-out:
-	if (buf)
-		free(buf);
-	RSA_free(key);
+
+	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY_free(pkey);
 	return len;
+err:
+#ifdef USE_FPRINTF
+	ERR_print_errors_fp(stderr);
+#endif
+	EVP_PKEY_CTX_free(ctx);
+	EVP_PKEY_free(pkey);
+	return -1;
 }
 
 
-- 
2.11.0

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

[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 <vt@altlinux.org>
---
 src/imaevm.h    |  1 -
 src/libimaevm.c | 57 ---------------------------------------------------------
 2 files changed, 58 deletions(-)

diff --git a/src/imaevm.h b/src/imaevm.h
index 8c6aeea..31d05ae 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -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);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index f9bafe3..6458c63 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -81,63 +81,6 @@ const char *const pkey_hash_algo_kern[PKEY_HASH__LAST] = {
 	[PKEY_HASH_STREEBOG_512] = "streebog512",
 };
 
-/*
- * Hash algorithm OIDs plus ASN.1 DER wrappings [RFC4880 sec 5.2.2].
- */
-static const uint8_t RSA_digest_info_MD5[] = {
-	0x30, 0x20, 0x30, 0x0C, 0x06, 0x08,
-	0x2A, 0x86, 0x48, 0x86, 0xF7, 0x0D, 0x02, 0x05, /* OID */
-	0x05, 0x00, 0x04, 0x10
-};
-
-static const uint8_t RSA_digest_info_SHA1[] = {
-	0x30, 0x21, 0x30, 0x09, 0x06, 0x05,
-	0x2B, 0x0E, 0x03, 0x02, 0x1A,
-	0x05, 0x00, 0x04, 0x14
-};
-
-static const uint8_t RSA_digest_info_RIPE_MD_160[] = {
-	0x30, 0x21, 0x30, 0x09, 0x06, 0x05,
-	0x2B, 0x24, 0x03, 0x02, 0x01,
-	0x05, 0x00, 0x04, 0x14
-};
-
-static const uint8_t RSA_digest_info_SHA224[] = {
-	0x30, 0x2d, 0x30, 0x0d, 0x06, 0x09,
-	0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x04,
-	0x05, 0x00, 0x04, 0x1C
-};
-
-static const uint8_t RSA_digest_info_SHA256[] = {
-	0x30, 0x31, 0x30, 0x0d, 0x06, 0x09,
-	0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01,
-	0x05, 0x00, 0x04, 0x20
-};
-
-static const uint8_t RSA_digest_info_SHA384[] = {
-	0x30, 0x41, 0x30, 0x0d, 0x06, 0x09,
-	0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02,
-	0x05, 0x00, 0x04, 0x30
-};
-
-static const uint8_t RSA_digest_info_SHA512[] = {
-	0x30, 0x51, 0x30, 0x0d, 0x06, 0x09,
-	0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03,
-	0x05, 0x00, 0x04, 0x40
-};
-
-const struct RSA_ASN1_template RSA_ASN1_templates[PKEY_HASH__LAST] = {
-#define _(X) { RSA_digest_info_##X, sizeof(RSA_digest_info_##X) }
-	[PKEY_HASH_MD5]		= _(MD5),
-	[PKEY_HASH_SHA1]	= _(SHA1),
-	[PKEY_HASH_RIPE_MD_160]	= _(RIPE_MD_160),
-	[PKEY_HASH_SHA256]	= _(SHA256),
-	[PKEY_HASH_SHA384]	= _(SHA384),
-	[PKEY_HASH_SHA512]	= _(SHA512),
-	[PKEY_HASH_SHA224]	= _(SHA224),
-#undef _
-};
-
 struct libevm_params params = {
 	.verbose = LOG_INFO - 1,
 	.x509 = 1,
-- 
2.11.0

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

[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,

On Fri, Jun 14, 2019 at 04:54:05AM +0300, Vitaly Chikunov wrote:
> 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(-)
> 
> -- 
> 2.11.0

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

[Mimi Zohar]

On Fri, 2019-06-14 at 04:54 +0300, Vitaly Chikunov wrote:
> Fix off-by-one error of the output buffer passed to sign_hash().
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>  src/evmctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 15a7226..b6333bf 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -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);

Should this be "<="?

Mimi

>  	if (len <= 1)
>  		return len;
>  

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

[Mimi Zohar]

On Fri, 2019-06-14 at 04:54 +0300, Vitaly Chikunov wrote:
> 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 <vt@altlinux.org>
> ---
>  src/libimaevm.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index da0f422..c620c1e 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -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.

> 
>  	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

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.

>  	}
> 
>  	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;
> +	}

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

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

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

[Mimi Zohar]

On Fri, 2019-06-14 at 04:54 +0300, Vitaly Chikunov wrote:
> 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

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

[Vitaly Chikunov]

Mimi,

On Mon, Jun 17, 2019 at 09:34:46AM -0400, Mimi Zohar wrote:
> On Fri, 2019-06-14 at 04:54 +0300, Vitaly Chikunov wrote:
> > 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.

> 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,

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

[Mimi Zohar]

On Mon, 2019-06-17 at 17:42 +0300, Vitaly Chikunov wrote:

> > 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.

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