[PATCH v1 0/5] ima-evm-utils: Assorted fixes and improvements

[PATCH v1 0/5] ima-evm-utils: Assorted fixes and improvements

[Vitaly Chikunov]

There is small fixes and improvements to ima-evm-utils.
Tested on x86_64.

Vitaly Chikunov (5):
  ima-evm-utils: Fix EVP_MD_CTX leak in ima_calc_hash
  ima-evm-utils: Fix memory leak in init_public_keys
  ima-evm-utils: Preload public keys for ima_verify
  ima-evm-utils: Allow multiple files in ima_verify
  ima-evm-utils: Fix clang warning about possible unaligned pointer for
    hdr->keyid

 src/evmctl.c    | 11 ++++++++---
 src/libimaevm.c | 38 ++++++++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 15 deletions(-)

-- 
2.11.0

[PATCH v1 1/5] ima-evm-utils: Fix EVP_MD_CTX leak in ima_calc_hash

[Vitaly Chikunov]

When pctx is allocated using EVP_MD_CTX_new() it should be freed.
Found with ASan.

Fixes: 81010f0 ("ima-evm-utils: Add backward compatible support for openssl 1.1")
Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
 src/libimaevm.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 51d6c33..fe1962b 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -252,19 +252,21 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 	err = lstat(file, &st);
 	if (err < 0) {
 		log_err("Failed to stat: %s\n", file);
-		return err;
+		goto err;
 	}
 
 	md = EVP_get_digestbyname(params.hash_algo);
 	if (!md) {
 		log_err("EVP_get_digestbyname(%s) failed\n", params.hash_algo);
-		return 1;
+		err = 1;
+		goto err;
 	}
 
 	err = EVP_DigestInit(pctx, md);
 	if (!err) {
 		log_err("EVP_DigestInit() failed\n");
-		return 1;
+		err = 1;
+		goto err;
 	}
 
 	switch (st.st_mode & S_IFMT) {
@@ -283,19 +285,25 @@ int ima_calc_hash(const char *file, uint8_t *hash)
 		break;
 	default:
 		log_errno("Unsupported file type");
-		return -1;
+		err = -1;
+		goto err;
 	}
 
 	if (err)
-		return err;
+		goto err;
 
 	err = EVP_DigestFinal(pctx, hash, &mdlen);
 	if (!err) {
 		log_err("EVP_DigestFinal() failed\n");
-		return 1;
+		err = 1;
+		goto err;
 	}
-
-	return mdlen;
+	err = mdlen;
+err:
+#if OPENSSL_VERSION_NUMBER >= 0x10100000
+	EVP_MD_CTX_free(pctx);
+#endif
+	return err;
 }
 
 EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
-- 
2.11.0

[PATCH v1 2/5] ima-evm-utils: Fix memory leak in init_public_keys

[Vitaly Chikunov]

strdup'ed string should be freed. Found with ASan.

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

