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

Message ID 1563287417-31780-1-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
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(-)

Comments

Vitaly Chikunov July 16, 2019, 9:37 p.m. UTC | #1
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
Mimi Zohar July 16, 2019, 9:55 p.m. UTC | #2
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

Patch
diff mbox series

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