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@xxxxxxxxxxxxx>
> > ---
> > 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,
