[PATCH 1/3] ima-evm-utils: Remove indirect call to subfunctions in verify_hash

[PATCH 1/3] ima-evm-utils: Remove indirect call to subfunctions in verify_hash

[Vitaly Chikunov]

This is more human understandable and also will improve handling of
the sources by cscope.

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

diff --git a/src/imaevm.h b/src/imaevm.h
index 36050f4..0414433 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -188,9 +188,6 @@ struct signature_v2_hdr {
 	uint8_t sig[0];		/* signature payload */
 } __packed;
 
-
-typedef int (*verify_hash_fn_t)(const char *file, const unsigned char *hash, int size, unsigned char *sig, int siglen, const char *keyfile);
-
 struct libevm_params {
 	int verbose;
 	int x509;
diff --git a/src/libimaevm.c b/src/libimaevm.c
index afd2105..97b7167 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -572,22 +572,18 @@ static int get_hash_algo_from_sig(unsigned char *sig)
 int verify_hash(const char *file, const unsigned char *hash, int size, unsigned char *sig,
 		int siglen)
 {
-	const char *key = NULL;
-	verify_hash_fn_t verify_hash;
-
 	/* Get signature type from sig header */
 	if (sig[0] == DIGSIG_VERSION_1) {
-		verify_hash = verify_hash_v1;
+		const char *key = NULL;
 
 		/* Read pubkey from RSA key */
 		if (!params.keyfile)
 			key = "/etc/keys/pubkey_evm.pem";
+		return verify_hash_v1(file, hash, size, sig, siglen, key);
 	} else if (sig[0] == DIGSIG_VERSION_2) {
-		verify_hash = verify_hash_v2;
+		return verify_hash_v2(file, hash, size, sig, siglen, NULL);
 	} else
 		return -1;
-
-	return verify_hash(file, hash, size, sig, siglen, key);
 }
 
 int ima_verify_signature(const char *file, unsigned char *sig, int siglen,
-- 
2.11.0

[PATCH 2/3] ima-evm-utils: Remove not needed argument from verify_hash_v2

[Vitaly Chikunov]

Since we now always call verify_hash_v2() with NULL keyfile (assuming
all keys are already loaded into public_keys list), remove keyfile
argument and its handling from verify_hash_v2().

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/libimaevm.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 97b7167..b153f1b 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -453,7 +453,7 @@ void init_public_keys(const char *keyfiles)
  * Return: 0 verification good, 1 verification bad, -1 error.
  */
 int verify_hash_v2(const char *file, const unsigned char *hash, int size,
-		   unsigned char *sig, int siglen, const char *keyfile)
+		   unsigned char *sig, int siglen)
 {
 	int ret = -1;
 	EVP_PKEY *pkey, *pkey_free = NULL;
@@ -467,20 +467,13 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 		log_dump(hash, size);
 	}
 
-	if (public_keys) {
+	pkey = find_keyid(hdr->keyid);
+	if (!pkey) {
 		uint32_t keyid = hdr->keyid;
 
-		pkey = find_keyid(keyid);
-		if (!pkey) {
-			log_err("%s: unknown keyid: %x\n", file,
-				__be32_to_cpup(&keyid));
-			return -1;
-		}
-	} else {
-		pkey = read_pub_pkey(keyfile, 1);
-		if (!pkey)
-			return -1;
-		pkey_free = pkey;
+		log_err("%s: unknown keyid: %x\n", file,
+			__be32_to_cpup(&keyid));
+		return -1;
 	}
 
 	st = "EVP_PKEY_CTX_new";
@@ -581,7 +574,7 @@ int verify_hash(const char *file, const unsigned char *hash, int size, unsigned
 			key = "/etc/keys/pubkey_evm.pem";
 		return verify_hash_v1(file, hash, size, sig, siglen, key);
 	} else if (sig[0] == DIGSIG_VERSION_2) {
-		return verify_hash_v2(file, hash, size, sig, siglen, NULL);
+		return verify_hash_v2(file, hash, size, sig, siglen);
 	} else
 		return -1;
 }
-- 
2.11.0

[PATCH 3/3] ima-evm-utils: Improve OpenSSL error reporting

[Vitaly Chikunov]

Previously OpenSSL errors was delayed until evmctl exit (sometimes not).
Since we try to make libimaevm more robust, there could be many errors
accumulated, so it's useful to output OpenSSL errors as they happen.
This will also make output more understandable as you can see which
openssl error correspond to which libimaevm error.

Additionally, change spelling of read_pub_pkey and read_priv_pkey to
include key file name.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/libimaevm.c | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index b153f1b..2a0486f 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -116,6 +116,18 @@ const char *get_hash_algo_by_id(int algo)
 	return "unknown";
 }
 
