[PATCH 1/2] ima_evm_utils: erroneous "verification failed: 0 (invalid padding)" message

[PATCH 1/2] ima_evm_utils: erroneous "verification failed: 0 (invalid padding)" message

[Mimi Zohar]

When keys are not provided, the default key is used to verify the file
signature, resulting in this erroneous message.  Before using the default
key to verify the file signature, verify the keyid is correct.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/libimaevm.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index ae487f9fe36c..472ab53c7b42 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -302,6 +302,9 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 	X509 *crt = NULL;
 	EVP_PKEY *pkey = NULL;
 
+	if (!keyfile)
+		return NULL;
+
 	fp = fopen(keyfile, "r");
 	if (!fp) {
 		log_err("Failed to open keyfile: %s\n", keyfile);
@@ -569,27 +572,25 @@ 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;
-	int x509;
+	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;
+
 		/* Read pubkey from RSA key */
-		x509 = 0;
+		if (!params.keyfile)
+			key = "/etc/keys/pubkey_evm.pem";
 	} else if (sig[0] == DIGSIG_VERSION_2) {
 		verify_hash = verify_hash_v2;
+
 		/* Read pubkey from x509 cert */
-		x509 = 1;
+		if (!params.keyfile)
+			init_public_keys("/etc/keys/x509_evm.der");
 	} else
 		return -1;
 
-	/* Determine what key to use for verification*/
-	key = params.keyfile ? : x509 ?
-			"/etc/keys/x509_evm.der" :
-			"/etc/keys/pubkey_evm.pem";
-
 	return verify_hash(file, hash, size, sig, siglen, key);
 }
 
-- 
2.7.5

[PATCH 2/2] ima_evm_utils: limit duplicate "Failed to open keyfile" messages

[Mimi Zohar]

Unlike the user provided list of public keys, we don't know which
default public key file to use until verify_hash().  As a result, the
"Failed to open keyfile" message may be repeated multiple times.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 src/libimaevm.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/src/libimaevm.c b/src/libimaevm.c
index 472ab53c7b42..793643331f4b 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -296,18 +296,49 @@ err:
 	return err;
 }
 
