| Message ID | 1563287417-31780-1-git-send-email-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ima_evm_utils: erroneous "verification failed: 0 (invalid padding)" message | expand |
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
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
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); }
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(-)