[PATCH v4 0/3] ima-evm-utils: Add --keyid option

[PATCH v4 0/3] ima-evm-utils: Add --keyid option

[Vitaly Chikunov]

Allow user to set signature's keyid using `--keyid' option. Keyid should
correspond to SKID in certificate. When keyid is calculated using SHA-1
in libimaevm it may mismatch keyid extracted by the kernel from SKID of
certificate (the way public key is presented to the kernel), thus making
signatures not verifiable. This may happen when certificate is using non
SHA-1 SKID (see rfc7093) or just 'unique number' (see rfc5280 4.2.1.2).
As a last resort user may specify arbitrary keyid using the new option.
Certificate @filename could be used instead of the hex number. And,
third option is to read keyid from the cert appended to the key file.

These commits create backward incompatible ABI change for libimaevm,
 thus soname should be incremented on release.

Changes from v3:
- ima_read_keyid() is improved to better support both use cases.

Changes from v2:
- Add ima_read_keyid() function to libemaevm and use it in both evmctl
  (for --keyid) and sign_hash_v2() (for concatenated PEMs). Suggested
  by Stefan Berger.
- Autodetect PEM by openssl reading it instead of magic string.
  Suggested by Stefan Berger.
- Trivial change: s/unsigned long int/unsigned long/ for keyid type.

Changes from v1:
- Extract keyid from cert associated to key file.
- Use sizeof instead of constant.

Changes since rfc version:
- `imaevm_params.keyid' now stored as native integer (instead of network
  order). Suggested by Stefan Berger.
- Added support for `--keyid=@filename'.

Vitaly Chikunov (3):
  ima-evm-utils: Allow manual setting keyid for signing
  ima-evm-utils: Allow manual setting keyid from a cert file
  ima-evm-utils: Read keyid from the cert appended to the key file

 README                 |  5 +++
 src/evmctl.c           | 27 ++++++++++++++
 src/imaevm.h           |  2 ++
 src/libimaevm.c        | 95 ++++++++++++++++++++++++++++++++++++++++++++++++--
 tests/sign_verify.test |  2 ++
 5 files changed, 128 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH v4 1/3] ima-evm-utils: Allow manual setting keyid for signing

[Vitaly Chikunov]

Allow user to set signature's keyid using `--keyid' option. Keyid should
correspond to SKID in certificate, when keyid is calculated using SHA-1
in libimaevm it may mismatch keyid extracted by the kernel from SKID of
certificate (the way public key is presented to the kernel), thus making
signatures not verifiable. This may happen when certificate is using non
SHA-1 SKID (see rfc7093) or just 'unique number' (see rfc5280 4.2.1.2).
As a last resort user may specify arbitrary keyid using the new option.

This commit creates ABI change for libimaevm, because of adding
additional parameter to imaevm_params - newer libimaevm cannot work
with older clients.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
Reported-by: Elvira Khabirova <lineprinter0@gmail.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 README                 |  1 +
 src/evmctl.c           | 15 +++++++++++++++
 src/imaevm.h           |  1 +
 src/libimaevm.c        |  8 +++++---
 tests/sign_verify.test |  1 +
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/README b/README
index 321045d..8cd66e0 100644
--- a/README
+++ b/README
@@ -48,6 +48,7 @@ OPTIONS
       --xattr-user   store xattrs in user namespace (for testing purposes)
       --rsa          use RSA key type and signing scheme v1
   -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)
+      --keyid val    overwrite signature keyid with a value (for signing)
   -o, --portable     generate portable EVM signatures
   -p, --pass         password for encrypted signing key
   -r, --recursive    recurse into directories (sign)
diff --git a/src/evmctl.c b/src/evmctl.c
index 1815f55..8ae5488 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -2446,6 +2446,7 @@ static void usage(void)
 		"      --xattr-user   store xattrs in user namespace (for testing purposes)\n"
 		"      --rsa          use RSA key type and signing scheme v1\n"
 		"  -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)\n"
+		"      --keyid val    overwrite signature keyid with a value (for signing)\n"
 		"  -o, --portable     generate portable EVM signatures\n"
 		"  -p, --pass         password for encrypted signing key\n"
 		"  -r, --recursive    recurse into directories (sign)\n"
@@ -2525,6 +2526,7 @@ static struct option opts[] = {
 	{"xattr-user", 0, 0, 140},
 	{"ignore-violations", 0, 0, 141},
 	{"pcrs", 1, 0, 142},
+	{"keyid", 1, 0, 143},
 	{}
 
 };