+/* Output all remaining openssl error messages. */
+static void output_openssl_errors(void)
+{
+	while (ERR_peek_error()) {
+		char buf[256];
+		/* buf must be at least 256 bytes long according to man */
+
+		ERR_error_string(ERR_get_error(), buf);
+		log_err("openssl: %s\n", buf);
+	}
+}
+
 static int add_file_hash(const char *file, EVP_MD_CTX *ctx)
 {
 	uint8_t *data;
@@ -191,6 +203,7 @@ static int add_dir_hash(const char *file, EVP_MD_CTX *ctx)
 		err |= EVP_DigestUpdate(ctx, &type, sizeof(type));
 		if (!err) {
 			log_err("EVP_DigestUpdate() failed\n");
+			output_openssl_errors();
 			result = 1;
 			break;
 		}
@@ -290,6 +303,8 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 	}
 	err = mdlen;
 err:
+	if (err == 1)
+		output_openssl_errors();
 #if OPENSSL_VERSION_NUMBER >= 0x10100000
 	EVP_MD_CTX_free(pctx);
 #endif
@@ -299,7 +314,6 @@ err:
 EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 {
 	FILE *fp;
-	X509 *crt = NULL;
 	EVP_PKEY *pkey = NULL;
 
 	if (!keyfile)
@@ -312,24 +326,30 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 	}
 
 	if (x509) {
-		crt = d2i_X509_fp(fp, NULL);
+		X509 *crt = d2i_X509_fp(fp, NULL);
+
 		if (!crt) {
-			log_err("d2i_X509_fp() failed\n");
+			log_err("Failed to d2i_X509_fp key file: %s\n",
+				keyfile);
 			goto out;
 		}
 		pkey = X509_extract_key(crt);
 		X509_free(crt);
 		if (!pkey) {
-			log_err("X509_extract_key() failed\n");
+			log_err("Failed to X509_extract_key key file: %s\n",
+				keyfile);
 			goto out;
 		}
 	} else {
 		pkey = PEM_read_PUBKEY(fp, NULL, NULL, NULL);
 		if (!pkey)
-			log_err("PEM_read_PUBKEY() failed\n");
+			log_err("Failed to PEM_read_PUBKEY key file: %s\n",
+				keyfile);
 	}
 
 out:
+	if (!pkey)
+		output_openssl_errors();
 	fclose(fp);
 	return pkey;
 }
@@ -346,6 +366,7 @@ RSA *read_pub_key(const char *keyfile, int x509)
 	EVP_PKEY_free(pkey);
 	if (!key) {
 		log_err("read_pub_key: unsupported key type\n");
+		output_openssl_errors();
 		return NULL;
 	}
 	return key;
@@ -379,6 +400,7 @@ int verify_hash_v1(const char *file, const unsigned char *hash, int size,
 	RSA_free(key);
 	if (err < 0) {
 		log_err("%s: RSA_public_decrypt() failed: %d\n", file, err);
+		output_openssl_errors();
 		return 1;
 	}
 
@@ -496,6 +518,7 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 	else if (ret == 0) {
 		log_err("%s: verification failed: %d (%s)\n",
 			file, ret, ERR_reason_error_string(ERR_get_error()));
+		output_openssl_errors();
 		ret = 1;
 	}
 err:
@@ -503,6 +526,7 @@ err:
 		log_err("%s: verification failed: %d (%s) in %s\n",
 			file, ret, ERR_reason_error_string(ERR_peek_error()),
 			st);
+		output_openssl_errors();
 		ret = -1;
 	}
 	EVP_PKEY_CTX_free(ctx);
@@ -715,10 +739,9 @@ static EVP_PKEY *read_priv_pkey(const char *keyfile, const char *keypass)
 	ERR_load_crypto_strings();
 	pkey = PEM_read_PrivateKey(fp, NULL, NULL, (void *)keypass);
 	if (!pkey) {
-		char str[256];
-
-		ERR_error_string(ERR_get_error(), str);
-		log_err("PEM_read_PrivateKey() failed: %s\n", str);
+		log_err("Failed to PEM_read_PrivateKey key file: %s\n",
+			keyfile);
+		output_openssl_errors();
 	}
 
 	fclose(fp);
@@ -737,6 +760,7 @@ static RSA *read_priv_key(const char *keyfile, const char *keypass)
 	EVP_PKEY_free(pkey);
 	if (!key) {
 		log_err("read_priv_key: unsupported key type\n");
+		output_openssl_errors();
 		return NULL;
 	}
 	return key;
