Fix handling of NULL sig parameter in ECDSA_sign and similar #23529
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex.
The handling of sig=NULL was broken in this function, but since it is only used internally and was never called with sig=NULL, it is better to return an error in that case.
bernd-edlinger
added
branch: master
bernd-edlinger
force-pushed
the
fix_handling_of_null_sig_in_ecdsa_sign
branch
from
6a93305
to
747343a
Compare
|
I've split the PR in two commits, the first one will cherry-pick to all branches, and the second one is for 3.2+master only. |
github-actions
bot
added
the
severity: fips change
t8m
requested changes
Feb 9, 2024
t8m
added
triaged: bug
t8m
approved these changes
Feb 9, 2024
t8m
removed
the
approval: otc review pending
slontis
reviewed
Feb 14, 2024
| @@ -157,6 +157,11 @@ int ossl_dsa_sign_int(int type, const unsigned char *dgst, int dlen, | |||
| { | |||
| DSA_SIG *s; | |||
|
|
|||
| if (sig == NULL) { | |||
There was a problem hiding this comment.
would it be safe to move this code down below the legacy case (line 165-174)?
There was a problem hiding this comment.
No, I don't think so. Which branch is taken depends on the dsa object,
that is passed to DSA_sign()
only thing that is sure that both possible ways will not create a deterministic
signature, thus the correct thing to do is return the max. possible length here.
There was a problem hiding this comment.
What I am asking is, is there any way that line 172 should be run instead of the code you now run when sig is NULL
There was a problem hiding this comment.
I dont see why, the user is just asking for the memory size he needs to allocate,
and of course any error may happen when the call is repeated with sig != NULL.
This replicates the code pattern how EVP_DigestSign is to be used:
EVP_DigestSignInit(md_ctx, NULL, EVP_sha256(), NULL, dsa);
EVP_DigestSign(md_ctx, NULL, &sig_len, tbs, tbs_len);
sig = (unsigned char*) OPENSSL_malloc(sig_len);
EVP_DigestSign(md_ctx, sig, &sig_len, tbs, tbs_len);just with DSA_sign like this:
DSA_sign(NID_sha256, dgst, dgst_len, NULL, &sig_len, dsa);
sig = (unsigned char*) OPENSSL_malloc(sig_len);
DSA_sign(NID_sha256, dgst, dgst_len, sig, &sig_len, dsa);
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
beldmit
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
openssl-machine
pushed a commit
that referenced
this pull request
Apr 2, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23529) (cherry picked from commit 1fa2bf9)
openssl-machine
pushed a commit
that referenced
this pull request
Apr 2, 2024
The handling of sig=NULL was broken in this function, but since it is only used internally and was never called with sig=NULL, it is better to return an error in that case. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23529) (cherry picked from commit 294782f)
openssl-machine
pushed a commit
that referenced
this pull request
Apr 2, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23529) (cherry picked from commit 1fa2bf9)
openssl-machine
pushed a commit
that referenced
this pull request
Apr 2, 2024
The handling of sig=NULL was broken in this function, but since it is only used internally and was never called with sig=NULL, it is better to return an error in that case. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23529) (cherry picked from commit 294782f)
openssl-machine
pushed a commit
that referenced
this pull request
Apr 2, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23529)
openssl-machine
pushed a commit
that referenced
this pull request
Apr 2, 2024
The handling of sig=NULL was broken in this function, but since it is only used internally and was never called with sig=NULL, it is better to return an error in that case. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23529)
openssl-machine
pushed a commit
that referenced
this pull request
Apr 2, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23529) (cherry picked from commit 1fa2bf9)
|
Merged to all the active branches. (The deterministic ecdsa fix only to branches where this is present.) Thank you for your contribution. |
openssl-machine
pushed a commit
that referenced
this pull request
Apr 2, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23529) (cherry picked from commit 1fa2bf9)
bernd-edlinger
added a commit
to bernd-edlinger/openssl
that referenced
this pull request
Apr 5, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#23529) (cherry picked from commit 1fa2bf9)
bernd-edlinger
added a commit
to bernd-edlinger/openssl
that referenced
this pull request
Apr 5, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#23529) (cherry picked from commit 1fa2bf9)
jvdsn
pushed a commit
to jvdsn/openssl
that referenced
this pull request
Jun 3, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#23529)
jvdsn
pushed a commit
to jvdsn/openssl
that referenced
this pull request
Jun 3, 2024
The handling of sig=NULL was broken in this function, but since it is only used internally and was never called with sig=NULL, it is better to return an error in that case. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#23529)
eclipse-oniro-oh-bot
pushed a commit
to eclipse-oniro-mirrors/third_party_openssl
that referenced
this pull request
Jul 5, 2024
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature. But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption. Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#23529) (cherry picked from commit 1fa2bf9b1885d2e87524421fea5041d40149cffa) Signed-off-by: hhhFun <fanghaojie@huawei.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem is, that it almost works to pass sig=NULL to the ECDSA_sign, ECDSA_sign_ex and DSA_sign, to compute the necessary space for the resulting signature.
But since the ECDSA signature is non-deterministic (except when ECDSA_sign_setup/ECDSA_sign_ex are used) the resulting length may be different when the API is called again. This can easily cause random memory corruption.
Several internal APIs had the same issue, but since they are never called with sig=NULL, it is better to make them return an error in that case, instead of making the code more complex.
Checklist