@@ -2569,6 +2571,8 @@ int main(int argc, char *argv[])
 {
 	int err = 0, c, lind;
 	ENGINE *eng = NULL;
+	unsigned long keyid;
+	char *eptr;
 
 #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
 	OPENSSL_init_crypto(
@@ -2713,6 +2717,17 @@ int main(int argc, char *argv[])
 			}
 			pcrfile[npcrfile++] = optarg;
 			break;
+		case 143:
+			errno = 0;
+			keyid = strtoul(optarg, &eptr, 16);
+			if (errno || eptr - optarg != strlen(optarg) ||
+			    keyid == ULONG_MAX || keyid > UINT_MAX ||
+			    keyid == 0) {
+				log_err("Invalid keyid value.\n");
+				exit(1);
+			}
+			imaevm_params.keyid = keyid;
+			break;
 		case '?':
 			exit(1);
 			break;
diff --git a/src/imaevm.h b/src/imaevm.h
index 4503919..9f38059 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -196,6 +196,7 @@ struct libimaevm_params {
 	const char *hash_algo;
 	const char *keyfile;
 	const char *keypass;
+	uint32_t keyid;		/* keyid overriding value, unless 0. */
 };
 
 struct RSA_ASN1_template {
diff --git a/src/libimaevm.c b/src/libimaevm.c
index fa6c278..481d29d 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -45,6 +45,7 @@
 #include <sys/param.h>
 #include <sys/stat.h>
 #include <asm/byteorder.h>
+#include <arpa/inet.h>
 #include <unistd.h>
 #include <dirent.h>
 #include <string.h>
@@ -894,7 +895,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");
@@ -932,8 +932,10 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 		return -1;
 	}
 
-	calc_keyid_v2(&keyid, name, pkey);
-	hdr->keyid = keyid;
+	if (imaevm_params.keyid)
+		hdr->keyid = htonl(imaevm_params.keyid);
+	else
+		calc_keyid_v2(&hdr->keyid, name, pkey);
 
 	st = "EVP_PKEY_CTX_new";
 	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
diff --git a/tests/sign_verify.test b/tests/sign_verify.test
index 288e133..2c21812 100755
--- a/tests/sign_verify.test
+++ b/tests/sign_verify.test
@@ -359,6 +359,7 @@ sign_verify  rsa1024  sha256  0x0301 --rsa
 sign_verify  rsa1024  md5     0x030201:K:0080
 sign_verify  rsa1024  sha1    0x030202:K:0080
 sign_verify  rsa1024  sha224  0x030207:K:0080
+expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204aabbccdd0080 OPTS=--keyid=aabbccdd
 sign_verify  rsa1024  sha256  0x030204:K:0080
   try_different_keys
   try_different_sigs
-- 
2.11.0

[PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file

[Vitaly Chikunov]

Allow user to specify `--keyid @/path/to/cert.pem' to extract keyid from
SKID of the certificate file. PEM or DER format is auto-detected.

`--keyid' option is reused instead of adding a new option (like possible
`--cert') to signify to the user it's only keyid extraction and nothing
more.

This commit creates ABI change for libimaevm, due to adding new function
ima_read_keyid(). Newer clients cannot work with older libimaevm.
Together with previous commit it creates backward-incompatible ABI
change, thus soname should be incremented on release.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 README                 |  1 +
 src/evmctl.c           | 22 ++++++++++---
 src/imaevm.h           |  1 +
 src/libimaevm.c        | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/sign_verify.test |  1 +
 5 files changed, 105 insertions(+), 5 deletions(-)

diff --git a/README b/README
index 8cd66e0..0e1f6ba 100644
--- a/README
+++ b/README
@@ -49,6 +49,7 @@ OPTIONS
       --rsa          use RSA key type and signing scheme v1
   -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)
       --keyid val    overwrite signature keyid with a value (for signing)
+                     val is a x509 cert file if prefixed with '@'
   -o, --portable     generate portable EVM signatures
   -p, --pass         password for encrypted signing key
   -r, --recursive    recurse into directories (sign)
diff --git a/src/evmctl.c b/src/evmctl.c
index 8ae5488..6a60599 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -42,6 +42,7 @@
 #include <sys/param.h>
 #include <sys/stat.h>
 #include <sys/ioctl.h>
+#include <arpa/inet.h>
 #include <fcntl.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -57,12 +58,14 @@
 #include <termios.h>
 #include <assert.h>
 
+#include <openssl/asn1.h>
 #include <openssl/sha.h>
 #include <openssl/pem.h>
 #include <openssl/hmac.h>
 #include <openssl/err.h>
 #include <openssl/rsa.h>
 #include <openssl/engine.h>
+#include <openssl/x509v3.h>
 #include "hash_info.h"
 #include "pcr.h"
 #include "utils.h"
@@ -2447,6 +2450,7 @@ static void usage(void)
 		"      --rsa          use RSA key type and signing scheme v1\n"
 		"  -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)\n"
 		"      --keyid val    overwrite signature keyid with a value (for signing)\n"
+		"                     val is a x509 cert file if prefixed with '@'\n"
 		"  -o, --portable     generate portable EVM signatures\n"
 		"  -p, --pass         password for encrypted signing key\n"
 		"  -r, --recursive    recurse into directories (sign)\n"
@@ -2572,7 +2576,6 @@ int main(int argc, char *argv[])
 	int err = 0, c, lind;
 	ENGINE *eng = NULL;
 	unsigned long keyid;
-	char *eptr;
 
 #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
 	OPENSSL_init_crypto(
@@ -2718,10 +2721,19 @@ int main(int argc, char *argv[])
 			pcrfile[npcrfile++] = optarg;
 			break;
 		case 143:
-			errno = 0;
-			keyid = strtoul(optarg, &eptr, 16);
-			if (errno || eptr - optarg != strlen(optarg) ||
-			    keyid == ULONG_MAX || keyid > UINT_MAX ||
+			if (optarg[0] == '@') {
+				keyid = ima_read_keyid(optarg + 1, NULL);
+			} else {
+				char *eptr;
+
+				errno = 0;
+				keyid = strtoul(optarg, &eptr, 16);
+				if (eptr - optarg != strlen(optarg) || errno) {
+					log_err("Invalid keyid value.\n");
+					exit(1);
+				}
+			}
+			if (keyid == ULONG_MAX || keyid > UINT_MAX ||
 			    keyid == 0) {
 				log_err("Invalid keyid value.\n");
 				exit(1);
diff --git a/src/imaevm.h b/src/imaevm.h
index 9f38059..eab7f32 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -219,6 +219,7 @@ 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, EVP_PKEY *pkey);
 int key2bin(RSA *key, unsigned char *pub);
+unsigned long ima_read_keyid(const char *certfile, uint32_t *keyid);
 
 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);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 481d29d..a22d9bb 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -57,6 +57,7 @@
 #include <openssl/pem.h>
 #include <openssl/evp.h>
 #include <openssl/x509.h>
+#include <openssl/x509v3.h>
 #include <openssl/err.h>
 
 #include "imaevm.h"
@@ -748,6 +749,90 @@ void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey)
 	X509_PUBKEY_free(pk);
 }
 
+enum keyid_file_type {
+	KEYID_FILE_PEM_KEY = 0,
+	KEYID_FILE_UNK_CERT,
+};
+
+/*
+ * @is_cert: 1 - this is a x509 cert file (maybe PEM, maybe DER encoded);
+ *           0 - this is a PEM encoded private key file with possible appended
+ *           x509 cert.
+ */
+static unsigned long _ima_read_keyid(const char *certfile, uint32_t *keyid,
+				     enum keyid_file_type file_type)
+{
+	uint32_t keyid_raw;
+	const ASN1_OCTET_STRING *skid;
+	int skid_len;
+	X509 *x = NULL;
+	FILE *fp;
+
+	if (!(fp = fopen(certfile, "r"))) {
+		log_err("read keyid: %s: Cannot open: %s\n", certfile,
+			strerror(errno));
+		return -1;
+	}
+	if (!PEM_read_X509(fp, &x, NULL, NULL)) {
+		if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) {
+			ERR_clear_error();
+			if (file_type == KEYID_FILE_PEM_KEY) {
+				log_debug("%s: x509 certificate not found\n",
+					  certfile);
+				fclose(fp);
+				return -1;
+			}
+			rewind(fp);
+			d2i_X509_fp(fp, &x);
+		}
+		if (!x) {
+			ERR_print_errors_fp(stderr);
+			log_err("read keyid: %s: Error reading x509 certificate\n",
+				certfile);
+			fclose(fp);
+			return -1;
+		}
+	}
+	fclose(fp);
+
+	if (!(skid = X509_get0_subject_key_id(x))) {
+		log_err("read keyid: %s: SKID not found\n", certfile);
+		goto err_free;
+	}
+	skid_len = ASN1_STRING_length(skid);
+	if (skid_len < sizeof(keyid_raw)) {
+		log_err("read keyid: %s: SKID too short (len %d)\n", certfile,
+			skid_len);
+		goto err_free;
+	}
+	memcpy(&keyid_raw, ASN1_STRING_get0_data(skid) + skid_len
+	       - sizeof(keyid_raw), sizeof(keyid_raw));
+	if (keyid)
+		memcpy(keyid, &keyid_raw, sizeof(*keyid));
+	log_info("keyid %04x (from %s)\n", ntohl(keyid_raw), certfile);
+	return ntohl(keyid_raw);
+
+err_free:
+	X509_free(x);
+	return -1;
+}
+
+/**
+ * ima_read_keyid() - Read 32-bit keyid from the cert file.
+ * @certfile:	File possibly containing certificate in DER/PEM format.
+ * @keyid:	Output keyid in network order.
+ *
+ * Try to read keyid from Subject Key Identifier (SKID) of certificate.
+ * Autodetect if cert is in PEM or DER encoding.
+ *
+ * Return: -1 (ULONG_MAX) on error;
+ *         32-bit keyid as unsigned integer in host order.
+ */
+unsigned long ima_read_keyid(const char *certfile, uint32_t *keyid)
+{
+	return _ima_read_keyid(certfile, keyid, KEYID_FILE_UNK_CERT);
+}
+
 static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 {
 	FILE *fp;
diff --git a/tests/sign_verify.test b/tests/sign_verify.test
index 2c21812..52ea33a 100755
--- a/tests/sign_verify.test
+++ b/tests/sign_verify.test
@@ -360,6 +360,7 @@ sign_verify  rsa1024  md5     0x030201:K:0080
 sign_verify  rsa1024  sha1    0x030202:K:0080
 sign_verify  rsa1024  sha224  0x030207:K:0080
 expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204aabbccdd0080 OPTS=--keyid=aabbccdd
+expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204:K:0080 OPTS=--keyid=@test-rsa1024.cer
 sign_verify  rsa1024  sha256  0x030204:K:0080
   try_different_keys
   try_different_sigs
-- 
2.11.0

[PATCH v4 3/3] ima-evm-utils: Read keyid from the cert appended to the key file

[Vitaly Chikunov]

Allow to have certificate appended to the private key of `--key'
specified (PEM) file (for v2 signing) to facilitate reading of keyid
from the associated cert. This will allow users to have private and
public key as a single file. There is no check that public key form the
cert matches associated private key.

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

diff --git a/README b/README
index 0e1f6ba..ea11bde 100644
--- a/README
+++ b/README
@@ -127,6 +127,9 @@ for signing and importing the key.
 Second key format uses X509 DER encoded public key certificates and uses asymmetric key support
 in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default).
 
+For v2 signatures x509 certificate with the public key could be appended to the private
+key (both are in PEM format) to properly determine its Subject Key Identifier (SKID).
+
 
 Integrity keyrings
 ----------------
diff --git a/src/libimaevm.c b/src/libimaevm.c
index a22d9bb..ac4bb46 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
 		return -1;
 	}
 
