| Message ID | 20210106094335.3178261-2-patrick@puiterwijk.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima-evm-utils: Fix use of sign_hash via API | expand |
Hi Patrick, On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > This fixes sign_hash not using the correct algorithm for creating the > signature, by ensuring it uses the passed in variable value. > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> Thank you. This is a regression first introduced in commit 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). Mimi
Patrick, Mimi, On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote: > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > > This fixes sign_hash not using the correct algorithm for creating the > > signature, by ensuring it uses the passed in variable value. > > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > > Thank you. This is a regression first introduced in commit > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). Ah, when sign_hash() is used not via evmctl, but as a library imaevm_params may be not set. Thanks!
On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote: > Patrick, Mimi, > > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote: > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > > > This fixes sign_hash not using the correct algorithm for creating the > > > signature, by ensuring it uses the passed in variable value. > > > > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > > > > Thank you. This is a regression first introduced in commit > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). > > Ah, when sign_hash() is used not via evmctl, but as a library > imaevm_params may be not set. Thinking about imaevm_params -- it's still belong to a library and extensively used in libimaevm.c, so I wonder if the fix is perfect. Since, now there is hash_algo and algo duplication which one to prefer and why? Maybe, it should be [also] set like keypass in sign_hash? int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig) { if (keypass) imaevm_params.keypass = keypass; + if (hashalgo) + imaevm_params.hash_algo = hashalgo; return imaevm_params.x509 ? sign_hash_v2(hashalgo, hash, size, keyfile, sig) : sign_hash_v1(hashalgo, hash, size, keyfile, sig); }
On Thu, 2021-01-07 at 16:15 +0300, Vitaly Chikunov wrote: > On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote: > > Patrick, Mimi, > > > > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote: > > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > > > > This fixes sign_hash not using the correct algorithm for creating the > > > > signature, by ensuring it uses the passed in variable value. > > > > > > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > > > > > > Thank you. This is a regression first introduced in commit > > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). > > > > Ah, when sign_hash() is used not via evmctl, but as a library > > imaevm_params may be not set. > > Thinking about imaevm_params -- it's still belong to a library and > extensively used in libimaevm.c, so I wonder if the fix is perfect. > Since, now there is hash_algo and algo duplication which one to prefer > and why? > > Maybe, it should be [also] set like keypass in sign_hash? > > int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig) > { > if (keypass) > imaevm_params.keypass = keypass; > > + if (hashalgo) > + imaevm_params.hash_algo = hashalgo; > > return imaevm_params.x509 ? > sign_hash_v2(hashalgo, hash, size, keyfile, sig) : > sign_hash_v1(hashalgo, hash, size, keyfile, sig); > } > > As seen above, the main difference between keypass and hashalgo is that hashalgo is being passed as an argument, while keypass isn't. Instead of changing the function arguments, it looks like keypass was stored as global variable. For this reason, the priority should be the function argument, not the global variable. thanks, Mimi
On Thu, Jan 7, 2021 at 2:15 PM Vitaly Chikunov <vt@altlinux.org> wrote: > > On Thu, Jan 07, 2021 at 04:08:16PM +0300, Vitaly Chikunov wrote: > > Patrick, Mimi, > > > > On Thu, Jan 07, 2021 at 07:24:43AM -0500, Mimi Zohar wrote: > > > On Wed, 2021-01-06 at 10:43 +0100, Patrick Uiterwijk wrote: > > > > This fixes sign_hash not using the correct algorithm for creating the > > > > signature, by ensuring it uses the passed in variable value. > > > > > > > > Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> > > > > > > Thank you. This is a regression first introduced in commit > > > 07e623b60848 ("ima-evm-utils: Convert sign_hash_v2 to EVP_PKEY API"). > > > > Ah, when sign_hash() is used not via evmctl, but as a library > > imaevm_params may be not set. > > Thinking about imaevm_params -- it's still belong to a library and > extensively used in libimaevm.c, so I wonder if the fix is perfect. > Since, now there is hash_algo and algo duplication which one to prefer > and why? > > Maybe, it should be [also] set like keypass in sign_hash? I had this exact diff as an initial version of this patch, but then personally thought that because "hashalgo" is passed to sign_hash_v2 anyway, and sign_hash_v1 already prefers the argument (and ignores imaevm_params.hash_algo), keeping this behaviour in sync is more consistent. With my patch, imaevm_params.hash_algo is only used in verify_hash_v2 and ima_calc_hash, both of which are primarily called by ima_verify_signature which sets it, as a way to pass the argument without having it explicit everywhere. > > int sign_hash(const char *hashalgo, const unsigned char *hash, int size, const char *keyfile, const char *keypass, unsigned char *sig) > { > if (keypass) > imaevm_params.keypass = keypass; > > + if (hashalgo) > + imaevm_params.hash_algo = hashalgo; > > return imaevm_params.x509 ? > sign_hash_v2(hashalgo, hash, size, keyfile, sig) : > sign_hash_v1(hashalgo, hash, size, keyfile, sig); > } > >
diff --git a/src/libimaevm.c b/src/libimaevm.c index fa6c278..72d5e67 100644 --- a/src/libimaevm.c +++ b/src/libimaevm.c @@ -916,7 +916,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash, return -1; } - log_info("hash(%s): ", imaevm_params.hash_algo); + log_info("hash(%s): ", algo); log_dump(hash, size); pkey = read_priv_pkey(keyfile, imaevm_params.keypass); @@ -942,7 +942,7 @@ static int sign_hash_v2(const char *algo, const unsigned char *hash, if (!EVP_PKEY_sign_init(ctx)) goto err; st = "EVP_get_digestbyname"; - if (!(md = EVP_get_digestbyname(imaevm_params.hash_algo))) + if (!(md = EVP_get_digestbyname(algo))) goto err; st = "EVP_PKEY_CTX_set_signature_md"; if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
This fixes sign_hash not using the correct algorithm for creating the signature, by ensuring it uses the passed in variable value. Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> --- src/libimaevm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)