@@ -820,6 +844,7 @@ int sign_hash_v1(const char *hashalgo, const unsigned char *hash, int size, cons
 	len = RSA_private_encrypt(sizeof(sighash), sighash, sig + sizeof(*hdr) + 2, key, RSA_PKCS1_PADDING);
 	if (len < 0) {
 		log_err("RSA_private_encrypt() failed: %d\n", len);
+		output_openssl_errors();
 		goto out;
 	}
 
@@ -908,9 +933,11 @@ int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const ch
 	log_info("evm/ima signature: %d bytes\n", len);
 
 err:
-	if (len == -1)
+	if (len == -1) {
 		log_err("sign_hash_v2: signing failed: (%s) in %s\n",
 			ERR_reason_error_string(ERR_peek_error()), st);
+		output_openssl_errors();
+	}
 	EVP_PKEY_CTX_free(ctx);
 	EVP_PKEY_free(pkey);
 	return len;
-- 
2.11.0

Re: [PATCH 3/3] ima-evm-utils: Improve OpenSSL error reporting

[Mimi Zohar]

On Fri, 2019-07-19 at 00:35 +0300, Vitaly Chikunov wrote:
> Previously OpenSSL errors was delayed until evmctl exit (sometimes not).
> Since we try to make libimaevm more robust, there could be many errors
> accumulated, so it's useful to output OpenSSL errors as they happen.
> This will also make output more understandable as you can see which
> openssl error correspond to which libimaevm error.
> 
> Additionally, change spelling of read_pub_pkey and read_priv_pkey to
> include key file name.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

Thanks!

Mimi

Re: [PATCH 2/3] ima-evm-utils: Remove not needed argument from verify_hash_v2

[Mimi Zohar]

On Fri, 2019-07-19 at 00:35 +0300, Vitaly Chikunov wrote:
> Since we now always call verify_hash_v2() with NULL keyfile (assuming
> all keys are already loaded into public_keys list), remove keyfile
> argument and its handling from verify_hash_v2().
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

Thanks!

> ---
>  src/libimaevm.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 97b7167..b153f1b 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -453,7 +453,7 @@ void init_public_keys(const char *keyfiles)
>   * Return: 0 verification good, 1 verification bad, -1 error.
>   */
>  int verify_hash_v2(const char *file, const unsigned char *hash, int size,
> -		   unsigned char *sig, int siglen, const char *keyfile)
> +		   unsigned char *sig, int siglen)
>  {

While making this change, could we also make both this and
verify_hash_v1() functions static?  Should I make the change?

Mimi

>  	int ret = -1;
>  	EVP_PKEY *pkey, *pkey_free = NULL;
> @@ -467,20 +467,13 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
>  		log_dump(hash, size);
>  	}
> 
> -	if (public_keys) {
> +	pkey = find_keyid(hdr->keyid);
> +	if (!pkey) {
>  		uint32_t keyid = hdr->keyid;
> 
> -		pkey = find_keyid(keyid);
> -		if (!pkey) {
> -			log_err("%s: unknown keyid: %x\n", file,
> -				__be32_to_cpup(&keyid));
> -			return -1;
> -		}
> -	} else {
> -		pkey = read_pub_pkey(keyfile, 1);
> -		if (!pkey)
> -			return -1;
> -		pkey_free = pkey;
> +		log_err("%s: unknown keyid: %x\n", file,
> +			__be32_to_cpup(&keyid));
> +		return -1;
>  	}
> 
>  	st = "EVP_PKEY_CTX_new";
> @@ -581,7 +574,7 @@ int verify_hash(const char *file, const unsigned char *hash, int size, unsigned
>  			key = "/etc/keys/pubkey_evm.pem";
>  		return verify_hash_v1(file, hash, size, sig, siglen, key);
>  	} else if (sig[0] == DIGSIG_VERSION_2) {
> -		return verify_hash_v2(file, hash, size, sig, siglen, NULL);
> +		return verify_hash_v2(file, hash, size, sig, siglen);
>  	} else
>  		return -1;
>  }

Re: [PATCH 1/3] ima-evm-utils: Remove indirect call to subfunctions in verify_hash

[Mimi Zohar]

On Fri, 2019-07-19 at 00:35 +0300, Vitaly Chikunov wrote:
> This is more human understandable and also will improve handling of
> the sources by cscope.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

thanks!

Mimi

Re: [PATCH 2/3] ima-evm-utils: Remove not needed argument from verify_hash_v2

[Vitaly Chikunov]

Mimi,

On Thu, Jul 18, 2019 at 07:23:21PM -0400, Mimi Zohar wrote:
> On Fri, 2019-07-19 at 00:35 +0300, Vitaly Chikunov wrote:
> > Since we now always call verify_hash_v2() with NULL keyfile (assuming
> > all keys are already loaded into public_keys list), remove keyfile
> > argument and its handling from verify_hash_v2().
> > 
> > Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> 
> Thanks!
> 
> > ---
> >  src/libimaevm.c | 21 +++++++--------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index 97b7167..b153f1b 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -453,7 +453,7 @@ void init_public_keys(const char *keyfiles)
> >   * Return: 0 verification good, 1 verification bad, -1 error.
> >   */
> >  int verify_hash_v2(const char *file, const unsigned char *hash, int size,
> > -		   unsigned char *sig, int siglen, const char *keyfile)
> > +		   unsigned char *sig, int siglen)
> >  {
> 
> While making this change, could we also make both this and
> verify_hash_v1() functions static?

Yes, I wonder why they wasn't static in the first place.

> Should I make the change?

OK!

Thanks,