-	if (imaevm_params.keyid)
+	if (imaevm_params.keyid) {
 		hdr->keyid = htonl(imaevm_params.keyid);
-	else
-		calc_keyid_v2(&hdr->keyid, name, pkey);
+	} else {
+		if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX)
+			calc_keyid_v2(&hdr->keyid, name, pkey);
+	}
 
 	st = "EVP_PKEY_CTX_new";
 	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
-- 
2.11.0

Re: [PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file

[Stefan Berger]

On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
> Allow user to specify `--keyid @/path/to/cert.pem' to extract keyid from
> SKID of the certificate file. PEM or DER format is auto-detected.
>
> `--keyid' option is reused instead of adding a new option (like possible
> `--cert') to signify to the user it's only keyid extraction and nothing
> more.
>
> This commit creates ABI change for libimaevm, due to adding new function
> ima_read_keyid(). Newer clients cannot work with older libimaevm.
> Together with previous commit it creates backward-incompatible ABI
> change, thus soname should be incremented on release.
>
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>   README                 |  1 +
>   src/evmctl.c           | 22 ++++++++++---
>   src/imaevm.h           |  1 +
>   src/libimaevm.c        | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   tests/sign_verify.test |  1 +
>   5 files changed, 105 insertions(+), 5 deletions(-)
>
> diff --git a/README b/README
> index 8cd66e0..0e1f6ba 100644
> --- a/README
> +++ b/README
> @@ -49,6 +49,7 @@ OPTIONS
>         --rsa          use RSA key type and signing scheme v1
>     -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)
>         --keyid val    overwrite signature keyid with a value (for signing)
> +                     val is a x509 cert file if prefixed with '@'
>     -o, --portable     generate portable EVM signatures
>     -p, --pass         password for encrypted signing key
>     -r, --recursive    recurse into directories (sign)
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 8ae5488..6a60599 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -42,6 +42,7 @@
>   #include <sys/param.h>
>   #include <sys/stat.h>
>   #include <sys/ioctl.h>
> +#include <arpa/inet.h>
>   #include <fcntl.h>
>   #include <unistd.h>
>   #include <stdlib.h>
> @@ -57,12 +58,14 @@
>   #include <termios.h>
>   #include <assert.h>
>   
> +#include <openssl/asn1.h>
>   #include <openssl/sha.h>
>   #include <openssl/pem.h>
>   #include <openssl/hmac.h>
>   #include <openssl/err.h>
>   #include <openssl/rsa.h>
>   #include <openssl/engine.h>
> +#include <openssl/x509v3.h>
>   #include "hash_info.h"
>   #include "pcr.h"
>   #include "utils.h"
> @@ -2447,6 +2450,7 @@ static void usage(void)
>   		"      --rsa          use RSA key type and signing scheme v1\n"
>   		"  -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)\n"
>   		"      --keyid val    overwrite signature keyid with a value (for signing)\n"
> +		"                     val is a x509 cert file if prefixed with '@'\n"
>   		"  -o, --portable     generate portable EVM signatures\n"
>   		"  -p, --pass         password for encrypted signing key\n"
>   		"  -r, --recursive    recurse into directories (sign)\n"
> @@ -2572,7 +2576,6 @@ int main(int argc, char *argv[])
>   	int err = 0, c, lind;
>   	ENGINE *eng = NULL;
>   	unsigned long keyid;
> -	char *eptr;
>   
>   #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
>   	OPENSSL_init_crypto(
> @@ -2718,10 +2721,19 @@ int main(int argc, char *argv[])
>   			pcrfile[npcrfile++] = optarg;
>   			break;
>   		case 143:
> -			errno = 0;
> -			keyid = strtoul(optarg, &eptr, 16);
> -			if (errno || eptr - optarg != strlen(optarg) ||
> -			    keyid == ULONG_MAX || keyid > UINT_MAX ||
> +			if (optarg[0] == '@') {
> +				keyid = ima_read_keyid(optarg + 1, NULL);
> +			} else {
> +				char *eptr;
> +
> +				errno = 0;
> +				keyid = strtoul(optarg, &eptr, 16);
> +				if (eptr - optarg != strlen(optarg) || errno) {
> +					log_err("Invalid keyid value.\n");
> +					exit(1);
> +				}
> +			}
> +			if (keyid == ULONG_MAX || keyid > UINT_MAX ||
>   			    keyid == 0) {
>   				log_err("Invalid keyid value.\n");
>   				exit(1);
> diff --git a/src/imaevm.h b/src/imaevm.h
> index 9f38059..eab7f32 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -219,6 +219,7 @@ 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, EVP_PKEY *pkey);
>   int key2bin(RSA *key, unsigned char *pub);
> +unsigned long ima_read_keyid(const char *certfile, uint32_t *keyid);
>   
>   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);
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 481d29d..a22d9bb 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -57,6 +57,7 @@
>   #include <openssl/pem.h>
>   #include <openssl/evp.h>
>   #include <openssl/x509.h>
> +#include <openssl/x509v3.h>
>   #include <openssl/err.h>
>   
>   #include "imaevm.h"
> @@ -748,6 +749,90 @@ void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey)
>   	X509_PUBKEY_free(pk);
>   }
>   
> +enum keyid_file_type {
> +	KEYID_FILE_PEM_KEY = 0,
> +	KEYID_FILE_UNK_CERT,
> +};
> +
> +/*
> + * @is_cert: 1 - this is a x509 cert file (maybe PEM, maybe DER encoded);
This probably change since you wrote it. Is this file_type now?
> + *           0 - this is a PEM encoded private key file with possible appended
> + *           x509 cert.
> + */
> +static unsigned long _ima_read_keyid(const char *certfile, uint32_t *keyid,
> +				     enum keyid_file_type file_type)
> +{
> +	uint32_t keyid_raw;
> +	const ASN1_OCTET_STRING *skid;
> +	int skid_len;
> +	X509 *x = NULL;
> +	FILE *fp;
> +
> +	if (!(fp = fopen(certfile, "r"))) {
> +		log_err("read keyid: %s: Cannot open: %s\n", certfile,
> +			strerror(errno));
> +		return -1;
> +	}
> +	if (!PEM_read_X509(fp, &x, NULL, NULL)) {
> +		if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) {
> +			ERR_clear_error();
> +			if (file_type == KEYID_FILE_PEM_KEY) {
> +				log_debug("%s: x509 certificate not found\n",
> +					  certfile);
> +				fclose(fp);
> +				return -1;
> +			}
> +			rewind(fp);
> +			d2i_X509_fp(fp, &x);
> +		}
> +		if (!x) {
> +			ERR_print_errors_fp(stderr);
> +			log_err("read keyid: %s: Error reading x509 certificate\n",
> +				certfile);
> +			fclose(fp);
> +			return -1;
> +		}
> +	}
> +	fclose(fp);
> +
> +	if (!(skid = X509_get0_subject_key_id(x))) {
> +		log_err("read keyid: %s: SKID not found\n", certfile);
> +		goto err_free;
> +	}
> +	skid_len = ASN1_STRING_length(skid);
> +	if (skid_len < sizeof(keyid_raw)) {
> +		log_err("read keyid: %s: SKID too short (len %d)\n", certfile,
> +			skid_len);
> +		goto err_free;
> +	}
> +	memcpy(&keyid_raw, ASN1_STRING_get0_data(skid) + skid_len
> +	       - sizeof(keyid_raw), sizeof(keyid_raw));
> +	if (keyid)
> +		memcpy(keyid, &keyid_raw, sizeof(*keyid));


If keyid is supposed to be in native format then you have to apply 
ntohl() to it..


> +	log_info("keyid %04x (from %s)\n", ntohl(keyid_raw), certfile);
> +	return ntohl(keyid_raw);

I thought this should return 0  ?


> +
> +err_free:
> +	X509_free(x);
> +	return -1;
> +}
> +
> +/**
> + * ima_read_keyid() - Read 32-bit keyid from the cert file.
> + * @certfile:	File possibly containing certificate in DER/PEM format.
> + * @keyid:	Output keyid in network order.
> + *
> + * Try to read keyid from Subject Key Identifier (SKID) of certificate.
> + * Autodetect if cert is in PEM or DER encoding.
> + *
> + * Return: -1 (ULONG_MAX) on error;
> + *         32-bit keyid as unsigned integer in host order.
That's confusing, two times the same result, one time in host order, on 
time in network order. Why not just one return value in host order?
> + */
> +unsigned long ima_read_keyid(const char *certfile, uint32_t *keyid)
> +{
> +	return _ima_read_keyid(certfile, keyid, KEYID_FILE_UNK_CERT);
> +}
> +
>   static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
>   {
>   	FILE *fp;
> diff --git a/tests/sign_verify.test b/tests/sign_verify.test
> index 2c21812..52ea33a 100755
> --- a/tests/sign_verify.test
> +++ b/tests/sign_verify.test
> @@ -360,6 +360,7 @@ sign_verify  rsa1024  md5     0x030201:K:0080
>   sign_verify  rsa1024  sha1    0x030202:K:0080
>   sign_verify  rsa1024  sha224  0x030207:K:0080
>   expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204aabbccdd0080 OPTS=--keyid=aabbccdd
> +expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204:K:0080 OPTS=--keyid=@test-rsa1024.cer
>   sign_verify  rsa1024  sha256  0x030204:K:0080
>     try_different_keys
>     try_different_sigs

Re: [PATCH v4 3/3] ima-evm-utils: Read keyid from the cert appended to the key file

[Stefan Berger]

On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
> Allow to have certificate appended to the private key of `--key'
> specified (PEM) file (for v2 signing) to facilitate reading of keyid
> from the associated cert. This will allow users to have private and
> public key as a single file. There is no check that public key form the
> cert matches associated private key.
>
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>   README          | 3 +++
>   src/libimaevm.c | 8 +++++---
>   2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/README b/README
> index 0e1f6ba..ea11bde 100644
> --- a/README
> +++ b/README
> @@ -127,6 +127,9 @@ for signing and importing the key.
>   Second key format uses X509 DER encoded public key certificates and uses asymmetric key support
>   in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default).
>   
> +For v2 signatures x509 certificate with the public key could be appended to the private
> +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID).
> +
>   
>   Integrity keyrings
>   ----------------
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index a22d9bb..ac4bb46 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
>   		return -1;
>   	}
>   
> -	if (imaevm_params.keyid)
> +	if (imaevm_params.keyid) {
>   		hdr->keyid = htonl(imaevm_params.keyid);
> -	else
> -		calc_keyid_v2(&hdr->keyid, name, pkey);
> +	} else {
> +		if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX)
> +			calc_keyid_v2(&hdr->keyid, name, pkey);
> +	}

