[2/2] ima_evm_utils: limit duplicate "Failed to open keyfile" messages
diff mbox series

Message ID 1563287417-31780-2-git-send-email-zohar@linux.ibm.com
State New
Headers show
Series
  • [1/2] ima_evm_utils: erroneous "verification failed: 0 (invalid padding)" message
Related show

Commit Message

Mimi Zohar July 16, 2019, 2:30 p.m. UTC
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(-)

Comments

Vitaly Chikunov July 16, 2019, 9:49 p.m. UTC | #1
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
Mimi Zohar July 16, 2019, 10:13 p.m. UTC | #2
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
>
Mimi Zohar July 17, 2019, 1:23 p.m. UTC | #3
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

Patch
diff mbox series

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