-
-
Notifications
You must be signed in to change notification settings - Fork 11k
[1.1.0-bp][ec] match built-in curves on EC_GROUP_new_from_ecparameters #9810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
romen
wants to merge
8
commits into
openssl:OpenSSL_1_1_0-stable
from
romen:ec_match_explicit_params_110
Closed
[1.1.0-bp][ec] match built-in curves on EC_GROUP_new_from_ecparameters #9810
romen
wants to merge
8
commits into
openssl:OpenSSL_1_1_0-stable
from
romen:ec_match_explicit_params_110
Conversation
This file contains hidden or 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
Description ----------- Upon `EC_GROUP_new_from_ecparameters()` check if the parameters match any of the built-in curves. If that is the case, return a new `EC_GROUP_new_by_curve_name()` object instead of the explicit parameters `EC_GROUP`. This affects all users of `EC_GROUP_new_from_ecparameters()`: - direct calls to `EC_GROUP_new_from_ecparameters()` - direct calls to `EC_GROUP_new_from_ecpkparameters()` with an explicit parameters argument - ASN.1 parsing of explicit parameters keys (as it eventually ends up calling `EC_GROUP_new_from_ecpkparameters()`) A parsed explicit parameter key will still be marked with the `OPENSSL_EC_EXPLICIT_CURVE` ASN.1 flag on load, so, unless programmatically forced otherwise, if the key is eventually serialized the output will still be encoded with explicit parameters, even if internally it is treated as a named curve `EC_GROUP`. Before this change, creating any `EC_GROUP` object using `EC_GROUP_new_from_ecparameters()`, yielded an object associated with the default generic `EC_METHOD`, but this was never guaranteed in the documentation. After this commit, users of the library that intentionally want to create an `EC_GROUP` object using a specific `EC_METHOD` can still explicitly call `EC_GROUP_new(foo_method)` and then manually set the curve parameters using `EC_GROUP_set_*()`. Motivation ---------- This has obvious performance benefits for the built-in curves with specialized `EC_METHOD`s and subtle but important security benefits: - the specialized methods have better security hardening than the generic implementations - optional fields in the parameter encoding, like the `cofactor`, cannot be leveraged by an attacker to force execution of the less secure code-paths for single point scalar multiplication - in general, this leads to reducing the attack surface Check the manuscript at https://arxiv.org/abs/1909.01785 for an in depth analysis of the issues related to this commit. It should be noted that `libssl` does not allow to negotiate explicit parameters (as per RFC 8422), so it is not directly affected by the consequences of using explicit parameters that this commit fixes. On the other hand, we detected external applications and users in the wild that use explicit parameters by default (and sometimes using 0 as the cofactor value, which is technically not a valid value per the specification, but is tolerated by parsers for wider compatibility given that the field is optional). These external users of `libcrypto` are exposed to these vulnerabilities and their security will benefit from this commit. Related commits --------------- While this commit is beneficial for users using built-in curves and explicit parameters encoding for serialized keys, commit b783bee (and its equivalents for the 1.0.2, 1.1.0 and 1.1.1 stable branches) fixes the consequences of the invalid cofactor values more in general also for other curves (CVE-2019-1547). The following list covers commits in `master` that are related to the vulnerabilities presented in the manuscript motivating this commit: - d2baf88 [crypto/rsa] Set the constant-time flag in multi-prime RSA too - 311e903 [crypto/asn1] Fix multiple SCA vulnerabilities during RSA key validation. - b783bee [crypto/ec] for ECC parameters with NULL or zero cofactor, compute it - 724339f Fix SCA vulnerability when using PVK and MSBLOB key formats Note that the PRs that contributed the listed commits also include other commits providing related testing and documentation, in addition to links to PRs and commits backporting the fixes to the 1.0.2, 1.1.0 and 1.1.1 branches. Responsible Disclosure ---------------------- This and the other issues presented in https://arxiv.org/abs/1909.01785 were reported by Cesar Pereida García, Sohaib ul Hassan, Nicola Tuveri, Iaroslav Gridin, Alejandro Cabrera Aldaya and Billy Bob Brumley from the NISEC group at Tampere University, FINLAND. The OpenSSL Security Team evaluated the security risk for this vulnerability as low, and encouraged to propose fixes using public Pull Requests. _______________________________________________________________________________
…n curves This commit includes a partial backport of openssl#8555 (commit 8402cd5) for which the main author is Shane Lontis <shane.lontis@oracle.com> Co-authored-by: Shane Lontis <shane.lontis@oracle.com>
Fixup in a single commit before the final merge, keep as a separate commit until then to simplify the backport process.
… curves `NID_wap_wsg_idm_ecid_wtls12` and `NID_secp224r1` are both aliases for the same curve, we prefer the SECP nid when matching explicit parameters as that is associated with a specialized EC_METHOD.
[extended tests]
2 tasks
… curves Improve comment and swap LH/RH in comparisions
…uilt-in curves Avoid EC_GROUP_clear_free(): EC_GROUP_free() is sufficient as nothing confidential is stored in the EC_GROUP structure
mattcaswell
approved these changes
Sep 9, 2019
Member
Author
|
Merging now. Editorial note: I will edit the commit title as |
levitte
pushed a commit
that referenced
this pull request
Sep 9, 2019
Description ----------- Upon `EC_GROUP_new_from_ecparameters()` check if the parameters match any of the built-in curves. If that is the case, return a new `EC_GROUP_new_by_curve_name()` object instead of the explicit parameters `EC_GROUP`. This affects all users of `EC_GROUP_new_from_ecparameters()`: - direct calls to `EC_GROUP_new_from_ecparameters()` - direct calls to `EC_GROUP_new_from_ecpkparameters()` with an explicit parameters argument - ASN.1 parsing of explicit parameters keys (as it eventually ends up calling `EC_GROUP_new_from_ecpkparameters()`) A parsed explicit parameter key will still be marked with the `OPENSSL_EC_EXPLICIT_CURVE` ASN.1 flag on load, so, unless programmatically forced otherwise, if the key is eventually serialized the output will still be encoded with explicit parameters, even if internally it is treated as a named curve `EC_GROUP`. Before this change, creating any `EC_GROUP` object using `EC_GROUP_new_from_ecparameters()`, yielded an object associated with the default generic `EC_METHOD`, but this was never guaranteed in the documentation. After this commit, users of the library that intentionally want to create an `EC_GROUP` object using a specific `EC_METHOD` can still explicitly call `EC_GROUP_new(foo_method)` and then manually set the curve parameters using `EC_GROUP_set_*()`. Motivation ---------- This has obvious performance benefits for the built-in curves with specialized `EC_METHOD`s and subtle but important security benefits: - the specialized methods have better security hardening than the generic implementations - optional fields in the parameter encoding, like the `cofactor`, cannot be leveraged by an attacker to force execution of the less secure code-paths for single point scalar multiplication - in general, this leads to reducing the attack surface Check the manuscript at https://arxiv.org/abs/1909.01785 for an in depth analysis of the issues related to this commit. It should be noted that `libssl` does not allow to negotiate explicit parameters (as per RFC 8422), so it is not directly affected by the consequences of using explicit parameters that this commit fixes. On the other hand, we detected external applications and users in the wild that use explicit parameters by default (and sometimes using 0 as the cofactor value, which is technically not a valid value per the specification, but is tolerated by parsers for wider compatibility given that the field is optional). These external users of `libcrypto` are exposed to these vulnerabilities and their security will benefit from this commit. Related commits --------------- While this commit is beneficial for users using built-in curves and explicit parameters encoding for serialized keys, commit b783bee (and its equivalents for the 1.0.2, 1.1.0 and 1.1.1 stable branches) fixes the consequences of the invalid cofactor values more in general also for other curves (CVE-2019-1547). The following list covers commits in `master` that are related to the vulnerabilities presented in the manuscript motivating this commit: - d2baf88 [crypto/rsa] Set the constant-time flag in multi-prime RSA too - 311e903 [crypto/asn1] Fix multiple SCA vulnerabilities during RSA key validation. - b783bee [crypto/ec] for ECC parameters with NULL or zero cofactor, compute it - 724339f Fix SCA vulnerability when using PVK and MSBLOB key formats Note that the PRs that contributed the listed commits also include other commits providing related testing and documentation, in addition to links to PRs and commits backporting the fixes to the 1.0.2, 1.1.0 and 1.1.1 branches. This commit includes a partial backport of #8555 (commit 8402cd5) for which the main author is Shane Lontis. Responsible Disclosure ---------------------- This and the other issues presented in https://arxiv.org/abs/1909.01785 were reported by Cesar Pereida García, Sohaib ul Hassan, Nicola Tuveri, Iaroslav Gridin, Alejandro Cabrera Aldaya and Billy Bob Brumley from the NISEC group at Tampere University, FINLAND. The OpenSSL Security Team evaluated the security risk for this vulnerability as low, and encouraged to propose fixes using public Pull Requests. _______________________________________________________________________________ Co-authored-by: Shane Lontis <shane.lontis@oracle.com> (Backport from #9808) Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #9810)
Member
Author
|
Merged to Thanks! |
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.
This is a backport of #9808 for
1.1.0-stable.See the description in #9808 for a full report.
This commit includes a partial backport of #8555 (commit 8402cd5)
for which the main author is @slontis .
I had to drop the new tests as porting them to the older test architecture seems like a major effort.
I did manually spot check that the expected new behavior is happening, but not as extensively as the original testing.
I am triggering the extended tests for good measure.
Checklist