It might be convenient here to just write the result in network byte 
order into the header but for a library API I find it not so nice, but 
then there's calc_keyid_v2 that does that already... I just wouldn't 
expect that these parameter are in big endian order already, I would 
expect them in native order. So maybe ima_read_keyid should just return 
ULONG_MAX or the keyid in host order and it call _ima_read_keyid() with 
a NULL pointer or a dummy variable as keyid that the library API caller 
doesn't see.

    Stefan


>   
>   	st = "EVP_PKEY_CTX_new";
>   	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))

Re: [PATCH v4 3/3] ima-evm-utils: Read keyid from the cert appended to the key file

[Vitaly Chikunov]

Stefan,

On Wed, May 05, 2021 at 07:29:17PM -0400, Stefan Berger wrote:
> 
> On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
> > Allow to have certificate appended to the private key of `--key'
> > specified (PEM) file (for v2 signing) to facilitate reading of keyid
> > from the associated cert. This will allow users to have private and
> > public key as a single file. There is no check that public key form the
> > cert matches associated private key.
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > ---
> >   README          | 3 +++
> >   src/libimaevm.c | 8 +++++---
> >   2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/README b/README
> > index 0e1f6ba..ea11bde 100644
> > --- a/README
> > +++ b/README
> > @@ -127,6 +127,9 @@ for signing and importing the key.
> >   Second key format uses X509 DER encoded public key certificates and uses asymmetric key support
> >   in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default).
> > +For v2 signatures x509 certificate with the public key could be appended to the private
> > +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID).
> > +
> >   Integrity keyrings
> >   ----------------
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index a22d9bb..ac4bb46 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
> >   		return -1;
> >   	}
> > -	if (imaevm_params.keyid)
> > +	if (imaevm_params.keyid) {
> >   		hdr->keyid = htonl(imaevm_params.keyid);
> > -	else
> > -		calc_keyid_v2(&hdr->keyid, name, pkey);
> > +	} else {
> > +		if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX)
> > +			calc_keyid_v2(&hdr->keyid, name, pkey);
> > +	}
> 
> It might be convenient here to just write the result in network byte order
> into the header but for a library API I find it not so nice, but then
> there's calc_keyid_v2 that does that already... I just wouldn't expect that
> these parameter are in big endian order already, I would expect them in
> native order. 

I expect them in network order, similar to how calc_keyid_v2() already
writes it one line below. So, why should we mix orders? Both functions
write keyids, so it's not like completely different parts of API. Also,
it's documented that ima_read_keyid() writes to the pointer in network
order (and returns integer in host order), so I don't see the
problem. Thus, I would prefer not to follow this suggestion.

Thanks,

> So maybe ima_read_keyid should just return ULONG_MAX or the
> keyid in host order and it call _ima_read_keyid() with a NULL pointer or a
> dummy variable as keyid that the library API caller doesn't see.
> 
>    Stefan
> 
> 
> >   	st = "EVP_PKEY_CTX_new";
> >   	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))

Re: [PATCH v4 3/3] ima-evm-utils: Read keyid from the cert appended to the key file

[Vitaly Chikunov]

Stefan,

On Thu, May 06, 2021 at 03:29:32AM +0300, Vitaly Chikunov wrote:
> On Wed, May 05, 2021 at 07:29:17PM -0400, Stefan Berger wrote:
> > 
> > On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
> > > Allow to have certificate appended to the private key of `--key'
> > > specified (PEM) file (for v2 signing) to facilitate reading of keyid
> > > from the associated cert. This will allow users to have private and
> > > public key as a single file. There is no check that public key form the
> > > cert matches associated private key.
> > > 
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > ---
> > >   README          | 3 +++
> > >   src/libimaevm.c | 8 +++++---
> > >   2 files changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/README b/README
> > > index 0e1f6ba..ea11bde 100644
> > > --- a/README
> > > +++ b/README
> > > @@ -127,6 +127,9 @@ for signing and importing the key.
> > >   Second key format uses X509 DER encoded public key certificates and uses asymmetric key support
> > >   in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default).
> > > +For v2 signatures x509 certificate with the public key could be appended to the private
> > > +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID).
> > > +
> > >   Integrity keyrings
> > >   ----------------
> > > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > > index a22d9bb..ac4bb46 100644
> > > --- a/src/libimaevm.c
> > > +++ b/src/libimaevm.c
> > > @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
> > >   		return -1;
> > >   	}
> > > -	if (imaevm_params.keyid)
> > > +	if (imaevm_params.keyid) {
> > >   		hdr->keyid = htonl(imaevm_params.keyid);
> > > -	else
> > > -		calc_keyid_v2(&hdr->keyid, name, pkey);
> > > +	} else {
> > > +		if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX)
> > > +			calc_keyid_v2(&hdr->keyid, name, pkey);
> > > +	}
> > 
> > It might be convenient here to just write the result in network byte order
> > into the header but for a library API I find it not so nice, but then
> > there's calc_keyid_v2 that does that already... I just wouldn't expect that
> > these parameter are in big endian order already, I would expect them in
> > native order. 
> 
> I expect them in network order, similar to how calc_keyid_v2() already
> writes it one line below. So, why should we mix orders? Both functions
> write keyids, so it's not like completely different parts of API. Also,
> it's documented that ima_read_keyid() writes to the pointer in network
> order (and returns integer in host order), so I don't see the
> problem. Thus, I would prefer not to follow this suggestion.
> 
> Thanks,
> 
> > So maybe ima_read_keyid should just return ULONG_MAX or the
> > keyid in host order and it call _ima_read_keyid() with a NULL pointer or a
> > dummy variable as keyid that the library API caller doesn't see.

