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; }