From ac2b52c6ad0cd40482b1c5c1c4ec68eb16020ae8 Mon Sep 17 00:00:00 2001 From: Nicola Tuveri Date: Sun, 31 Mar 2019 18:46:53 +0300 Subject: [PATCH] Separate the lookup test This fixes the "verifying the alias" case. Actually, while working on it, I realized that conceptually we were testing the 2 different behaviours of `EC_GROUP_check_named_curve()` at the same time, and actually not in the proper way. I think it's fair to assume that overwriting the curve name for an existing group with `NID_undef` could lead to the unexpected behaviour we were observing and working around. Thus I decided to separate the lookup test in a dedicated simpler test that does what the documentation of `EC_GROUP_check_named_curve()` suggests: the lookup functionality is meant to find a name for a group generated with explicit parameters. In case an alternative alias is returned by the lookup instead of the expected nid, to avoid doing comparisons between `EC_GROUP`s with different `EC_METHOD`s, the workaround is to retrieve the `ECPARAMETERS` of the "alias group" and create a new explicit parameters group to use in `EC_GROUP_cmp()`. Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/8555) --- crypto/ec/ec_lib.c | 3 +- crypto/ec/ec_mult.c | 2 +- doc/man1/ecparam.pod | 6 +++ test/ectest.c | 117 +++++++++++++++++++++++++++++++------------ 4 files changed, 93 insertions(+), 35 deletions(-) diff --git a/crypto/ec/ec_lib.c b/crypto/ec/ec_lib.c index b2ae11d8ac..762cac4feb 100644 --- a/crypto/ec/ec_lib.c +++ b/crypto/ec/ec_lib.c @@ -536,6 +536,7 @@ int EC_GROUP_cmp(const EC_GROUP *a, const EC_GROUP *b, BN_CTX *ctx) if (r || BN_cmp(a1, b1) != 0 || BN_cmp(a2, b2) != 0 || BN_cmp(a3, b3) != 0) r = 1; + /* XXX EC_POINT_cmp() assumes that the methods are equal */ /* return 1 if the generators are different */ if (r || EC_POINT_cmp(a, EC_GROUP_get0_generator(a), EC_GROUP_get0_generator(b), ctx) != 0) @@ -543,7 +544,7 @@ int EC_GROUP_cmp(const EC_GROUP *a, const EC_GROUP *b, BN_CTX *ctx) if (!r) { const BIGNUM *ao, *bo, *ac, *bc; - /* compare the order's */ + /* compare the orders */ ao = EC_GROUP_get0_order(a); bo = EC_GROUP_get0_order(b); if (ao == NULL || bo == NULL) { diff --git a/crypto/ec/ec_mult.c b/crypto/ec/ec_mult.c index 76dc524ba9..755d64400a 100644 --- a/crypto/ec/ec_mult.c +++ b/crypto/ec/ec_mult.c @@ -156,7 +156,7 @@ int ec_scalar_mul_ladder(const EC_GROUP *group, EC_POINT *r, ECerr(EC_F_EC_SCALAR_MUL_LADDER, EC_R_UNKNOWN_ORDER); return 0; } - if (BN_is_zero(group->cofactor) || BN_is_zero(group->cofactor)) { + if (BN_is_zero(group->cofactor)) { ECerr(EC_F_EC_SCALAR_MUL_LADDER, EC_R_UNKNOWN_COFACTOR); return 0; } diff --git a/doc/man1/ecparam.pod b/doc/man1/ecparam.pod index ed39788bd7..bfed11f1bc 100644 --- a/doc/man1/ecparam.pod +++ b/doc/man1/ecparam.pod @@ -17,6 +17,7 @@ B [B<-text>] [B<-C>] [B<-check>] +[B<-check_named>] [B<-name arg>] [B<-list_curves>] [B<-conv_form arg>] @@ -79,6 +80,11 @@ be loaded by calling the get_ec_group_XXX() function. Validate the elliptic curve parameters. +=item B<-check_named> + +Validate the elliptic name curve parameters by checking if the curve parameters +match an inbuilt curves. + =item B<-name arg> Use the EC parameters with the specified 'short' name. Use B<-list_curves> diff --git a/test/ectest.c b/test/ectest.c index c8cb752301..355e912f3a 100644 --- a/test/ectest.c +++ b/test/ectest.c @@ -1557,10 +1557,15 @@ static const unsigned char p521_explicit[] = { 0xbb, 0x6f, 0xb7, 0x1e, 0x91, 0x38, 0x64, 0x09, 0x02, 0x01, 0x01, }; -static int check_named_curve(int id) +/* + * This test validates a named curve's group parameters using + * EC_GROUP_check_named_curve(). It also checks that modifying any of the + * group parameters results in the curve not being valid. + */ +static int check_named_curve_test(int id) { - int ret = 0, nid, field_nid, has_seed, rv = 0; - EC_GROUP *group = NULL, *gtest = NULL, *galias = NULL; + int ret = 0, nid, field_nid, has_seed; + EC_GROUP *group = NULL, *gtest = NULL; const EC_POINT *group_gen = NULL; EC_POINT *other_gen = NULL; BIGNUM *group_p = NULL, *group_a = NULL, *group_b = NULL; @@ -1622,35 +1627,21 @@ static int check_named_curve(int id) /* Passes because this is a valid curve */ if (!TEST_int_eq(EC_GROUP_check_named_curve(group, 0), nid) /* Only NIST curves pass */ - || !TEST_int_eq(EC_GROUP_check_named_curve(group, 1), + || !TEST_int_eq(EC_GROUP_check_named_curve(group, 1), EC_curve_nid2nist(nid) != NULL ? nid : NID_undef)) goto err; - /* - * Pass if the named curve is not known but the parameters are correct. - * It is possible to find the wrong alias if there is no curve name. - */ - EC_GROUP_set_curve_name(group, NID_undef); - if (!TEST_int_gt(rv = EC_GROUP_check_named_curve(group, 0), 0)) - goto err; -#if 0 - /* This code does not currently work since aliases are not supported - * currently. - */ - /* Found an alias */ - if (rv != nid) { - /* Fail if the returned nid is not an alias of the original group */ - if (!TEST_ptr(galias = EC_GROUP_new_by_curve_name(rv))) - goto err; - EC_GROUP_set_curve_name(galias, nid); - if (!TEST_int_eq(EC_GROUP_check_named_curve(galias, 0), nid)) - goto err; - } -#endif - /* Fail if the curve name doesnt match the parameters */ + + /* Fail if the curve name doesn't match the parameters */ EC_GROUP_set_curve_name(group, nid + 1); + ERR_set_mark(); if (!TEST_int_le(EC_GROUP_check_named_curve(group, 0), 0)) goto err; + ERR_pop_to_mark(); + + /* Restore curve name and ensure it's passing */ EC_GROUP_set_curve_name(group, nid); + if (!TEST_int_eq(EC_GROUP_check_named_curve(group, 0), nid)) + goto err; if (!TEST_int_eq(EC_GROUP_set_seed(group, invalid_seed, invalid_seed_len), invalid_seed_len)) @@ -1673,14 +1664,14 @@ static int check_named_curve(int id) } /* Pass if the seed is unknown (as it is optional) */ if (!TEST_int_eq(EC_GROUP_set_seed(group, NULL, 0), 1) - || !TEST_int_gt(EC_GROUP_check_named_curve(group, 0), 0)) + || !TEST_int_eq(EC_GROUP_check_named_curve(group, 0), nid)) goto err; /* Check that a duped group passes */ if (!TEST_int_eq(EC_GROUP_check_named_curve(gtest, 0), nid)) goto err; - /* check that changing any generator parameters fail */ + /* check that changing any generator parameter fails */ if (!TEST_true(EC_GROUP_set_generator(gtest, other_gen, group_order, group_cofactor)) || !TEST_int_eq(EC_GROUP_check_named_curve(gtest, 0), 0) @@ -1704,8 +1695,7 @@ static int check_named_curve(int id) || !TEST_int_eq(EC_GROUP_check_named_curve(gtest, 0), nid)) goto err; - - /*- + /* * check that changing any curve parameter fails * * Setting arbitrary p, a or b might fail for some EC_GROUPs @@ -1739,7 +1729,7 @@ static int check_named_curve(int id) } ERR_pop_to_mark(); - /* Check that restoring the curve parameters pass */ + /* Check that restoring the curve parameters passes */ if (!TEST_true(EC_GROUP_set_curve(gtest, group_p, group_a, group_b, NULL)) || !TEST_int_eq(EC_GROUP_check_named_curve(gtest, 0), nid)) goto err; @@ -1756,13 +1746,73 @@ err: BN_free(other_cofactor); BN_free(other_order); EC_POINT_free(other_gen); - EC_GROUP_free(galias); EC_GROUP_free(gtest); EC_GROUP_free(group); BN_CTX_free(bn_ctx); return ret; } +/* + * This checks the lookup capability of EC_GROUP_check_named_curve() + * when the given group was created with explicit parameters. + * + * It is possible to retrieve an alternative alias that does not match + * the original nid in this case. + */ +static int check_named_curve_lookup_test(int id) +{ + int ret = 0, nid, rv = 0; + EC_GROUP *g = NULL , *ga = NULL; + ECPARAMETERS *p = NULL, *pa = NULL; + BN_CTX *ctx = NULL; + + /* Do some setup */ + nid = curves[id].nid; + if (!TEST_ptr(ctx = BN_CTX_new()) + || !TEST_ptr(g = EC_GROUP_new_by_curve_name(nid)) + || !TEST_ptr(p = EC_GROUP_get_ecparameters(g, NULL))) + goto err; + + /* replace with group from explicit parameters */ + EC_GROUP_free(g); + if (!TEST_ptr(g = EC_GROUP_new_from_ecparameters(p))) + goto err; + + if (!TEST_int_gt(rv = EC_GROUP_check_named_curve(g, 0), 0)) + goto err; + if (rv != nid) { + /* + * Found an alias: + * fail if the returned nid is not an alias of the original group. + * + * The comparison here is done by comparing two explicit + * parameter EC_GROUPs with EC_GROUP_cmp(), to ensure the + * comparison happens with unnamed EC_GROUPs using the same + * EC_METHODs. + */ + if (!TEST_ptr(ga = EC_GROUP_new_by_curve_name(rv)) + || !TEST_ptr(pa = EC_GROUP_get_ecparameters(ga, NULL))) + goto err; + + /* replace with group from explicit parameters, then compare */ + EC_GROUP_free(ga); + if (!TEST_ptr(ga = EC_GROUP_new_from_ecparameters(pa)) + || !TEST_int_eq(EC_GROUP_cmp(g, ga, ctx), 0)) + goto err; + } + + ret = 1; + + err: + EC_GROUP_free(g); + EC_GROUP_free(ga); + ECPARAMETERS_free(p); + ECPARAMETERS_free(pa); + BN_CTX_free(ctx); + + return ret; +} + static int parameter_test(void) { EC_GROUP *group = NULL, *group2 = NULL; @@ -1828,7 +1878,8 @@ int setup_tests(void) ADD_ALL_TESTS(internal_curve_test, crv_len); ADD_ALL_TESTS(internal_curve_test_method, crv_len); ADD_TEST(group_field_test); - ADD_ALL_TESTS(check_named_curve, crv_len); + ADD_ALL_TESTS(check_named_curve_test, crv_len); + ADD_ALL_TESTS(check_named_curve_lookup_test, crv_len); #endif return 1; } -- 2.25.1