Skip to content

Comments

s390x: Check and fail on invalid malformed ECDSA signatures#29214

Closed
holger-dengler wants to merge 2 commits intoopenssl:masterfrom
holger-dengler:ecdsa-param-check
Closed

s390x: Check and fail on invalid malformed ECDSA signatures#29214
holger-dengler wants to merge 2 commits intoopenssl:masterfrom
holger-dengler:ecdsa-param-check

Conversation

@holger-dengler
Copy link
Contributor

Check parameters of ECDSA signatures on verify and fail for invalid malformed signarures, even in the code path for s390x accelerators.

Fixes: #29173

Copy link
Contributor

@ifranzki ifranzki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat related to your change:

Your added code uses ERR_raise(ERR_LIB_EC, EC_R_BAD_SIGNATURE); when the signature is wrong due to R and S being out of range, however, when further down the KDSA instruction detects that the signature is invalid, no ERR_raise(ERR_LIB_EC, EC_R_BAD_SIGNATURE); is used. Should this be added?

On the other hand, ossl_ecdsa_simple_verify_sig() also does not add an error on its final BN_ucmp() call to verify the signature:

ret = (BN_ucmp(u1, sig->r) == 0);
In so far, our code behaves the same as ossl_ecdsa_simple_verify_sig()....

Besides that, looks good to me.

@holger-dengler
Copy link
Contributor Author

The patch also applies on branch openssl-3.0, openssl-3.2, openssl-3.3, openssl-3,4, openssl-3,5 and openssl-3,6.

@holger-dengler
Copy link
Contributor Author

Somewhat related to your change:

Your added code uses ERR_raise(ERR_LIB_EC, EC_R_BAD_SIGNATURE); when the signature is wrong due to R and S being out of range, however, when further down the KDSA instruction detects that the signature is invalid, no ERR_raise(ERR_LIB_EC, EC_R_BAD_SIGNATURE); is used. Should this be added?

I think, it's more a kind of tri-state thing. A signature can either be regular or malformed. While a malformed signature is an error case (bad input), the regular case split into the two normal verification cases. Both are, in my opinion, true/false cases and only the caller can treat them as error or not. From a library point of view, both cases are normal resulty and not errors.

But that's only my view, I've no idea if this is generic view.

@t8m t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer branch: 3.0 Applies to openssl-3.0 branch branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Nov 25, 2025
t8m
t8m previously approved these changes Nov 25, 2025
@t8m t8m requested a review from a team November 25, 2025 18:19
beldmit
beldmit previously approved these changes Nov 25, 2025
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@gstarovo, could you please test it?

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 25, 2025
@holger-dengler
Copy link
Contributor Author

I looked into this now for a while and there will be a new version with optimizations. So please do not merge this PR at the moment.

@holger-dengler holger-dengler dismissed stale reviews from beldmit and t8m via b96af39 November 26, 2025 16:22
@holger-dengler
Copy link
Contributor Author

The new version of the PR tries to avoid duplicate checkings. The kdsa instruction is already doing some checks for r and s, so only the missing ones (r/s negative or too large) are done in addition. The openssl tests all pass and also the both wycheproof test-vectors fail as expected. Unfortunately, I've not been able to setup an env to run all wycheproof tests. @gstarovo, could you jump in?

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Nov 26, 2025
@gstarovo
Copy link
Contributor

@holger-dengler sure. I have tested the previous version of the PR on RHEL-10.1 and all the tests passed. I will retest with the new version.

@holger-dengler
Copy link
Contributor Author

Thx!

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@gstarovo
Copy link
Contributor

@holger-dengler the wycheproof tests passes. Tested on RHEL-10.1.

@beldmit
Copy link
Member

beldmit commented Nov 27, 2025

@gstarovo can you please confirm?

@ifranzki
Copy link
Contributor

ifranzki commented Dec 1, 2025

@gstarovo can you please confirm?

@beldmit @gstarovo confirmed at #29214 (comment)
Do you need another confirmation?

@beldmit
Copy link
Member

beldmit commented Dec 1, 2025

