[openssl] OpenSSL_1_0_2-stable update
Matt Caswell
matt at openssl.org
Mon Sep 9 16:01:39 UTC 2019
The branch OpenSSL_1_0_2-stable has been updated
via 66fd724daa89996bb775ab8911479c11143b44df (commit)
from 21c856b75d81eff61aa63b4f036bb64a85bf6d46 (commit)
- Log -----------------------------------------------------------------
commit 66fd724daa89996bb775ab8911479c11143b44df
Author: Nicola Tuveri <nic.tuv at gmail.com>
Date: Sat Sep 7 18:05:31 2019 +0300
[ec] Match built-in curves on EC_GROUP_new_from_ecparameters
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
b783beeadf6b80bc431e6f3230b5d5585c87ef87 (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:
- d2baf88c43 [crypto/rsa] Set the constant-time flag in multi-prime RSA too
- 311e903d84 [crypto/asn1] Fix multiple SCA vulnerabilities during RSA key validation.
- b783beeadf [crypto/ec] for ECC parameters with NULL or zero cofactor, compute it
- 724339ff44 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
https://github.com/openssl/openssl/pull/8555
(commit 8402cd5f75f8c2f60d8bd39775b24b03dd8b3b38)
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 at oracle.com>
(Backport from https://github.com/openssl/openssl/pull/9808)
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9811)
-----------------------------------------------------------------------
Summary of changes:
CHANGES | 11 +++++
crypto/ec/ec_asn1.c | 76 +++++++++++++++++++++++++++++++---
crypto/ec/ec_curve.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++
crypto/ec/ec_lcl.h | 15 +++++++
crypto/ec/ec_lib.c | 57 ++++++++++++++++++++++++++
5 files changed, 268 insertions(+), 5 deletions(-)
diff --git a/CHANGES b/CHANGES
index ee272f2266..e9b467bd04 100644
--- a/CHANGES
+++ b/CHANGES
@@ -9,6 +9,17 @@
Changes between 1.0.2s and 1.0.2t [xx XXX xxxx]
+ *) For built-in EC curves, ensure an EC_GROUP built from the curve name is
+ used even when parsing explicit parameters, when loading a serialized key
+ or calling `EC_GROUP_new_from_ecpkparameters()`/
+ `EC_GROUP_new_from_ecparameters()`.
+ This prevents bypass of security hardening and performance gains,
+ especially for curves with specialized EC_METHODs.
+ By default, if a key encoded with explicit parameters is loaded and later
+ serialized, the output is still encoded with explicit parameters, even if
+ internally a "named" EC_GROUP is used for computation.
+ [Nicola Tuveri]
+
*) Compute ECC cofactors if not provided during EC_GROUP construction. Before
this change, EC_GROUP_set_generator would accept order and/or cofactor as
NULL. After this change, only the cofactor parameter can be NULL. It also
diff --git a/crypto/ec/ec_asn1.c b/crypto/ec/ec_asn1.c
index b0cd3e1788..a9b90787c5 100644
--- a/crypto/ec/ec_asn1.c
+++ b/crypto/ec/ec_asn1.c
@@ -695,10 +695,12 @@ ECPKPARAMETERS *ec_asn1_group2pkparameters(const EC_GROUP *group,
static EC_GROUP *ec_asn1_parameters2group(const ECPARAMETERS *params)
{
int ok = 0, tmp;
- EC_GROUP *ret = NULL;
+ EC_GROUP *ret = NULL, *dup = NULL;
BIGNUM *p = NULL, *a = NULL, *b = NULL;
EC_POINT *point = NULL;
long field_bits;
+ int curve_name = NID_undef;
+ BN_CTX *ctx = NULL;
if (!params->fieldID || !params->fieldID->fieldType ||
!params->fieldID->p.ptr) {
@@ -914,13 +916,75 @@ static EC_GROUP *ec_asn1_parameters2group(const ECPARAMETERS *params)
goto err;
}
+ /*
+ * Check if the explicit parameters group just created matches one of the
+ * built-in curves.
+ *
+ * We create a copy of the group just built, so that we can remove optional
+ * fields for the lookup: we do this to avoid the possibility that one of
+ * the optional parameters is used to force the library into using a less
+ * performant and less secure EC_METHOD instead of the specialized one.
+ * In any case, `seed` is not really used in any computation, while a
+ * cofactor different from the one in the built-in table is just
+ * mathematically wrong anyway and should not be used.
+ */
+ if ((ctx = BN_CTX_new()) == NULL) {
+ ECerr(EC_F_EC_ASN1_PARAMETERS2GROUP, ERR_R_BN_LIB);
+ goto err;
+ }
+ if ((dup = EC_GROUP_dup(ret)) == NULL
+ || EC_GROUP_set_seed(dup, NULL, 0) != 1
+ || !EC_GROUP_set_generator(dup, point, a, NULL)) {
+ ECerr(EC_F_EC_ASN1_PARAMETERS2GROUP, ERR_R_EC_LIB);
+ goto err;
+ }
+ if ((curve_name = ec_curve_nid_from_params(dup, ctx)) != NID_undef) {
+ /*
+ * The input explicit parameters successfully matched one of the
+ * built-in curves: often for built-in curves we have specialized
+ * methods with better performance and hardening.
+ *
+ * In this case we replace the `EC_GROUP` created through explicit
+ * parameters with one created from a named group.
+ */
+ EC_GROUP *named_group = NULL;
+
+#ifndef OPENSSL_NO_EC_NISTP_64_GCC_128
+ /*
+ * 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.
+ */
+ if (curve_name == NID_wap_wsg_idm_ecid_wtls12)
+ curve_name = NID_secp224r1;
+#endif /* !def(OPENSSL_NO_EC_NISTP_64_GCC_128) */
+
+ if ((named_group = EC_GROUP_new_by_curve_name(curve_name)) == NULL) {
+ ECerr(EC_F_EC_ASN1_PARAMETERS2GROUP, ERR_R_EC_LIB);
+ goto err;
+ }
+ EC_GROUP_free(ret);
+ ret = named_group;
+
+ /*
+ * Set the flag so that EC_GROUPs created from explicit parameters are
+ * serialized using explicit parameters by default.
+ *
+ * 0x0 = OPENSSL_EC_EXPLICIT_CURVE
+ */
+ EC_GROUP_set_asn1_flag(ret, 0x0);
+ }
+
ok = 1;
- err:if (!ok) {
+ err:
+ if (!ok) {
if (ret)
- EC_GROUP_clear_free(ret);
+ EC_GROUP_free(ret);
ret = NULL;
}
+ if (dup)
+ EC_GROUP_free(dup);
if (p)
BN_free(p);
@@ -930,6 +994,8 @@ static EC_GROUP *ec_asn1_parameters2group(const ECPARAMETERS *params)
BN_free(b);
if (point)
EC_POINT_free(point);
+ if (ctx)
+ BN_CTX_free(ctx);
return (ret);
}
@@ -990,7 +1056,7 @@ EC_GROUP *d2i_ECPKParameters(EC_GROUP **a, const unsigned char **in, long len)
}
if (a && *a)
- EC_GROUP_clear_free(*a);
+ EC_GROUP_free(*a);
if (a)
*a = group;
@@ -1040,7 +1106,7 @@ EC_KEY *d2i_ECPrivateKey(EC_KEY **a, const unsigned char **in, long len)
if (priv_key->parameters) {
if (ret->group)
- EC_GROUP_clear_free(ret->group);
+ EC_GROUP_free(ret->group);
ret->group = ec_asn1_pkparameters2group(priv_key->parameters);
}
diff --git a/crypto/ec/ec_curve.c b/crypto/ec/ec_curve.c
index 6dbe9d8258..9d4c71637b 100644
--- a/crypto/ec/ec_curve.c
+++ b/crypto/ec/ec_curve.c
@@ -75,6 +75,8 @@
#include <openssl/obj_mac.h>
#include <openssl/opensslconf.h>
+#include "bn_int.h"
+
#ifdef OPENSSL_FIPS
# include <openssl/fips.h>
#endif
@@ -3246,3 +3248,115 @@ int EC_curve_nist2nid(const char *name)
}
return NID_undef;
}
+
+#define NUM_BN_FIELDS 6
+/*
+ * Validates EC domain parameter data for known named curves.
+ * This can be used when a curve is loaded explicitly (without a curve
+ * name) or to validate that domain parameters have not been modified.
+ *
+ * Returns: The nid associated with the found named curve, or NID_undef
+ * if not found. If there was an error it returns -1.
+ */
+int ec_curve_nid_from_params(const EC_GROUP *group, BN_CTX *ctx)
+{
+ int ret = -1, nid, len, field_type, param_len;
+ size_t i, seed_len;
+ const unsigned char *seed, *params_seed, *params;
+ unsigned char *param_bytes = NULL;
+ const EC_CURVE_DATA *data;
+ const EC_POINT *generator = NULL;
+ const EC_METHOD *meth;
+ const BIGNUM *cofactor = NULL;
+ /* An array of BIGNUMs for (p, a, b, x, y, order) */
+ BIGNUM *bn[NUM_BN_FIELDS] = {NULL, NULL, NULL, NULL, NULL, NULL};
+
+ meth = EC_GROUP_method_of(group);
+ if (meth == NULL)
+ return -1;
+ /* Use the optional named curve nid as a search field */
+ nid = EC_GROUP_get_curve_name(group);
+ field_type = EC_METHOD_get_field_type(meth);
+ seed_len = EC_GROUP_get_seed_len(group);
+ seed = EC_GROUP_get0_seed(group);
+ cofactor = &group->cofactor;
+
+ BN_CTX_start(ctx);
+
+ /*
+ * The built-in curves contains data fields (p, a, b, x, y, order) that are
+ * all zero-padded to be the same size. The size of the padding is
+ * determined by either the number of bytes in the field modulus (p) or the
+ * EC group order, whichever is larger.
+ */
+ param_len = BN_num_bytes(&group->order);
+ len = BN_num_bytes(&group->field);
+ if (len > param_len)
+ param_len = len;
+
+ /* Allocate space to store the padded data for (p, a, b, x, y, order) */
+ param_bytes = OPENSSL_malloc(param_len * NUM_BN_FIELDS);
+ if (param_bytes == NULL)
+ goto end;
+
+ /* Create the bignums */
+ for (i = 0; i < NUM_BN_FIELDS; ++i) {
+ if ((bn[i] = BN_CTX_get(ctx)) == NULL)
+ goto end;
+ }
+ /*
+ * Fill in the bn array with the same values as the internal curves
+ * i.e. the values are p, a, b, x, y, order.
+ */
+ /* Get p, a & b */
+ if (!(ec_group_get_curve(group, bn[0], bn[1], bn[2], ctx)
+ && ((generator = EC_GROUP_get0_generator(group)) != NULL)
+ /* Get x & y */
+ && ec_point_get_affine_coordinates(group, generator, bn[3], bn[4], ctx)
+ /* Get order */
+ && EC_GROUP_get_order(group, bn[5], ctx)))
+ goto end;
+
+ /*
+ * Convert the bignum array to bytes that are joined together to form
+ * a single buffer that contains data for all fields.
+ * (p, a, b, x, y, order) are all zero padded to be the same size.
+ */
+ for (i = 0; i < NUM_BN_FIELDS; ++i) {
+ if (bn_bn2binpad(bn[i], ¶m_bytes[i*param_len], param_len) <= 0)
+ goto end;
+ }
+
+ for (i = 0; i < curve_list_length; i++) {
+ const ec_list_element curve = curve_list[i];
+
+ data = curve.data;
+ /* Get the raw order byte data */
+ params_seed = (const unsigned char *)(data + 1); /* skip header */
+ params = params_seed + data->seed_len;
+
+ /* Look for unique fields in the fixed curve data */
+ if (data->field_type == field_type
+ && param_len == data->param_len
+ && (nid <= 0 || nid == curve.nid)
+ /* check the optional cofactor (ignore if its zero) */
+ && (BN_is_zero(cofactor)
+ || BN_is_word(cofactor, (const BN_ULONG)curve.data->cofactor))
+ /* Check the optional seed (ignore if its not set) */
+ && (data->seed_len == 0 || seed_len == 0
+ || ((size_t)data->seed_len == seed_len
+ && memcmp(params_seed, seed, seed_len) == 0))
+ /* Check that the groups params match the built-in curve params */
+ && memcmp(param_bytes, params, param_len * NUM_BN_FIELDS)
+ == 0) {
+ ret = curve.nid;
+ goto end;
+ }
+ }
+ /* Gets here if the group was not found */
+ ret = NID_undef;
+end:
+ OPENSSL_free(param_bytes);
+ BN_CTX_end(ctx);
+ return ret;
+}
diff --git a/crypto/ec/ec_lcl.h b/crypto/ec/ec_lcl.h
index 8665a4c9c7..76deef170d 100644
--- a/crypto/ec/ec_lcl.h
+++ b/crypto/ec/ec_lcl.h
@@ -565,3 +565,18 @@ EC_GROUP *FIPS_ec_group_new_curve_gf2m(const BIGNUM *p, const BIGNUM *a,
const BIGNUM *b, BN_CTX *ctx);
EC_GROUP *FIPS_ec_group_new_by_curve_name(int nid);
#endif
+
+int ec_curve_nid_from_params(const EC_GROUP *group, BN_CTX *ctx);
+
+/*
+ * The next 2 functions are just internal wrappers around the omonimous
+ * functions with either the `_GFp` or the `_GF2m` suffix.
+ *
+ * They are meant to facilitate backporting of code from newer branches, where
+ * the public API includes a "field agnostic" version of these 2 functions.
+ */
+int ec_group_get_curve(const EC_GROUP *group, BIGNUM *p, BIGNUM *a,
+ BIGNUM *b, BN_CTX *ctx);
+int ec_point_get_affine_coordinates(const EC_GROUP *group,
+ const EC_POINT *point, BIGNUM *x,
+ BIGNUM *y, BN_CTX *ctx);
diff --git a/crypto/ec/ec_lib.c b/crypto/ec/ec_lib.c
index 15302322f7..e3f2e82f68 100644
--- a/crypto/ec/ec_lib.c
+++ b/crypto/ec/ec_lib.c
@@ -1257,3 +1257,60 @@ int ec_precompute_mont_data(EC_GROUP *group)
BN_CTX_free(ctx);
return ret;
}
+
+/*
+ * This is just a wrapper around the public functions
+ * - EC_GROUP_get_curve_GF2m
+ * - EC_GROUP_get_curve_GFp
+ *
+ * It is meant to facilitate backporting of code from newer branches, where
+ * the public API includes a "field agnostic" version of it.
+ */
+int ec_group_get_curve(const EC_GROUP *group, BIGNUM *p, BIGNUM *a,
+ BIGNUM *b, BN_CTX *ctx)
+{
+ int field_nid;
+
+ field_nid = EC_METHOD_get_field_type(EC_GROUP_method_of(group));
+
+#ifndef OPENSSL_NO_EC2M
+ if (field_nid == NID_X9_62_characteristic_two_field) {
+ return EC_GROUP_get_curve_GF2m(group, p, a, b, ctx);
+ } else
+#endif /* !def(OPENSSL_NO_EC2M) */
+ if (field_nid == NID_X9_62_prime_field) {
+ return EC_GROUP_get_curve_GFp(group, p, a, b, ctx);
+ } else {
+ /* this should never happen */
+ return 0;
+ }
+}
+
+/*
+ * This is just a wrapper around the public functions
+ * - EC_POINT_get_affine_coordinates_GF2m
+ * - EC_POINT_get_affine_coordinates_GFp
+ *
+ * It is meant to facilitate backporting of code from newer branches, where
+ * the public API includes a "field agnostic" version of it.
+ */
+int ec_point_get_affine_coordinates(const EC_GROUP *group,
+ const EC_POINT *point, BIGNUM *x,
+ BIGNUM *y, BN_CTX *ctx)
+{
+ int field_nid;
+
+ field_nid = EC_METHOD_get_field_type(EC_GROUP_method_of(group));
+
+#ifndef OPENSSL_NO_EC2M
+ if (field_nid == NID_X9_62_characteristic_two_field) {
+ return EC_POINT_get_affine_coordinates_GF2m(group, point, x, y, ctx);
+ } else
+#endif /* !def(OPENSSL_NO_EC2M) */
+ if (field_nid == NID_X9_62_prime_field) {
+ return EC_POINT_get_affine_coordinates_GFp(group, point, x, y, ctx);
+ } else {
+ /* this should never happen */
+ return 0;
+ }
+}
More information about the openssl-commits
mailing list