fix CRMF symmetric key handling#9774
fix CRMF symmetric key handling#9774Akretsch wants to merge 5 commits intoopenssl:masterfrom Akretsch:crmf_fix
Conversation
crypto/crmf/crmf_lib.c
Outdated
| EVP_CIPHER_CTX_set_padding(evp_ctx, 0); | ||
|
|
||
| if (!EVP_DecryptInit(evp_ctx, cipher, ek, iv) | ||
| || eksize != (size_t)EVP_CIPHER_key_length(cipher) |
There was a problem hiding this comment.
you need to check the eksize immediatly after the EVP_PKEY_decrypt.
best practice would be to do that with constant time code.
and to call ERR_clear_error before looking at the result code.
There was a problem hiding this comment.
Hopefully I got it now.
There was a problem hiding this comment.
It meanwhile turned out the constant-time style of checking the decryption result is not needed in the given context - see #17319 (comment).
crypto/crmf/crmf_lib.c
Outdated
| size_t eksize = 0; | ||
|
|
||
| if (EVP_PKEY_decrypt(pkctx, NULL, &eksize, encKey->data, encKey->length) | ||
| <= 0 |
There was a problem hiding this comment.
Almost okay. I suggest:
|| EVP_PKEY_decrypt(pkctx, ek, &eksize, encKey->data,
encKey->length) <= 0
|| eksize != cikeysize) {
ERR_clear_error(); /* error state has sensitive information */
CRMFerr(CRMF_F_OSSL_CRMF_ENCRYPTEDVALUE_GET1_ENCCERT,
CRMF_R_ERROR_DECRYPTING_SYMMETRIC_KEY);
goto end;
}Bonus points, if you can do the result <= 0 || eksize != cikeysize in constant time.
There was a problem hiding this comment.
Hope I am closer to the bonus now. Thanks for your suggestions.
|
Thanks for help and guidance! Do I need a second reviewer for this PR? |
bernd-edlinger
added
branch: master
|
correct. |
|
@mattcaswell May I ask you for 2nd review of this PR to drive our #9107 a little bit forward? |
mattcaswell
added
approval: done
|
Pushed. Thanks. |
Reviewed-by: Bernd Edlinger <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #9774)
4 participants
Fixes related to review comments #7646 (comment) , #7646 (comment) , #7646 (comment) given by @bernd-edlinger