From bacaa618c26411d212015493d0eb82076a3e76a1 Mon Sep 17 00:00:00 2001 From: Nicola Tuveri Date: Sat, 7 Sep 2019 18:05:31 +0300 Subject: [PATCH] [ec] Match built-in curves on EC_GROUP_new_from_ecparameters MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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. 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. _______________________________________________________________________________ Reviewed-by: Matt Caswell Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/9808) --- CHANGES | 11 ++ crypto/ec/ec_asn1.c | 71 +++++++++++- test/ectest.c | 267 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 345 insertions(+), 4 deletions(-) diff --git a/CHANGES b/CHANGES index 9599c64545..3d2f94303a 100644 --- a/CHANGES +++ b/CHANGES @@ -9,6 +9,17 @@ Changes between 1.1.1 and 3.0.0 [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 3a8128b755..e0d02bb01a 100644 --- a/crypto/ec/ec_asn1.c +++ b/crypto/ec/ec_asn1.c @@ -571,10 +571,12 @@ ECPKPARAMETERS *EC_GROUP_get_ecpkparameters(const EC_GROUP *group, EC_GROUP *EC_GROUP_new_from_ecparameters(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) { @@ -792,18 +794,79 @@ EC_GROUP *EC_GROUP_new_from_ecparameters(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_GROUP_NEW_FROM_ECPARAMETERS, 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_GROUP_NEW_FROM_ECPARAMETERS, 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_GROUP_NEW_FROM_ECPARAMETERS, 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. + */ + EC_GROUP_set_asn1_flag(ret, OPENSSL_EC_EXPLICIT_CURVE); + } + ok = 1; err: if (!ok) { - EC_GROUP_clear_free(ret); + EC_GROUP_free(ret); ret = NULL; } + EC_GROUP_free(dup); BN_free(p); BN_free(a); BN_free(b); EC_POINT_free(point); + + BN_CTX_free(ctx); + return ret; } @@ -864,7 +927,7 @@ EC_GROUP *d2i_ECPKParameters(EC_GROUP **a, const unsigned char **in, long len) } if (a) { - EC_GROUP_clear_free(*a); + EC_GROUP_free(*a); *a = group; } @@ -912,7 +975,7 @@ EC_KEY *d2i_ECPrivateKey(EC_KEY **a, const unsigned char **in, long len) ret = *a; if (priv_key->parameters) { - EC_GROUP_clear_free(ret->group); + EC_GROUP_free(ret->group); ret->group = EC_GROUP_new_from_ecpkparameters(priv_key->parameters); } diff --git a/test/ectest.c b/test/ectest.c index b51a3b1207..e0081c8866 100644 --- a/test/ectest.c +++ b/test/ectest.c @@ -1812,6 +1812,272 @@ static int check_named_curve_lookup_test(int id) return ret; } +/* + * Sometime we cannot compare nids for equality, as the built-in curve table + * includes aliases with different names for the same curve. + * + * This function returns TRUE (1) if the checked nids are identical, or if they + * alias to the same curve. FALSE (0) otherwise. + */ +static ossl_inline +int are_ec_nids_compatible(int n1d, int n2d) +{ + int ret = 0; + switch (n1d) { +# ifndef OPENSSL_NO_EC2M + case NID_sect113r1: + case NID_wap_wsg_idm_ecid_wtls4: + ret = (n2d == NID_sect113r1 || n2d == NID_wap_wsg_idm_ecid_wtls4); + break; + case NID_sect163k1: + case NID_wap_wsg_idm_ecid_wtls3: + ret = (n2d == NID_sect163k1 || n2d == NID_wap_wsg_idm_ecid_wtls3); + break; + case NID_sect233k1: + case NID_wap_wsg_idm_ecid_wtls10: + ret = (n2d == NID_sect233k1 || n2d == NID_wap_wsg_idm_ecid_wtls10); + break; + case NID_sect233r1: + case NID_wap_wsg_idm_ecid_wtls11: + ret = (n2d == NID_sect233r1 || n2d == NID_wap_wsg_idm_ecid_wtls11); + break; + case NID_X9_62_c2pnb163v1: + case NID_wap_wsg_idm_ecid_wtls5: + ret = (n2d == NID_X9_62_c2pnb163v1 + || n2d == NID_wap_wsg_idm_ecid_wtls5); + break; +# endif /* OPENSSL_NO_EC2M */ + case NID_secp112r1: + case NID_wap_wsg_idm_ecid_wtls6: + ret = (n2d == NID_secp112r1 || n2d == NID_wap_wsg_idm_ecid_wtls6); + break; + case NID_secp160r2: + case NID_wap_wsg_idm_ecid_wtls7: + ret = (n2d == NID_secp160r2 || n2d == NID_wap_wsg_idm_ecid_wtls7); + break; +# ifdef OPENSSL_NO_EC_NISTP_64_GCC_128 + case NID_secp224r1: + case NID_wap_wsg_idm_ecid_wtls12: + ret = (n2d == NID_secp224r1 || n2d == NID_wap_wsg_idm_ecid_wtls12); + break; +# else + /* + * For SEC P-224 we want to ensure that the SECP nid is returned, as + * that is associated with a specialized method. + */ + case NID_wap_wsg_idm_ecid_wtls12: + ret = (n2d == NID_secp224r1); + break; +# endif /* def(OPENSSL_NO_EC_NISTP_64_GCC_128) */ + + default: + ret = (n1d == n2d); + } + return ret; +} + +/* + * This checks that EC_GROUP_bew_from_ecparameters() returns a "named" + * EC_GROUP for built-in curves. + * + * Note that it is possible to retrieve an alternative alias that does not match + * the original nid. + * + * Ensure that the OPENSSL_EC_EXPLICIT_CURVE ASN1 flag is set. + */ +static int check_named_curve_from_ecparameters(int id) +{ + int ret = 0, nid, tnid; + EC_GROUP *group = NULL, *tgroup = NULL, *tmpg = NULL; + const EC_POINT *group_gen = NULL; + EC_POINT *other_gen = NULL; + BIGNUM *group_cofactor = NULL, *other_cofactor = NULL; + BIGNUM *other_gen_x = NULL, *other_gen_y = NULL; + const BIGNUM *group_order = NULL; + BIGNUM *other_order = NULL; + BN_CTX *bn_ctx = NULL; + static const unsigned char invalid_seed[] = "THIS IS NOT A VALID SEED"; + static size_t invalid_seed_len = sizeof(invalid_seed); + ECPARAMETERS *params = NULL, *other_params = NULL; + EC_GROUP *g_ary[8] = {NULL}; + EC_GROUP **g_next = &g_ary[0]; + ECPARAMETERS *p_ary[8] = {NULL}; + ECPARAMETERS **p_next = &p_ary[0]; + + /* Do some setup */ + nid = curves[id].nid; + TEST_note("Curve %s", OBJ_nid2sn(nid)); + if (!TEST_ptr(bn_ctx = BN_CTX_new())) + return ret; + BN_CTX_start(bn_ctx); + + if (/* Allocations */ + !TEST_ptr(group_cofactor = BN_CTX_get(bn_ctx)) + || !TEST_ptr(other_gen_x = BN_CTX_get(bn_ctx)) + || !TEST_ptr(other_gen_y = BN_CTX_get(bn_ctx)) + || !TEST_ptr(other_order = BN_CTX_get(bn_ctx)) + || !TEST_ptr(other_cofactor = BN_CTX_get(bn_ctx)) + /* Generate reference group and params */ + || !TEST_ptr(group = EC_GROUP_new_by_curve_name(nid)) + || !TEST_ptr(params = EC_GROUP_get_ecparameters(group, NULL)) + || !TEST_ptr(group_gen = EC_GROUP_get0_generator(group)) + || !TEST_ptr(group_order = EC_GROUP_get0_order(group)) + || !TEST_true(EC_GROUP_get_cofactor(group, group_cofactor, NULL)) + /* compute `other_*` values */ + || !TEST_ptr(tmpg = EC_GROUP_dup(group)) + || !TEST_ptr(other_gen = EC_POINT_dup(group_gen, group)) + || !TEST_true(EC_POINT_add(group, other_gen, group_gen, group_gen, NULL)) + || !TEST_true(EC_POINT_get_affine_coordinates(group, other_gen, + other_gen_x, other_gen_y, bn_ctx)) + || !TEST_true(BN_copy(other_order, group_order)) + || !TEST_true(BN_add_word(other_order, 1)) + || !TEST_true(BN_copy(other_cofactor, group_cofactor)) + || !TEST_true(BN_add_word(other_cofactor, 1))) + goto err; + + EC_POINT_free(other_gen); + other_gen = NULL; + + if (!TEST_ptr(other_gen = EC_POINT_new(tmpg)) + || !TEST_true(EC_POINT_set_affine_coordinates(tmpg, other_gen, + other_gen_x, other_gen_y, + bn_ctx))) + goto err; + + /* + * ########################### + * # Actual tests start here # + * ########################### + */ + + /* + * Creating a group from built-in explicit parameters returns a + * "named" EC_GROUP + */ + if (!TEST_ptr(tgroup = *g_next++ = EC_GROUP_new_from_ecparameters(params)) + || !TEST_int_ne((tnid = EC_GROUP_get_curve_name(tgroup)), NID_undef)) + goto err; + /* + * We cannot always guarantee the names match, as the built-in table + * contains aliases for the same curve with different names. + */ + if (!TEST_true(are_ec_nids_compatible(nid, tnid))) { + TEST_info("nid = %s, tnid = %s", OBJ_nid2sn(nid), OBJ_nid2sn(tnid)); + goto err; + } + /* Ensure that the OPENSSL_EC_EXPLICIT_CURVE ASN1 flag is set. */ + if (!TEST_int_eq(EC_GROUP_get_asn1_flag(tgroup), OPENSSL_EC_EXPLICIT_CURVE)) + goto err; + + /* + * An invalid seed in the parameters should be ignored: expect a "named" + * group. + */ + if (!TEST_int_eq(EC_GROUP_set_seed(tmpg, invalid_seed, invalid_seed_len), + invalid_seed_len) + || !TEST_ptr(other_params = *p_next++ = + EC_GROUP_get_ecparameters(tmpg, NULL)) + || !TEST_ptr(tgroup = *g_next++ = + EC_GROUP_new_from_ecparameters(other_params)) + || !TEST_int_ne((tnid = EC_GROUP_get_curve_name(tgroup)), NID_undef) + || !TEST_true(are_ec_nids_compatible(nid, tnid)) + || !TEST_int_eq(EC_GROUP_get_asn1_flag(tgroup), + OPENSSL_EC_EXPLICIT_CURVE)) { + TEST_info("nid = %s, tnid = %s", OBJ_nid2sn(nid), OBJ_nid2sn(tnid)); + goto err; + } + + /* + * A null seed in the parameters should be ignored, as it is optional: + * expect a "named" group. + */ + if (!TEST_int_eq(EC_GROUP_set_seed(tmpg, NULL, 0), 1) + || !TEST_ptr(other_params = *p_next++ = + EC_GROUP_get_ecparameters(tmpg, NULL)) + || !TEST_ptr(tgroup = *g_next++ = + EC_GROUP_new_from_ecparameters(other_params)) + || !TEST_int_ne((tnid = EC_GROUP_get_curve_name(tgroup)), NID_undef) + || !TEST_true(are_ec_nids_compatible(nid, tnid)) + || !TEST_int_eq(EC_GROUP_get_asn1_flag(tgroup), + OPENSSL_EC_EXPLICIT_CURVE)) { + TEST_info("nid = %s, tnid = %s", OBJ_nid2sn(nid), OBJ_nid2sn(tnid)); + goto err; + } + + /* + * Check that changing any of the generator parameters does not yield a + * match with the built-in curves + */ + if (/* Other gen, same group order & cofactor */ + !TEST_true(EC_GROUP_set_generator(tmpg, other_gen, group_order, + group_cofactor)) + || !TEST_ptr(other_params = *p_next++ = + EC_GROUP_get_ecparameters(tmpg, NULL)) + || !TEST_ptr(tgroup = *g_next++ = + EC_GROUP_new_from_ecparameters(other_params)) + || !TEST_int_eq((tnid = EC_GROUP_get_curve_name(tgroup)), NID_undef) + /* Same gen & cofactor, different order */ + || !TEST_true(EC_GROUP_set_generator(tmpg, group_gen, other_order, + group_cofactor)) + || !TEST_ptr(other_params = *p_next++ = + EC_GROUP_get_ecparameters(tmpg, NULL)) + || !TEST_ptr(tgroup = *g_next++ = + EC_GROUP_new_from_ecparameters(other_params)) + || !TEST_int_eq((tnid = EC_GROUP_get_curve_name(tgroup)), NID_undef) + /* The order is not an optional field, so this should fail */ + || !TEST_false(EC_GROUP_set_generator(tmpg, group_gen, NULL, + group_cofactor)) + /* Check that a wrong cofactor is ignored, and we still match */ + || !TEST_true(EC_GROUP_set_generator(tmpg, group_gen, group_order, + other_cofactor)) + || !TEST_ptr(other_params = *p_next++ = + EC_GROUP_get_ecparameters(tmpg, NULL)) + || !TEST_ptr(tgroup = *g_next++ = + EC_GROUP_new_from_ecparameters(other_params)) + || !TEST_int_ne((tnid = EC_GROUP_get_curve_name(tgroup)), NID_undef) + || !TEST_true(are_ec_nids_compatible(nid, tnid)) + || !TEST_int_eq(EC_GROUP_get_asn1_flag(tgroup), + OPENSSL_EC_EXPLICIT_CURVE) + /* Check that if the cofactor is not set then it still matches */ + || !TEST_true(EC_GROUP_set_generator(tmpg, group_gen, group_order, + NULL)) + || !TEST_ptr(other_params = *p_next++ = + EC_GROUP_get_ecparameters(tmpg, NULL)) + || !TEST_ptr(tgroup = *g_next++ = + EC_GROUP_new_from_ecparameters(other_params)) + || !TEST_int_ne((tnid = EC_GROUP_get_curve_name(tgroup)), NID_undef) + || !TEST_true(are_ec_nids_compatible(nid, tnid)) + || !TEST_int_eq(EC_GROUP_get_asn1_flag(tgroup), + OPENSSL_EC_EXPLICIT_CURVE) + /* check that restoring the generator passes */ + || !TEST_true(EC_GROUP_set_generator(tmpg, group_gen, group_order, + group_cofactor)) + || !TEST_ptr(other_params = *p_next++ = + EC_GROUP_get_ecparameters(tmpg, NULL)) + || !TEST_ptr(tgroup = *g_next++ = + EC_GROUP_new_from_ecparameters(other_params)) + || !TEST_int_ne((tnid = EC_GROUP_get_curve_name(tgroup)), NID_undef) + || !TEST_true(are_ec_nids_compatible(nid, tnid)) + || !TEST_int_eq(EC_GROUP_get_asn1_flag(tgroup), + OPENSSL_EC_EXPLICIT_CURVE)) + goto err; + + ret = 1; +err: + for (g_next = &g_ary[0]; g_next < g_ary + OSSL_NELEM(g_ary); g_next++) + EC_GROUP_free(*g_next); + for (p_next = &p_ary[0]; p_next < p_ary + OSSL_NELEM(g_ary); p_next++) + ECPARAMETERS_free(*p_next); + ECPARAMETERS_free(params); + EC_POINT_free(other_gen); + EC_GROUP_free(tmpg); + EC_GROUP_free(group); + BN_CTX_end(bn_ctx); + BN_CTX_free(bn_ctx); + return ret; +} + + static int parameter_test(void) { EC_GROUP *group = NULL, *group2 = NULL; @@ -2019,6 +2285,7 @@ int setup_tests(void) ADD_ALL_TESTS(check_named_curve_test, crv_len); ADD_ALL_TESTS(check_named_curve_lookup_test, crv_len); ADD_ALL_TESTS(check_ec_key_field_public_range_test, crv_len); + ADD_ALL_TESTS(check_named_curve_from_ecparameters, crv_len); #endif /* OPENSSL_NO_EC */ return 1; } -- 2.40.0