apps/speed.c: skip binary curves when compiling with NO_EC2M #8422
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
openssl speed doesn't take into account that the library could be compiled without the support for the binary curves and happily uses them, which results in EC_GROUP_new_by_curve_name() errors.
The ecdh_c array is allocated of the same size as ecdh_choices, whose size depends on whether the support for binary curves is enabled or not. (The same goes for ecdsa_c). On systems without SIGALRM, ecdh_c is indexed by predefined constants intended for representing the index of the ciphers in the ecdh_choices array. However, in case of NO_EC2M some of the #defined constants won't match and would actually access the ecdh_c out-of-bounds. Use enum instead of a macro to define the curve indexes so they're within the bounds of the ecdh_c array.
mattcaswell
added
branch: master
mattcaswell
approved these changes
Mar 7, 2019
mattcaswell
added
the
approval: review pending
paulidale
approved these changes
Mar 12, 2019
paulidale
added
approval: done
levitte
pushed a commit
that referenced
this pull request
Mar 19, 2019
openssl speed doesn't take into account that the library could be compiled without the support for the binary curves and happily uses them, which results in EC_GROUP_new_by_curve_name() errors. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8422) (cherry picked from commit d61f489)
levitte
pushed a commit
that referenced
this pull request
Mar 19, 2019
The ecdh_c array is allocated of the same size as ecdh_choices, whose size depends on whether the support for binary curves is enabled or not. (The same goes for ecdsa_c). On systems without SIGALRM, ecdh_c is indexed by predefined constants intended for representing the index of the ciphers in the ecdh_choices array. However, in case of NO_EC2M some of the #defined constants won't match and would actually access the ecdh_c out-of-bounds. Use enum instead of a macro to define the curve indexes so they're within the bounds of the ecdh_c array. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8422) (cherry picked from commit f5c9916)
|
Pushed. Thanks. |
levitte
pushed a commit
that referenced
this pull request
Mar 19, 2019
openssl speed doesn't take into account that the library could be compiled without the support for the binary curves and happily uses them, which results in EC_GROUP_new_by_curve_name() errors. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8422)
levitte
pushed a commit
that referenced
this pull request
Mar 19, 2019
The ecdh_c array is allocated of the same size as ecdh_choices, whose size depends on whether the support for binary curves is enabled or not. (The same goes for ecdsa_c). On systems without SIGALRM, ecdh_c is indexed by predefined constants intended for representing the index of the ciphers in the ecdh_choices array. However, in case of NO_EC2M some of the #defined constants won't match and would actually access the ecdh_c out-of-bounds. Use enum instead of a macro to define the curve indexes so they're within the bounds of the ecdh_c array. Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #8422)
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.
openssl speed doesn't take into account that the library could be compiled without the support for the binary curves and will happily uses them, which will results in EC_GROUP_new_by_curve_name() errors.
$ openssl speed -seconds 1 ecdh
[...]
Doing 521 bits ecdh's for 1s: 2355 521-bits ECDH ops in 1.00s
ECDH EC params init failure.
140292076904960:error:100AE081:elliptic curve routines:EC_GROUP_new_by_curve_name:unknown group:crypto/ec/ec_curve.c:3132:
140292076904960:error:100C508D:elliptic curve routines:pkey_ec_ctrl:invalid curve:crypto/ec/ec_pmeth.c:231:
The switch to enum from macros for defining constants is needed because of the following reason:
Just adding ifdefs isn't enough. Systems without SIGALRM need special care.
The ecdh_c array is allocated of the same size as ecdh_choices, whose size depends on whether the support for binary curves is enabled or not. (The same goes for ecdsa_c).
On systems without SIGALRM, ecdh_c is indexed by predefined constants (eg. R_EC_BRP256R1) intended for representing the index of the ciphers in the ecdh_choices array.
However, in case of NO_EC2M some of the #defined constants would become invalid because the size of the array shrunk. They would point after the ecdh_c array causing an out-of-bounds access.
Using enum instead of the macros to define the curve indexes will prevent the invalid access.