No, thanks

Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 1, 2025
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Dec 2, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
The kdsa instruction is doing some parameter checking for the verify
function codes, like r/s equals zero and range checks. To handle these
cases correctly in the calling functions, the asm returns now also
condition code 2.

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit bcaa2a3)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
Check parameters of ECDSA signatures on verify and fail for invalid
malformed signatures in the code path for s390x accelerators. Handle
condition code of kdsa instruction for detecting invalid parameters.

For NIST P521 curves, kdsa ignores completely the upper 14 bytes of
the sections for r and s in the parameter-block, so adapt the offset
and length for bignum conversions for these curves. This will detect
cases of malformed signatures which are not covered by the kdsa
parameter checking.

Fixes: #29173

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit 8e28ea9)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
The kdsa instruction is doing some parameter checking for the verify
function codes, like r/s equals zero and range checks. To handle these
cases correctly in the calling functions, the asm returns now also
condition code 2.

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit bcaa2a3)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
Check parameters of ECDSA signatures on verify and fail for invalid
malformed signatures in the code path for s390x accelerators. Handle
condition code of kdsa instruction for detecting invalid parameters.

For NIST P521 curves, kdsa ignores completely the upper 14 bytes of
the sections for r and s in the parameter-block, so adapt the offset
and length for bignum conversions for these curves. This will detect
cases of malformed signatures which are not covered by the kdsa
parameter checking.

Fixes: #29173

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit 8e28ea9)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
The kdsa instruction is doing some parameter checking for the verify
function codes, like r/s equals zero and range checks. To handle these
cases correctly in the calling functions, the asm returns now also
condition code 2.

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
Check parameters of ECDSA signatures on verify and fail for invalid
malformed signatures in the code path for s390x accelerators. Handle
condition code of kdsa instruction for detecting invalid parameters.

For NIST P521 curves, kdsa ignores completely the upper 14 bytes of
the sections for r and s in the parameter-block, so adapt the offset
and length for bignum conversions for these curves. This will detect
cases of malformed signatures which are not covered by the kdsa
parameter checking.

Fixes: #29173

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)
@t8m
Copy link
Member

t8m commented Dec 3, 2025

Merged to all the active branches. Thank you for your contribution.

@t8m t8m closed this Dec 3, 2025
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
The kdsa instruction is doing some parameter checking for the verify
function codes, like r/s equals zero and range checks. To handle these
cases correctly in the calling functions, the asm returns now also
condition code 2.

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit bcaa2a3)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
Check parameters of ECDSA signatures on verify and fail for invalid
malformed signatures in the code path for s390x accelerators. Handle
condition code of kdsa instruction for detecting invalid parameters.

For NIST P521 curves, kdsa ignores completely the upper 14 bytes of
the sections for r and s in the parameter-block, so adapt the offset
and length for bignum conversions for these curves. This will detect
cases of malformed signatures which are not covered by the kdsa
parameter checking.

Fixes: #29173

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit 8e28ea9)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
The kdsa instruction is doing some parameter checking for the verify
function codes, like r/s equals zero and range checks. To handle these
cases correctly in the calling functions, the asm returns now also
condition code 2.

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit bcaa2a3)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
Check parameters of ECDSA signatures on verify and fail for invalid
malformed signatures in the code path for s390x accelerators. Handle
condition code of kdsa instruction for detecting invalid parameters.

For NIST P521 curves, kdsa ignores completely the upper 14 bytes of
the sections for r and s in the parameter-block, so adapt the offset
and length for bignum conversions for these curves. This will detect
cases of malformed signatures which are not covered by the kdsa
parameter checking.

Fixes: #29173

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit 8e28ea9)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
The kdsa instruction is doing some parameter checking for the verify
function codes, like r/s equals zero and range checks. To handle these
cases correctly in the calling functions, the asm returns now also
condition code 2.

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit bcaa2a3)
openssl-machine pushed a commit that referenced this pull request Dec 3, 2025
Check parameters of ECDSA signatures on verify and fail for invalid
malformed signatures in the code path for s390x accelerators. Handle
condition code of kdsa instruction for detecting invalid parameters.

For NIST P521 curves, kdsa ignores completely the upper 14 bytes of
the sections for r and s in the parameter-block, so adapt the offset
and length for bignum conversions for these curves. This will detect
cases of malformed signatures which are not covered by the kdsa
parameter checking.

