Add EVP_PKEY_supports_digest_nid() and use it for TLS CertificateVerify#7408
Add EVP_PKEY_supports_digest_nid() and use it for TLS CertificateVerify#7408dwmw2 wants to merge 3 commits intoopenssl:masterfrom
Conversation
|
Hm, this should check for |
Done. |
|
Hm, and now it appears to fail because our default software EC keys say that SHA256 is mandatory and they can't do anything else? |
ca6aec5 to
6418082
Compare
|
Added backports (without the new https://github.com/dwmw2/openssl/commits/pkey_md_1.1.1 |
|
Also, I believe the backports should be in different PRs. |
|
Ping to @mattcaswell that reviewed the original issue for more comments. |
|
@romen does this look more like what you were expecting? |
romen
left a comment
There was a problem hiding this comment.
I would say most of my expectations have been met, thanks @dwmw2 for sticking with me!
I have a few more remarks, but it's mostly either about comments, or ping more knowledgeable reviewers on specific issues I am not entirely familiar with.
kaduk
left a comment
There was a problem hiding this comment.
Just chiming in where summoned; I didn't review the whole thing.
|
My intent is to push the first two of these as bug fixes for older versions of OpenSSL as far back as you'll have them, and the third only for 1.1.2 (or 1.2.0 or whatever's next). |
|
I can't work out how to make it stop saying that changes were requested. To my knowledge, nothing is outstanding. |
|
You have to "dismiss" the review, and in the text say "all requested changes were done" or similar. |
|
I have done so to all but the one which is 404 because the commits that were commented on are gone. |
|
Does this mean you don't have a button to dismiss the review? If not, maybe it's a matter of permissions. In this case, I can dismiss it for you. Or you just ping @romen.
|
|
Indeed, I don't have that button. Dismissing the stale review (especially if it's holding up the PR being merged) would be appreciated; thanks! |
ASN1_PKEY_CTRL_DEFAULT_MD_NID is documented to return 2 for a mandatory digest algorithm, when the key can't support any others. That isn't true here, so return 1 instead. Partially fixes openssl#7348
If the private key says it can only support one specific digest, then don't ask it to perform a different one. Fixes: openssl#7348
Rather than relying only on mandatory default digests, add a way for the EVP_PKEY to individually report whether each digest algorithm is supported.
|
Is this good to go? What branches is this for? |
|
This is for master. I was planning to update my backports once the dust was settled and it was merged; it needs massaging for 1.1.0 and I haven't even looked at 1.0.2 yet. For the backports, I would appreciate guidance: how far back should we backport it, and do we do just the first two commits which have a limited fix for the bug without adding the new API? Or is it OK to add the new |
|
1.1.0 is in security-fix only mode. So I'd suggest 1.1.1 and 1.0.2 only. Bug fixes only to stable branches so you shouldn't be adding the new function in those branches. |
|
OK, thanks. I'll do those once this is merged to master so I can reference the stable commit IDs. |
|
Backports (which will need the "cherry picked from…" commit IDs updated after this origiinal PR is merged: https://github.com/dwmw2/openssl/commits/pkey_md_1.1.1 The first is a trivial cherry-pick but for 1.0.2 it looks a little different. I was planning to submit a new PR for each once this one is merged and the commit IDs are known. In the meantime, @romen can you opine on dwmw2@ede8375 please? |
|
I can merge this to master now, and then we can discuss other commits in the 2 separate PRs for 1.0.2 and 1.1.1. @dwmw2 I had a look at dwmw2/openssl@ede8375 : it seems fine, even if a little bit confusing at first; but again I am not that familiar with libssl code, so I'll rely on better reviewers than me for that! |
ASN1_PKEY_CTRL_DEFAULT_MD_NID is documented to return 2 for a mandatory digest algorithm, when the key can't support any others. That isn't true here, so return 1 instead. Partially fixes #7348 Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #7408)
Rather than relying only on mandatory default digests, add a way for the EVP_PKEY to individually report whether each digest algorithm is supported. Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #7408)
ASN1_PKEY_CTRL_DEFAULT_MD_NID is documented to return 2 for a mandatory digest algorithm, when the key can't support any others. That isn't true here, so return 1 instead. Partially fixes openssl#7348 Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#7408) (cherry picked from commit eb7eb13)
If the private key says it can only support one specific digest, then don't ask it to perform a different one. Fixes: openssl#7348 Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#7408) (cherry picked from commit 2d263a4)
ASN1_PKEY_CTRL_DEFAULT_MD_NID is documented to return 2 for a mandatory digest algorithm, when the key can't support any others. That isn't true here, so return 1 instead. Partially fixes openssl#7348 Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#7408) (cherry picked from commit eb7eb13)
If the private key says it can only support one specific digest, then don't ask it to perform a different one. Fixes: openssl#7348 Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#7408) (cherry picked from commit 2d263a4 and reworked for 1.0.2)
The openssl#7408 implemented mandatory digest checking in TLS. However this broke compatibility of DSS support with GnuTLS which supports only SHA1 with DSS. There is no reason why SHA256 would be a mandatory digest for DSA as other digests in SHA family can be used as well.
The #7408 implemented mandatory digest checking in TLS. However this broke compatibility of DSS support with GnuTLS which supports only SHA1 with DSS. There is no reason why SHA256 would be a mandatory digest for DSA as other digests in SHA family can be used as well. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #9015)
The #7408 implemented mandatory digest checking in TLS. However this broke compatibility of DSS support with GnuTLS which supports only SHA1 with DSS. There is no reason why SHA256 would be a mandatory digest for DSA as other digests in SHA family can be used as well. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #9015) (cherry picked from commit cd4c83b)
Fixes #7348
I think it's correct not to filter our outbound advertisement. We can check the peer's signature with any digest; it's only our own signature where we're limited to what the EVP_PKEY can support.