+/*
+ * Keep track of missing keyfile names.
+ *
+ * Return 1 for found, return 0 for not found.
+ */
+static int lookup_keyfile_name(const char *keyfile_name)
+{
+	struct keyfile_name_entry {
+		struct keyfile_name_entry *next;
+		char name[];
+	} *entry;
+	static struct keyfile_name_entry *keyfile_names = NULL;
+
+	for (entry = keyfile_names; entry != NULL; entry = entry->next) {
+		if (strcmp(entry->name, keyfile_name) == 0)
+			return 1;
+	}
+
+	entry = malloc(sizeof(struct keyfile_name_entry) +
+			strlen(keyfile_name) + 1);
+	if (entry) {
+		strcpy(entry->name, keyfile_name);
+		entry->next = keyfile_names;
+		keyfile_names = entry;
+	}
+	return 0;
+}
+
 EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
 {
 	FILE *fp;
 	X509 *crt = NULL;
 	EVP_PKEY *pkey = NULL;
+	int found;
 
 	if (!keyfile)
 		return NULL;
 
 	fp = fopen(keyfile, "r");
 	if (!fp) {
-		log_err("Failed to open keyfile: %s\n", keyfile);
+		found = lookup_keyfile_name(keyfile);
+		if (!found)
+			log_err("Failed to open keyfile: %s\n", keyfile);
 		return NULL;
 	}
 
-- 
2.7.5

Re: [PATCH 2/2] ima_evm_utils: limit duplicate "Failed to open keyfile" messages

[Vitaly Chikunov]

Mimi,

On Tue, Jul 16, 2019 at 10:30:17AM -0400, Mimi Zohar wrote:
> Unlike the user provided list of public keys, we don't know which
> default public key file to use until verify_hash().  As a result, the
> "Failed to open keyfile" message may be repeated multiple times.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  src/libimaevm.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 472ab53c7b42..793643331f4b 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -296,18 +296,49 @@ err:
>  	return err;
>  }
>  
> +/*
> + * Keep track of missing keyfile names.
> + *
> + * Return 1 for found, return 0 for not found.
> + */
> +static int lookup_keyfile_name(const char *keyfile_name)
> +{
> +	struct keyfile_name_entry {
> +		struct keyfile_name_entry *next;
> +		char name[];
> +	} *entry;
> +	static struct keyfile_name_entry *keyfile_names = NULL;
> +
> +	for (entry = keyfile_names; entry != NULL; entry = entry->next) {
> +		if (strcmp(entry->name, keyfile_name) == 0)
> +			return 1;
> +	}
> +
> +	entry = malloc(sizeof(struct keyfile_name_entry) +
> +			strlen(keyfile_name) + 1);
> +	if (entry) {
> +		strcpy(entry->name, keyfile_name);
> +		entry->next = keyfile_names;
> +		keyfile_names = entry;
> +	}
> +	return 0;
> +}
> +
>  EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
>  {
>  	FILE *fp;
>  	X509 *crt = NULL;
>  	EVP_PKEY *pkey = NULL;
> +	int found;
>  
>  	if (!keyfile)
>  		return NULL;
>  
>  	fp = fopen(keyfile, "r");
>  	if (!fp) {
> -		log_err("Failed to open keyfile: %s\n", keyfile);
> +		found = lookup_keyfile_name(keyfile);
> +		if (!found)
> +			log_err("Failed to open keyfile: %s\n", keyfile);
>  		return NULL;


Now filename list is decoupled from keys themselves. Also we have key
list creation in init_public_keys(). Maybe we should just always call
init_public_keys for verify operations?

Thanks,

>  	}
>  
> -- 
> 2.7.5

Re: [PATCH 2/2] ima_evm_utils: limit duplicate "Failed to open keyfile" messages

[Mimi Zohar]

On Wed, 2019-07-17 at 00:49 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Tue, Jul 16, 2019 at 10:30:17AM -0400, Mimi Zohar wrote:
> > Unlike the user provided list of public keys, we don't know which
> > default public key file to use until verify_hash().  As a result, the
> > "Failed to open keyfile" message may be repeated multiple times.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  src/libimaevm.c | 33 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index 472ab53c7b42..793643331f4b 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -296,18 +296,49 @@ err:
> >  	return err;
> >  }
> >  
> > +/*
> > + * Keep track of missing keyfile names.
> > + *
> > + * Return 1 for found, return 0 for not found.
> > + */
> > +static int lookup_keyfile_name(const char *keyfile_name)
> > +{
> > +	struct keyfile_name_entry {
> > +		struct keyfile_name_entry *next;
> > +		char name[];
> > +	} *entry;
> > +	static struct keyfile_name_entry *keyfile_names = NULL;
> > +
> > +	for (entry = keyfile_names; entry != NULL; entry = entry->next) {
> > +		if (strcmp(entry->name, keyfile_name) == 0)
> > +			return 1;
> > +	}
> > +
> > +	entry = malloc(sizeof(struct keyfile_name_entry) +
> > +			strlen(keyfile_name) + 1);
> > +	if (entry) {
> > +		strcpy(entry->name, keyfile_name);
> > +		entry->next = keyfile_names;
> > +		keyfile_names = entry;
> > +	}
> > +	return 0;
> > +}
> > +
> >  EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
> >  {
> >  	FILE *fp;
> >  	X509 *crt = NULL;
> >  	EVP_PKEY *pkey = NULL;
> > +	int found;
> >  
> >  	if (!keyfile)
> >  		return NULL;
> >  
> >  	fp = fopen(keyfile, "r");
> >  	if (!fp) {
> > -		log_err("Failed to open keyfile: %s\n", keyfile);
> > +		found = lookup_keyfile_name(keyfile);
> > +		if (!found)
> > +			log_err("Failed to open keyfile: %s\n", keyfile);
> >  		return NULL;
> 
> 
> Now filename list is decoupled from keys themselves. Also we have key
> list creation in init_public_keys(). Maybe we should just always call
> init_public_keys for verify operations?

Initially, I tried that.  The code snippet, below, would be called
once.  The problem is that only during verify_hash() do we know which
default public key file to use.

if (params.keyfile)
    params.keyfile = <default key file>
		
init_public_keys(params.keyfile);

Mimi

> 
> Thanks,
> 
> >  	}
> >  
> > -- 
> > 2.7.5
> 

Re: [PATCH 2/2] ima_evm_utils: limit duplicate "Failed to open keyfile" messages

[Mimi Zohar]

Vitaly,

On Wed, 2019-07-17 at 00:49 +0300, Vitaly Chikunov wrote:
> >  EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
> >  {
> >  	FILE *fp;
> >  	X509 *crt = NULL;
> >  	EVP_PKEY *pkey = NULL;
> > +	int found;
> >  
> >  	if (!keyfile)
> >  		return NULL;
> >  
> >  	fp = fopen(keyfile, "r");
> >  	if (!fp) {
> > -		log_err("Failed to open keyfile: %s\n", keyfile);
> > +		found = lookup_keyfile_name(keyfile);
> > +		if (!found)
> > +			log_err("Failed to open keyfile: %s\n", keyfile);
> >  		return NULL;
> 
> 
> Now filename list is decoupled from keys themselves. Also we have key
> list creation in init_public_keys(). Maybe we should just always call
> init_public_keys for verify operations?

With v2 of "ima_evm_utils: erroneous "verification failed: 0 (invalid
padding)" message", I was able to drop this patch.

Mimi

Re: [PATCH 1/2] ima_evm_utils: erroneous "verification failed: 0 (invalid padding)" message

[Vitaly Chikunov]

Mimi,

On Tue, Jul 16, 2019 at 10:30:16AM -0400, Mimi Zohar wrote:
> When keys are not provided, the default key is used to verify the file
> signature, resulting in this erroneous message.  Before using the default
> key to verify the file signature, verify the keyid is correct.

1. What is default key when keys are not provided?
2. I don't see keyid verification in the patch.
3. Now we have so complicated keyfile handling.

> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  src/libimaevm.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index ae487f9fe36c..472ab53c7b42 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -302,6 +302,9 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
>  	X509 *crt = NULL;
>  	EVP_PKEY *pkey = NULL;
>  
> +	if (!keyfile)
> +		return NULL;
> +
>  	fp = fopen(keyfile, "r");
>  	if (!fp) {
>  		log_err("Failed to open keyfile: %s\n", keyfile);
> @@ -569,27 +572,25 @@ 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;
> -	int x509;
> +	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;
> +
>  		/* Read pubkey from RSA key */
> -		x509 = 0;
> +		if (!params.keyfile)
> +			key = "/etc/keys/pubkey_evm.pem";
>  	} else if (sig[0] == DIGSIG_VERSION_2) {
>  		verify_hash = verify_hash_v2;
> +
>  		/* Read pubkey from x509 cert */
> -		x509 = 1;
> +		if (!params.keyfile)
> +			init_public_keys("/etc/keys/x509_evm.der");

Also, in "ima-evm-utils: Preload public keys for ima_verify" I call
init_public_keys in cmd_verify_ima() which calls this verify_hash().

So there will be double calling of init_public_keys().

ps. Btw, I think we should remove this verify_hash_fn_t indirect call
trick and replace it with two normal calls in each if branch.

verify_hash() calling verify_hash() obscures cscope, and with direct
calls it will be easier to parse. I may send patch for this.

Thanks,


>  	} else
>  		return -1;
>  
> -	/* Determine what key to use for verification*/
> -	key = params.keyfile ? : x509 ?
> -			"/etc/keys/x509_evm.der" :
> -			"/etc/keys/pubkey_evm.pem";
> -
>  	return verify_hash(file, hash, size, sig, siglen, key);
>  }
>  
> -- 
> 2.7.5

Re: [PATCH 1/2] ima_evm_utils: erroneous "verification failed: 0 (invalid padding)" message

[Mimi Zohar]

On Wed, 2019-07-17 at 00:37 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Tue, Jul 16, 2019 at 10:30:16AM -0400, Mimi Zohar wrote:
> > When keys are not provided, the default key is used to verify the file
> > signature, resulting in this erroneous message.  Before using the default
> > key to verify the file signature, verify the keyid is correct.
> 
> 1. What is default key when keys are not provided?

The defaults was either "/etc/keys/x509_evm.der" for DIGSIG_VERSION_1
or "/etc/keys/pubkey_evm.pem" for DIGSIG_VERSION_2.

> 2. I don't see keyid verification in the patch.

Since no keys were provided, this patch loads the default key onto the
list of public_keys.  The normal find_keyid() is then called.

> 3. Now we have so complicated keyfile handling.

It would definitely be simpler to remove support for these default
keys, but I'm hesitant to remove it.

> 
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  src/libimaevm.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index ae487f9fe36c..472ab53c7b42 100644
> > --- a/src/libimaevm.c
> > +++ b/src/libimaevm.c
> > @@ -302,6 +302,9 @@ EVP_PKEY *read_pub_pkey(const char *keyfile, int x509)
> >  	X509 *crt = NULL;
> >  	EVP_PKEY *pkey = NULL;
> >  
> > +	if (!keyfile)
> > +		return NULL;
> > +
> >  	fp = fopen(keyfile, "r");
> >  	if (!fp) {
> >  		log_err("Failed to open keyfile: %s\n", keyfile);
> > @@ -569,27 +572,25 @@ 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;
> > -	int x509;
> > +	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;
> > +
> >  		/* Read pubkey from RSA key */
> > -		x509 = 0;
> > +		if (!params.keyfile)
> > +			key = "/etc/keys/pubkey_evm.pem";
> >  	} else if (sig[0] == DIGSIG_VERSION_2) {
> >  		verify_hash = verify_hash_v2;
> > +
> >  		/* Read pubkey from x509 cert */
> > -		x509 = 1;
> > +		if (!params.keyfile)
> > +			init_public_keys("/etc/keys/x509_evm.der");
> 
> Also, in "ima-evm-utils: Preload public keys for ima_verify" I call
> init_public_keys in cmd_verify_ima() which calls this verify_hash().
> 
> So there will be double calling of init_public_keys().

init_public_keys() will only be called once, either there or here.  It
depends on whether params.keyfile is defined or not.

> 
> ps. Btw, I think we should remove this verify_hash_fn_t indirect call
> trick and replace it with two normal calls in each if branch.
> 
> verify_hash() calling verify_hash() obscures cscope, and with direct
> calls it will be easier to parse. I may send patch for this.
> 
> Thanks,

Agreed, every time I come back to this code it takes me a few minutes
to remember.

Mimi

> 
> 
> >  	} else
> >  		return -1;
> >  
> > -	/* Determine what key to use for verification*/
> > -	key = params.keyfile ? : x509 ?
> > -			"/etc/keys/x509_evm.der" :
> > -			"/etc/keys/pubkey_evm.pem";
> > -
> >  	return verify_hash(file, hash, size, sig, siglen, key);
> >  }
> >  
> > -- 
> > 2.7.5