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

[Vitaly Chikunov <vt@altlinux.org>]

Mimi,

On Thu, Jul 18, 2019 at 02:14:46AM +0300, Vitaly Chikunov wrote:
> On Tue, Jul 16, 2019 at 09:36:29PM -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.
> > 
> > This patch adds the public key from the default x509 certificate onto the
> > "public_keys" list.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  src/evmctl.c    |  9 ++++++---
> >  src/libimaevm.c | 17 +++++++----------
> >  2 files changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 61808d276419..65cc5bd12bad 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -879,8 +879,10 @@ static int cmd_verify_ima(struct command *cmd)
> >  	char *file = g_argv[optind++];
> >  	int err;
> >  
> > -	if (params.keyfile)
> > +	if (params.keyfile)	/* Support multiple public keys */
> >  		init_public_keys(params.keyfile);
> > +	else			/* assume read pubkey from x509 cert */
> > +		init_public_keys("/etc/keys/x509_evm.der");
> >  
> >  	errno = 0;
> >  	if (!file) {
> > @@ -1602,9 +1604,10 @@ static int ima_measurement(const char *file)
> >  		return -1;
> >  	}
> >  
> > -	/* Support multiple public keys */
> > -	if (params.keyfile)
> > +	if (params.keyfile)	/* Support multiple public keys */
> >  		init_public_keys(params.keyfile);
> > +	else			/* assume read pubkey from x509 cert */
> > +		init_public_keys("/etc/keys/x509_evm.der");
> >  
> >  	while (fread(&entry.header, sizeof(entry.header), 1, fp)) {
> >  		ima_extend_pcr(pcr[entry.header.pcr], entry.header.digest,
> > diff --git a/src/libimaevm.c b/src/libimaevm.c
> > index ae487f9fe36c..afd21051b09a 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,21 @@ 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";
> 
> There is only three code path reaching here:
> 
> 1. From cmd_ima_measurement - calls init_public_keys.
> 2. From cmd_verify_ima - calls init_public_keys.
> 3. From cmd_verify_evm - probably it should call init_public_keys too.
> 
> Otherwise this change looks, good. When `--key` is not specified load
> default public key from x509_evm.der, but for signature v1 pass
> pubkey_evm.pem into verify_hash_v1.
> 
> As a consequence, verify_hash_v2 should remove code handling `keyfile`
> argument (maybe with argument itself) because it's now always NULL, and
> just call find_keyid.

Btw, there is strange code in evmctl.c:cmd_convert():

        params.x509 = 0;

        inkey = g_argv[optind++];
        if (!inkey) {
                inkey = params.x509 ? "/etc/keys/x509_evm.der" :
                                      "/etc/keys/pubkey_evm.pem";
        }

Assigning zero to params.x509 makes `params.x509 ? ... : ...` redundant.

Thanks,