s390x: Check and fail on invalid malformed ECDSA signatures#29214
s390x: Check and fail on invalid malformed ECDSA signatures#29214holger-dengler wants to merge 2 commits intoopenssl:masterfrom
Conversation
ab6bb39 to
67a08d7
Compare
ifranzki
left a comment
There was a problem hiding this comment.
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:
openssl/crypto/ec/ecdsa_ossl.c
Line 544 in abcf402
ossl_ecdsa_simple_verify_sig()....
Besides that, looks good to me.
|
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. |
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. |
|
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. |
67a08d7 to
b96af39
Compare
|
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? |
|
@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. |
|
Thx! |
|
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. |
|
@holger-dengler the wycheproof tests passes. Tested on RHEL-10.1. |
|
@gstarovo can you please confirm? |
@beldmit @gstarovo confirmed at #29214 (comment) |
|
No, thanks |
|
This pull request is ready to merge |
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)
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)
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)
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)
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)
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)
|
Merged to all the active branches. Thank you for your contribution. |
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)
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)
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)
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)
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)
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)
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)
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)
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
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
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
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
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
Check parameters of ECDSA signatures on verify and fail for invalid malformed signarures, even in the code path for s390x accelerators.
Fixes: #29173