diff --git a/src/libimaevm.c b/src/libimaevm.c
index fe1962b..b556276 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -421,11 +421,12 @@ static EVP_PKEY *find_keyid(uint32_t keyid)
 void init_public_keys(const char *keyfiles)
 {
 	struct public_key_entry *entry;
-	char *tmp_keyfiles;
+	char *tmp_keyfiles, *keyfiles_free;
 	char *keyfile;
 	int i = 1;
 
 	tmp_keyfiles = strdup(keyfiles);
+	keyfiles_free = tmp_keyfiles;
 
 	while ((keyfile = strsep(&tmp_keyfiles, ", \t")) != NULL) {
 		if (!keyfile)
@@ -452,6 +453,7 @@ void init_public_keys(const char *keyfiles)
 		entry->next = public_keys;
 		public_keys = entry;
 	}
+	free(keyfiles_free);
 }
 
 /*
-- 
2.11.0

[PATCH v1 3/5] ima-evm-utils: Preload public keys for ima_verify

[Vitaly Chikunov]

This allows testing verify_hash_v2() with multiple public keys.

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

diff --git a/src/evmctl.c b/src/evmctl.c
index 4e0a831..fac593a 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -840,6 +840,9 @@ static int cmd_verify_ima(struct command *cmd)
 	char *file = g_argv[optind++];
 	int err;
 
+	if (params.keyfile)
+		init_public_keys(params.keyfile);
+
 	errno = 0;
 	if (!file) {
 		log_err("Parameters missing\n");
-- 
2.11.0

[PATCH v1 4/5] ima-evm-utils: Allow multiple files in ima_verify

[Vitaly Chikunov]

This allows testing multiple verify in a row, similar to ima_measurement.

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

diff --git a/src/evmctl.c b/src/evmctl.c
index fac593a..7ce2022 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -850,9 +850,11 @@ static int cmd_verify_ima(struct command *cmd)
 		return -1;
 	}
 
-	err = verify_ima(file);
-	if (!err && params.verbose >= LOG_INFO)
-		log_info("%s: verification is OK\n", file);
+	do {
+		err = verify_ima(file);
+		if (!err && params.verbose >= LOG_INFO)
+			log_info("%s: verification is OK\n", file);
+	} while ((file = g_argv[optind++]));
 	return err;
 }
 
-- 
2.11.0

[PATCH v1 5/5] ima-evm-utils: Fix clang warning about possible unaligned pointer for hdr->keyid

[Vitaly Chikunov]

Struct signature_v2_hdr is packed so clang complains that taking address
of packed member may result in an unaligned pointer value:

libimaevm.c:481:21: warning: taking address of packed member 'keyid' of class or structure 'signature_v2_hdr' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
                                __be32_to_cpup(&hdr->keyid));
                                                ^~~~~~~~~~

libimaevm.c:905:17: warning: taking address of packed member 'keyid' of class or structure 'signature_v2_hdr' may result in an unaligned pointer value
      [-Waddress-of-packed-member]
        calc_keyid_v2(&hdr->keyid, name, pkey);
                       ^~~~~~~~~~

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

diff --git a/src/libimaevm.c b/src/libimaevm.c
index b556276..f8ab812 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -475,10 +475,12 @@ int verify_hash_v2(const char *file, const unsigned char *hash, int size,
 	}
 
 	if (public_keys) {
-		pkey = find_keyid(hdr->keyid);
+		uint32_t keyid = hdr->keyid;
+
+		pkey = find_keyid(keyid);
 		if (!pkey) {
 			log_err("%s: unknown keyid: %x\n", file,
-				__be32_to_cpup(&hdr->keyid));
+				__be32_to_cpup(&keyid));
 			return -1;
 		}
 	} else {
@@ -869,6 +871,7 @@ int sign_hash_v2(const char *algo, const unsigned char *hash, int size, const ch
 	const EVP_MD *md;
 	size_t sigsize;
 	const char *st;
+	uint32_t keyid;
 
 	if (!hash) {
 		log_err("sign_hash_v2: hash is null\n");
@@ -902,7 +905,8 @@ 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, pkey);
+	calc_keyid_v2(&keyid, name, pkey);
+	hdr->keyid = keyid;
 
 	st = "EVP_PKEY_CTX_new";
 	if (!(ctx = EVP_PKEY_CTX_new(pkey, NULL)))
-- 
2.11.0

Re: [PATCH v1 0/5] ima-evm-utils: Assorted fixes and improvements

[Mimi Zohar]

[Cc'ing Roberto, Petr, Thiago, Prakhar]

Hi Vitaly,

On Mon, 2019-07-08 at 02:48 +0300, Vitaly Chikunov wrote:
> There is small fixes and improvements to ima-evm-utils.
> Tested on x86_64.
> 
> Vitaly Chikunov (5):
>   ima-evm-utils: Fix EVP_MD_CTX leak in ima_calc_hash
>   ima-evm-utils: Fix memory leak in init_public_keys
>   ima-evm-utils: Preload public keys for ima_verify
>   ima-evm-utils: Allow multiple files in ima_verify
>   ima-evm-utils: Fix clang warning about possible unaligned pointer for
>     hdr->keyid
> 
>  src/evmctl.c    | 11 ++++++++---
>  src/libimaevm.c | 38 ++++++++++++++++++++++++++------------
>  2 files changed, 34 insertions(+), 15 deletions(-)

Thanks, this patch set looks good.  These patches, the "ima-evm-utils: 
Convert v2 signatures from RSA to EVP_PKEY AP", and the two patches I
posted today are now in #next, but I'd really appreciate some
additional Review's/Tested's on these patches.

Now that we're including ALL the kernel exported hash_info algorithms,
a colleague suggested defining a list of deprecated hash algorithms.
 Instead of preventing the usage of these deprecated hash algorithms,
initially I would start out with a warning.  It would be helpful to
indicate which standard deprecated the hash algorithm and year.  At
some point, we might want to prevent their usage in signing files, but
not verifying file signatures.

evmctl "ima_measurement" doesn't support custom template definitions.
Also missing is support for verifying the "ima-buf" kexec command boot
command line and the "ima-modsig" template appended signature.

David Jacobson started writing a regression framework and posted a v2
version.  I'd really appreciate help with cleaning up that code. 

Any other comments/suggestions/ideas?

thanks,

Mimi

Re: [PATCH v1 0/5] ima-evm-utils: Assorted fixes and improvements

[Vitaly Chikunov]

Mimi,

On Mon, Jul 08, 2019 at 11:30:50AM -0400, Mimi Zohar wrote:
> [Cc'ing Roberto, Petr, Thiago, Prakhar]
> Now that we're including ALL the kernel exported hash_info algorithms,
> a colleague suggested defining a list of deprecated hash algorithms.
>  Instead of preventing the usage of these deprecated hash algorithms,
> initially I would start out with a warning.  It would be helpful to
> indicate which standard deprecated the hash algorithm and year.  At
> some point, we might want to prevent their usage in signing files, but
> not verifying file signatures.

I think this is not a problem, because user explicitly states which hash
algorithm he wants to use. Except for SHA1, which is also silent
fallback algorithm. I think this fallback mechanism should be removed.

Also, return values of sign_hash/ima_calc_hash/etc are not defined
clearly and callers have weird checks such as `if (len <= 1)`. I think
this should be conceptually simplified and made them `return -1` on any
error.


> evmctl "ima_measurement" doesn't support custom template definitions.
> Also missing is support for verifying the "ima-buf" kexec command boot
> command line and the "ima-modsig" template appended signature.
> 
> David Jacobson started writing a regression framework and posted a v2
> version.  I'd really appreciate help with cleaning up that code. 

Maybe tests should be integrated into ima-evm-utils too.

Thanks,

Re: [PATCH v1 0/5] ima-evm-utils: Assorted fixes and improvements

[Mimi Zohar]

On Tue, 2019-07-09 at 18:43 +0300, Vitaly Chikunov wrote:
> On Mon, Jul 08, 2019 at 11:30:50AM -0400, Mimi Zohar wrote:
> > [Cc'ing Roberto, Petr, Thiago, Prakhar]
> > Now that we're including ALL the kernel exported hash_info algorithms,
> > a colleague suggested defining a list of deprecated hash algorithms.
> >  Instead of preventing the usage of these deprecated hash algorithms,
> > initially I would start out with a warning.  It would be helpful to
> > indicate which standard deprecated the hash algorithm and year.  At
> > some point, we might want to prevent their usage in signing files, but
> > not verifying file signatures.
> 
> I think this is not a problem, because user explicitly states which hash
> algorithm he wants to use. Except for SHA1, which is also silent
> fallback algorithm. I think this fallback mechanism should be removed.

I don't see a problem with informing whoever is using ima-evm-utils,
that the requested hash algorithm has been deprecated.  Just as NIST
has deprecated the use of sha1 for most usecases, certain versions of
the gost standard have also been deprecated.

Someone verifying the measurement list or the filesystem signatures
should be informed that although the verification succeeded, or not,
some of the signatures verified were based on deprecated hash
algorithms.  Maybe something along the lines of "<algorithm> hash
algorithm was deprecated (<year> - <standard>)"?  In verbose mode, the
message could include the filename.

The EVM hmac is still sha1 based, but that shouldn't affect ima-evm-
utils.  So yes we should explicitly require a valid hash algorithm and
not fall back to using sha1.

> 
> Also, return values of sign_hash/ima_calc_hash/etc are not defined
> clearly and callers have weird checks such as `if (len <= 1)`. I think
> this should be conceptually simplified and made them `return -1` on any
> error.

Agreed.  Mostly these functions are returning "-1" on error.  There
are a few places returning 1 instead.

> 
> 
> > evmctl "ima_measurement" doesn't support custom template definitions.
> > Also missing is support for verifying the "ima-buf" kexec command boot
> > command line and the "ima-modsig" template appended signature.
> > 
> > David Jacobson started writing a regression framework and posted a v2
> > version.  I'd really appreciate help with cleaning up that code. 
> 
> Maybe tests should be integrated into ima-evm-utils too.

Including regression tests in ima-evm-utils is the plan. :)

Mimi

Re: [PATCH v1 0/5] ima-evm-utils: Assorted fixes and improvements

[Petr Vorel]

Hi Mimi, Vitaly,

> Mimi,

> On Mon, Jul 08, 2019 at 11:30:50AM -0400, Mimi Zohar wrote:
> > [Cc'ing Roberto, Petr, Thiago, Prakhar]
> > Now that we're including ALL the kernel exported hash_info algorithms,
> > a colleague suggested defining a list of deprecated hash algorithms.
> >  Instead of preventing the usage of these deprecated hash algorithms,
> > initially I would start out with a warning.  It would be helpful to
> > indicate which standard deprecated the hash algorithm and year.  At
> > some point, we might want to prevent their usage in signing files, but
> > not verifying file signatures.
Looks useful to me.

> I think this is not a problem, because user explicitly states which hash
> algorithm he wants to use. Except for SHA1, which is also silent
> fallback algorithm. I think this fallback mechanism should be removed.
Agree with removing fallback mechanism.

> Also, return values of sign_hash/ima_calc_hash/etc are not defined
> clearly and callers have weird checks such as `if (len <= 1)`. I think
> this should be conceptually simplified and made them `return -1` on any
> error.


> > evmctl "ima_measurement" doesn't support custom template definitions.
> > Also missing is support for verifying the "ima-buf" kexec command boot
> > command line and the "ima-modsig" template appended signature.

> > David Jacobson started writing a regression framework and posted a v2
> > version.  I'd really appreciate help with cleaning up that code. 

> Maybe tests should be integrated into ima-evm-utils too.
https://patchwork.kernel.org/project/linux-integrity/list/?series=95303

> Thanks,

Kind regards,
Petr

Re: [PATCH v1 4/5] ima-evm-utils: Allow multiple files in ima_verify

[Vitaly Chikunov]

Mimi,

On Mon, Jul 08, 2019 at 02:48:36AM +0300, Vitaly Chikunov wrote:
> This allows testing multiple verify in a row, similar to ima_measurement.
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
>  src/evmctl.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index fac593a..7ce2022 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -850,9 +850,11 @@ static int cmd_verify_ima(struct command *cmd)
>  		return -1;
>  	}
>  
> -	err = verify_ima(file);
> -	if (!err && params.verbose >= LOG_INFO)
> -		log_info("%s: verification is OK\n", file);
> +	do {
> +		err = verify_ima(file);
> +		if (!err && params.verbose >= LOG_INFO)
> +			log_info("%s: verification is OK\n", file);
> +	} while ((file = g_argv[optind++]));

Currently `err' is affected only by the last verified file. But I think
value of err should be affected by results from all files. But how?
Should we AND verification results or OR? I think it should be ANDed and
will send new version of this patch.

Thanks,

>  	return err;
>  }
>  
> -- 
> 2.11.0