Fixes: #29173

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #29214)

(cherry picked from commit 8e28ea9)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
The kdsa instruction is doing some parameter checking for the verify
function codes, like r/s equals zero and range checks. To handle these
cases correctly in the calling functions, the asm returns now also
condition code 2.

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29214)
cxx194832 pushed a commit to cxx194832/openssl that referenced this pull request Dec 12, 2025
Check parameters of ECDSA signatures on verify and fail for invalid
malformed signatures in the code path for s390x accelerators. Handle
condition code of kdsa instruction for detecting invalid parameters.

For NIST P521 curves, kdsa ignores completely the upper 14 bytes of
the sections for r and s in the parameter-block, so adapt the offset
and length for bignum conversions for these curves. This will detect
cases of malformed signatures which are not covered by the kdsa
parameter checking.

Fixes: openssl#29173

Signed-off-by: Holger Dengler <[email protected]>

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#29214)
openssl-machine pushed a commit that referenced this pull request Jan 27, 2026
3.6.1 CHANGES.md includes the following:
 * #28760
   "Improve the CPUINFO display for RISC-V"
 * #28797
   "Fix regression when X509_V_FLAG_CRL_CHECK_ALL is set"
 * #28955
   "Fix for TLS handshake issue with GnuTLS #28902"
 * #29155
   "fix(x509.c): fixed -checkend return values"
 * #29214
   "s390x: Check and fail on invalid malformed ECDSA signatures"
 * #29245
   "Clang format 3.6"
 * #29251
   "Fix change of behavior of the single stapled OCSP response API"

3.6.1 NEWS.md includes the following:
 * #28797
   "Fix regression when X509_V_FLAG_CRL_CHECK_ALL is set"
 * #28955
   "Fix for TLS handshake issue with GnuTLS #28902"

Co-Authored-by: Tomáš Mráz <[email protected]>
Signed-off-by: Eugene Syromiatnikov <[email protected]>

Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
MergeDate: Mon Jan 26 20:01:30 2026
openssl-machine pushed a commit that referenced this pull request Jan 27, 2026
3.5.5 CHANGES.md includes the following:
 * #28760
   "Improve the CPUINFO display for RISC-V"
 * #29214
   "s390x: Check and fail on invalid malformed ECDSA signatures"
 * #29262
   "Clang format 3.5"

Co-Authored-by: Tomáš Mráz <[email protected]>
Signed-off-by: Eugene Syromiatnikov <[email protected]>

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
MergeDate: Mon Jan 26 20:03:05 2026
openssl-machine pushed a commit that referenced this pull request Jan 27, 2026
3.4.4 CHANGES.md includes the following:
 * #28760
   "Improve the CPUINFO display for RISC-V"
 * #29214
   "s390x: Check and fail on invalid malformed ECDSA signatures"
 * #29260
   "Clang format 3.4"

Co-Authored-by: Tomáš Mráz <[email protected]>
Signed-off-by: Eugene Syromiatnikov <[email protected]>

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
MergeDate: Mon Jan 26 20:04:13 2026
openssl-machine pushed a commit that referenced this pull request Jan 27, 2026
3.3.6 CHANGES.md includes the following:
 * #29214
   "s390x: Check and fail on invalid malformed ECDSA signatures"
 * #29258
   "Clang format 3.3"

Co-Authored-by: Tomáš Mráz <[email protected]>
Signed-off-by: Eugene Syromiatnikov <[email protected]>

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
MergeDate: Mon Jan 26 20:04:58 2026
openssl-machine pushed a commit that referenced this pull request Jan 27, 2026
3.0.19 CHANGES.md includes the following:
 * #29214
   "s390x: Check and fail on invalid malformed ECDSA signatures"
 * #29256
   "Clang format 3.0"

Co-Authored-by: Tomáš Mráz <[email protected]>
Signed-off-by: Eugene Syromiatnikov <[email protected]>

Reviewed-by: Saša Nedvědický <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
MergeDate: Mon Jan 26 20:05:45 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accepting invalid signature on s390x

7 participants