So there will be exported
   uint32_t ima_read_keyid(char *certfile);
and internal
   static bool _read_keyid(char *certfile, uint32_t *keyid)

ima_read_keyid will wrap read_keyid and return keyid via intermediate
variable. Right?


> > 
> >    Stefan
> > 
> > 
> > >   	st = "EVP_PKEY_CTX_new";
> > >   	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))

Re: [PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file

[Vitaly Chikunov]

Stefan,

On Wed, May 05, 2021 at 07:13:39PM -0400, Stefan Berger wrote:
> 
> On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
> > Allow user to specify `--keyid @/path/to/cert.pem' to extract keyid from
> > SKID of the certificate file. PEM or DER format is auto-detected.
> > 
> > `--keyid' option is reused instead of adding a new option (like possible
> > `--cert') to signify to the user it's only keyid extraction and nothing
> > more.
> > 
> > This commit creates ABI change for libimaevm, due to adding new function
> > ima_read_keyid(). Newer clients cannot work with older libimaevm.
> > Together with previous commit it creates backward-incompatible ABI
> > change, thus soname should be incremented on release.
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > ---
> >   README                 |  1 +
> >   src/evmctl.c           | 22 ++++++++++---
> >   src/imaevm.h           |  1 +
> >   src/libimaevm.c        | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   tests/sign_verify.test |  1 +
> >   5 files changed, 105 insertions(+), 5 deletions(-)
> > 
> > diff --git a/README b/README
> > index 8cd66e0..0e1f6ba 100644
> > --- a/README
> > +++ b/README
> > @@ -49,6 +49,7 @@ OPTIONS
> >         --rsa          use RSA key type and signing scheme v1
> >     -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)
> >         --keyid val    overwrite signature keyid with a value (for signing)
> > +                     val is a x509 cert file if prefixed with '@'
> >     -o, --portable     generate portable EVM signatures
> >     -p, --pass         password for encrypted signing key
> >     -r, --recursive    recurse into directories (sign)
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 8ae5488..6a60599 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -42,6 +42,7 @@
> >   #include <sys/param.h>
> >   #include <sys/stat.h>
> >   #include <sys/ioctl.h>
> > +#include <arpa/inet.h>
> >   #include <fcntl.h>
> >   #include <unistd.h>
> >   #include <stdlib.h>
> > @@ -57,12 +58,14 @@
> >   #include <termios.h>
> >   #include <assert.h>
> > +#include <openssl/asn1.h>
> >   #include <openssl/sha.h>
> >   #include <openssl/pem.h>
> >   #include <openssl/hmac.h>
> >   #include <openssl/err.h>
> >   #include <openssl/rsa.h>
> >   #include <openssl/engine.h>
> > +#include <openssl/x509v3.h>
> >   #include "hash_info.h"
> >   #include "pcr.h"
> >   #include "utils.h"
> > @@ -2447,6 +2450,7 @@ static void usage(void)
> >   		"      --rsa          use RSA key type and signing scheme v1\n"
> >   		"  -k, --key          path to signing key (default: /etc/keys/{privkey,pubkey}_evm.pem)\n"
> >   		"      --keyid val    overwrite signature keyid with a value (for signing)\n"
> > +		"                     val is a x509 cert file if prefixed with '@'\n"
> >   		"  -o, --portable     generate portable EVM signatures\n"
> >   		"  -p, --pass         password for encrypted signing key\n"
> >   		"  -r, --recursive    recurse into directories (sign)\n"
> > @@ -2572,7 +2576,6 @@ int main(int argc, char *argv[])
> >   	int err = 0, c, lind;
> >   	ENGINE *eng = NULL;
> >   	unsigned long keyid;
> > -	char *eptr;
> >   #if !(OPENSSL_VERSION_NUMBER < 0x10100000)
> >   	OPENSSL_init_crypto(
> > @@ -2718,10 +2721,19 @@ int main(int argc, char *argv[])
> >   			pcrfile[npcrfile++] = optarg;
> >   			break;
> >   		case 143:
> > -			errno = 0;
> > -			keyid = strtoul(optarg, &eptr, 16);
> > -			if (errno || eptr - optarg != strlen(optarg) ||
> > -			    keyid == ULONG_MAX || keyid > UINT_MAX ||
> > +			if (optarg[0] == '@') {
> > +				keyid = ima_read_keyid(optarg + 1, NULL);
> > +			} else {
> > +				char *eptr;
> > +
> > +				errno = 0;
> > +				keyid = strtoul(optarg, &eptr, 16);
> > +				if (eptr - optarg != strlen(optarg) || errno) {
> > +					log_err("Invalid keyid value.\n");
> > +					exit(1);
> > +				}
> > +			}
> > +			if (keyid == ULONG_MAX || keyid > UINT_MAX ||
> >   			    keyid == 0) {
> >   				log_err("Invalid keyid value.\n");
> >   				exit(1);
> > diff --git a/src/imaevm.h b/src/imaevm.h
> > index 9f38059..eab7f32 100644
> > --- a/src/imaevm.h
> > +++ b/src/imaevm.h
> > @@ -219,6 +219,7 @@ 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, EVP_PKEY *pkey);
> >   int key2bin(RSA *key, unsigned char *pub);
> > +unsigned long ima_read_keyid(const char *certfile, uint32_t *keyid);
> >   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);
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index 481d29d..a22d9bb 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -57,6 +57,7 @@
> >   #include <openssl/pem.h>
> >   #include <openssl/evp.h>
> >   #include <openssl/x509.h>
> > +#include <openssl/x509v3.h>
> >   #include <openssl/err.h>
> >   #include "imaevm.h"
> > @@ -748,6 +749,90 @@ void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey)
> >   	X509_PUBKEY_free(pk);
> >   }
> > +enum keyid_file_type {
> > +	KEYID_FILE_PEM_KEY = 0,
> > +	KEYID_FILE_UNK_CERT,
> > +};
> > +
> > +/*
> > + * @is_cert: 1 - this is a x509 cert file (maybe PEM, maybe DER encoded);
>
> This probably change since you wrote it. Is this file_type now?

Yes. (This is a consequence of making generic function out of N smaller
functions- it's very hard to invent distinguisher that user could easily
or understand at all.)

> > + *           0 - this is a PEM encoded private key file with possible appended
> > + *           x509 cert.
> > + */
> > +static unsigned long _ima_read_keyid(const char *certfile, uint32_t *keyid,
> > +				     enum keyid_file_type file_type)
> > +{
> > +	uint32_t keyid_raw;
> > +	const ASN1_OCTET_STRING *skid;
> > +	int skid_len;
> > +	X509 *x = NULL;
> > +	FILE *fp;
> > +
> > +	if (!(fp = fopen(certfile, "r"))) {
> > +		log_err("read keyid: %s: Cannot open: %s\n", certfile,
> > +			strerror(errno));
> > +		return -1;
> > +	}
> > +	if (!PEM_read_X509(fp, &x, NULL, NULL)) {
> > +		if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) {
> > +			ERR_clear_error();
> > +			if (file_type == KEYID_FILE_PEM_KEY) {
> > +				log_debug("%s: x509 certificate not found\n",
> > +					  certfile);
> > +				fclose(fp);
> > +				return -1;
> > +			}
> > +			rewind(fp);
> > +			d2i_X509_fp(fp, &x);
> > +		}
> > +		if (!x) {
> > +			ERR_print_errors_fp(stderr);
> > +			log_err("read keyid: %s: Error reading x509 certificate\n",
> > +				certfile);
> > +			fclose(fp);
> > +			return -1;
> > +		}
> > +	}
> > +	fclose(fp);
> > +
> > +	if (!(skid = X509_get0_subject_key_id(x))) {
> > +		log_err("read keyid: %s: SKID not found\n", certfile);
> > +		goto err_free;
> > +	}
> > +	skid_len = ASN1_STRING_length(skid);
> > +	if (skid_len < sizeof(keyid_raw)) {
> > +		log_err("read keyid: %s: SKID too short (len %d)\n", certfile,
> > +			skid_len);
> > +		goto err_free;
> > +	}
> > +	memcpy(&keyid_raw, ASN1_STRING_get0_data(skid) + skid_len
> > +	       - sizeof(keyid_raw), sizeof(keyid_raw));
> > +	if (keyid)
> > +		memcpy(keyid, &keyid_raw, sizeof(*keyid));
> 
> 
> If keyid is supposed to be in native format then you have to apply ntohl()
> to it..

Keyid is supposed to be in network order, it's documented (for
ima_read_keyid which is just a wrapper for _ima_read_keyid).
> 
> 
> > +	log_info("keyid %04x (from %s)\n", ntohl(keyid_raw), certfile);
> > +	return ntohl(keyid_raw);
> 
> I thought this should return 0  ?

Why you thought so? It's documented for ima_read_keyid() return value is
-1 or 32-bit keyid.

> 
> 
> > +
> > +err_free:
> > +	X509_free(x);
> > +	return -1;
> > +}
> > +
> > +/**
> > + * ima_read_keyid() - Read 32-bit keyid from the cert file.
> > + * @certfile:	File possibly containing certificate in DER/PEM format.
> > + * @keyid:	Output keyid in network order.
> > + *
> > + * Try to read keyid from Subject Key Identifier (SKID) of certificate.
> > + * Autodetect if cert is in PEM or DER encoding.
> > + *
> > + * Return: -1 (ULONG_MAX) on error;
> > + *         32-bit keyid as unsigned integer in host order.
> That's confusing, two times the same result, one time in host order, on time
> in network order. Why not just one return value in host order?

Pointer API is similar to calc_keyid_v2().

Do you sugegst to change calc_keyid_v2() API too?

To introduce non-confusing API that contradict other parts of API would
be more confusing than it already is.

Thanks,

> > + */
> > +unsigned long ima_read_keyid(const char *certfile, uint32_t *keyid)
> > +{
> > +	return _ima_read_keyid(certfile, keyid, KEYID_FILE_UNK_CERT);
> > +}
> > +
> >   static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
> >   {
> >   	FILE *fp;
> > diff --git a/tests/sign_verify.test b/tests/sign_verify.test
> > index 2c21812..52ea33a 100755
> > --- a/tests/sign_verify.test
> > +++ b/tests/sign_verify.test
> > @@ -360,6 +360,7 @@ sign_verify  rsa1024  md5     0x030201:K:0080
> >   sign_verify  rsa1024  sha1    0x030202:K:0080
> >   sign_verify  rsa1024  sha224  0x030207:K:0080
> >   expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204aabbccdd0080 OPTS=--keyid=aabbccdd
> > +expect_pass check_sign TYPE=ima KEY=rsa1024 ALG=sha256 PREFIX=0x030204:K:0080 OPTS=--keyid=@test-rsa1024.cer
> >   sign_verify  rsa1024  sha256  0x030204:K:0080
> >     try_different_keys
> >     try_different_sigs

Re: [PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file

[Vitaly Chikunov]

Stefan, Mimi,

On Thu, May 06, 2021 at 03:54:53AM +0300, Vitaly Chikunov wrote:
> On Wed, May 05, 2021 at 07:13:39PM -0400, Stefan Berger wrote:
> > On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
> > > Allow user to specify `--keyid @/path/to/cert.pem' to extract keyid from
> > > SKID of the certificate file. PEM or DER format is auto-detected.
> > > 
> > > `--keyid' option is reused instead of adding a new option (like possible
> > > `--cert') to signify to the user it's only keyid extraction and nothing
> > > more.
> > > 
> > > This commit creates ABI change for libimaevm, due to adding new function
> > > ima_read_keyid(). Newer clients cannot work with older libimaevm.
> > > Together with previous commit it creates backward-incompatible ABI
> > > change, thus soname should be incremented on release.
> > > 
> > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > ---
> > >   README                 |  1 +
> > >   src/evmctl.c           | 22 ++++++++++---
> > >   src/imaevm.h           |  1 +
> > >   src/libimaevm.c        | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   tests/sign_verify.test |  1 +
> > >   5 files changed, 105 insertions(+), 5 deletions(-)
> > > 
> > > +/**
> > > + * ima_read_keyid() - Read 32-bit keyid from the cert file.
> > > + * @certfile:	File possibly containing certificate in DER/PEM format.
> > > + * @keyid:	Output keyid in network order.
> > > + *
> > > + * Try to read keyid from Subject Key Identifier (SKID) of certificate.
> > > + * Autodetect if cert is in PEM or DER encoding.
> > > + *
> > > + * Return: -1 (ULONG_MAX) on error;
> > > + *         32-bit keyid as unsigned integer in host order.
> > That's confusing, two times the same result, one time in host order, on time
> > in network order. Why not just one return value in host order?
> 
> Pointer API is similar to calc_keyid_v2().
> 
> Do you sugegst to change calc_keyid_v2() API too?
> 
> To introduce non-confusing API that contradict other parts of API would
> be more confusing than it already is.

Maybe we could change this libimaevm API:

  void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey);

to

  void calc_keyid_v2(uint8_t *keyid, char *str, EVP_PKEY *pkey);

To signal to the user that there it's not just uint32_t, but some byte
array (possible in network order). This would not even break ABI, only
API. (But, we breaking ABI with this patch set anyway.)

Thanks,

Re: [PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file

[Stefan Berger]

>> That's confusing, two times the same result, one time in host order, on time
>> in network order. Why not just one return value in host order?
> Pointer API is similar to calc_keyid_v2().
>
> Do you sugegst to change calc_keyid_v2() API too?

No, we have to leave the existing API as-is.


>
> To introduce non-confusing API that contradict other parts of API would
> be more confusing than it already is.


Indeed. Sorry for the confusion.


    Stefan

Re: [PATCH v4 3/3] ima-evm-utils: Read keyid from the cert appended to the key file

[Stefan Berger]

On 5/5/21 8:53 PM, Vitaly Chikunov wrote:
> Stefan,
>
> On Thu, May 06, 2021 at 03:29:32AM +0300, Vitaly Chikunov wrote:
>> On Wed, May 05, 2021 at 07:29:17PM -0400, Stefan Berger wrote:
>>> On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
>>>> Allow to have certificate appended to the private key of `--key'
>>>> specified (PEM) file (for v2 signing) to facilitate reading of keyid
>>>> from the associated cert. This will allow users to have private and
>>>> public key as a single file. There is no check that public key form the
>>>> cert matches associated private key.
>>>>
>>>> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
>>>> ---
>>>>    README          | 3 +++
>>>>    src/libimaevm.c | 8 +++++---
>>>>    2 files changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/README b/README
>>>> index 0e1f6ba..ea11bde 100644
>>>> --- a/README
>>>> +++ b/README
>>>> @@ -127,6 +127,9 @@ for signing and importing the key.
>>>>    Second key format uses X509 DER encoded public key certificates and uses asymmetric key support
>>>>    in the kernel (since kernel 3.9). CONFIG_INTEGRITY_ASYMMETRIC_KEYS must be enabled (default).
>>>> +For v2 signatures x509 certificate with the public key could be appended to the private
>>>> +key (both are in PEM format) to properly determine its Subject Key Identifier (SKID).
>>>> +
>>>>    Integrity keyrings
>>>>    ----------------
>>>> diff --git a/src/libimaevm.c b/src/libimaevm.c
>>>> index a22d9bb..ac4bb46 100644
>>>> --- a/src/libimaevm.c
>>>> +++ b/src/libimaevm.c
>>>> @@ -1017,10 +1017,12 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash,
>>>>    		return -1;
>>>>    	}
>>>> -	if (imaevm_params.keyid)
>>>> +	if (imaevm_params.keyid) {
>>>>    		hdr->keyid = htonl(imaevm_params.keyid);
>>>> -	else
>>>> -		calc_keyid_v2(&hdr->keyid, name, pkey);
>>>> +	} else {
>>>> +		if (_ima_read_keyid(keyfile, &hdr->keyid, KEYID_FILE_PEM_KEY) == ULONG_MAX)
>>>> +			calc_keyid_v2(&hdr->keyid, name, pkey);
>>>> +	}
>>> It might be convenient here to just write the result in network byte order
>>> into the header but for a library API I find it not so nice, but then
>>> there's calc_keyid_v2 that does that already... I just wouldn't expect that
>>> these parameter are in big endian order already, I would expect them in
>>> native order.
>> I expect them in network order, similar to how calc_keyid_v2() already
>> writes it one line below. So, why should we mix orders? Both functions
>> write keyids, so it's not like completely different parts of API. Also,
>> it's documented that ima_read_keyid() writes to the pointer in network
>> order (and returns integer in host order), so I don't see the
>> problem. Thus, I would prefer not to follow this suggestion.
>>
>> Thanks,
>>
>>> So maybe ima_read_keyid should just return ULONG_MAX or the
>>> keyid in host order and it call _ima_read_keyid() with a NULL pointer or a
>>> dummy variable as keyid that the library API caller doesn't see.
> So there will be exported
>     uint32_t ima_read_keyid(char *certfile);
> and internal
>     static bool _read_keyid(char *certfile, uint32_t *keyid)
>
> ima_read_keyid will wrap read_keyid and return keyid via intermediate
> variable. Right?

Yes like this. 0 returned from ima_read_keyid would indicate that 
nothing was found but could with a chance of 1/2^-32 be also a valid key 
id. Though we already declare it invalid in 1/3 when parsing a 0 from 
command line.

Re: [PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file

[Stefan Berger]

On 5/5/21 9:07 PM, Vitaly Chikunov wrote:
> Stefan, Mimi,
>
> On Thu, May 06, 2021 at 03:54:53AM +0300, Vitaly Chikunov wrote:
>> On Wed, May 05, 2021 at 07:13:39PM -0400, Stefan Berger wrote:
>>> On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
>>>> Allow user to specify `--keyid @/path/to/cert.pem' to extract keyid from
>>>> SKID of the certificate file. PEM or DER format is auto-detected.
>>>>
>>>> `--keyid' option is reused instead of adding a new option (like possible
>>>> `--cert') to signify to the user it's only keyid extraction and nothing
>>>> more.
>>>>
>>>> This commit creates ABI change for libimaevm, due to adding new function
>>>> ima_read_keyid(). Newer clients cannot work with older libimaevm.
>>>> Together with previous commit it creates backward-incompatible ABI
>>>> change, thus soname should be incremented on release.
>>>>
>>>> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
>>>> ---
>>>>    README                 |  1 +
>>>>    src/evmctl.c           | 22 ++++++++++---
>>>>    src/imaevm.h           |  1 +
>>>>    src/libimaevm.c        | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    tests/sign_verify.test |  1 +
>>>>    5 files changed, 105 insertions(+), 5 deletions(-)
>>>>
>>>> +/**
>>>> + * ima_read_keyid() - Read 32-bit keyid from the cert file.
>>>> + * @certfile:	File possibly containing certificate in DER/PEM format.
>>>> + * @keyid:	Output keyid in network order.
>>>> + *
>>>> + * Try to read keyid from Subject Key Identifier (SKID) of certificate.
>>>> + * Autodetect if cert is in PEM or DER encoding.
>>>> + *
>>>> + * Return: -1 (ULONG_MAX) on error;
>>>> + *         32-bit keyid as unsigned integer in host order.
>>> That's confusing, two times the same result, one time in host order, on time
>>> in network order. Why not just one return value in host order?
>> Pointer API is similar to calc_keyid_v2().
>>
>> Do you sugegst to change calc_keyid_v2() API too?
>>
>> To introduce non-confusing API that contradict other parts of API would
>> be more confusing than it already is.
> Maybe we could change this libimaevm API:
>
>    void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey);
>
> to
>
>    void calc_keyid_v2(uint8_t *keyid, char *str, EVP_PKEY *pkey);


I think it's better to leave it... :-(


>
> To signal to the user that there it's not just uint32_t, but some byte
> array (possible in network order). This would not even break ABI, only
> API. (But, we breaking ABI with this patch set anyway.)

You mean we are breaking it by introducing this extensions here?

@ -196,6 +196,7 @@ struct libimaevm_params {
  	const char *hash_algo;
  	const char *keyfile;
  	const char *keypass;
+	uint32_t keyid;		/* keyid overriding value, unless 0. */
  };

>
> Thanks,
>

Re: [PATCH v4 2/3] ima-evm-utils: Allow manual setting keyid from a cert file

[Vitaly Chikunov]

Stefan,

On Wed, May 05, 2021 at 10:24:48PM -0400, Stefan Berger wrote:
> On 5/5/21 9:07 PM, Vitaly Chikunov wrote:
> > On Thu, May 06, 2021 at 03:54:53AM +0300, Vitaly Chikunov wrote:
> > > On Wed, May 05, 2021 at 07:13:39PM -0400, Stefan Berger wrote:
> > > > On 5/5/21 2:48 AM, Vitaly Chikunov wrote:
> > > > > Allow user to specify `--keyid @/path/to/cert.pem' to extract keyid from
> > > > > SKID of the certificate file. PEM or DER format is auto-detected.
> > > > > 
> > > > > `--keyid' option is reused instead of adding a new option (like possible
> > > > > `--cert') to signify to the user it's only keyid extraction and nothing
> > > > > more.
> > > > > 
> > > > > This commit creates ABI change for libimaevm, due to adding new function
> > > > > ima_read_keyid(). Newer clients cannot work with older libimaevm.
> > > > > Together with previous commit it creates backward-incompatible ABI
> > > > > change, thus soname should be incremented on release.
> > > > > 
> > > > > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> > > > > ---
> > > > >    README                 |  1 +
> > > > >    src/evmctl.c           | 22 ++++++++++---
> > > > >    src/imaevm.h           |  1 +
> > > > >    src/libimaevm.c        | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    tests/sign_verify.test |  1 +
> > > > >    5 files changed, 105 insertions(+), 5 deletions(-)
> > > > > 
> > > > > +/**
> > > > > + * ima_read_keyid() - Read 32-bit keyid from the cert file.
> > > > > + * @certfile:	File possibly containing certificate in DER/PEM format.
> > > > > + * @keyid:	Output keyid in network order.
> > > > > + *
> > > > > + * Try to read keyid from Subject Key Identifier (SKID) of certificate.
> > > > > + * Autodetect if cert is in PEM or DER encoding.
> > > > > + *
> > > > > + * Return: -1 (ULONG_MAX) on error;
> > > > > + *         32-bit keyid as unsigned integer in host order.
> > > > That's confusing, two times the same result, one time in host order, on time
> > > > in network order. Why not just one return value in host order?
> > > Pointer API is similar to calc_keyid_v2().
> > > 
> > > Do you sugegst to change calc_keyid_v2() API too?
> > > 
> > > To introduce non-confusing API that contradict other parts of API would
> > > be more confusing than it already is.
> > Maybe we could change this libimaevm API:
> > 
> >    void calc_keyid_v2(uint32_t *keyid, char *str, EVP_PKEY *pkey);
> > 
> > to
> > 
> >    void calc_keyid_v2(uint8_t *keyid, char *str, EVP_PKEY *pkey);
> 
> 
> I think it's better to leave it... :-(

OK.

> 
> > 
> > To signal to the user that there it's not just uint32_t, but some byte
> > array (possible in network order). This would not even break ABI, only
> > API. (But, we breaking ABI with this patch set anyway.)
> 
> You mean we are breaking it by introducing this extensions here?

As described in commit messages - adding new API function breaks
compatibility of new clients with old lib, and adding keyid into
libimaevm_params breaks compatibility of new lib with old clients.
Thus, major ABI change.

> 
> @ -196,6 +196,7 @@ struct libimaevm_params {
>  	const char *hash_algo;
>  	const char *keyfile;
>  	const char *keypass;
> +	uint32_t keyid;		/* keyid overriding value, unless 0. */
>  };
> 
> > 
> > Thanks,
> >