Commit ac2b52c6 authored by Nicola Tuveri's avatar Nicola Tuveri
Browse files

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: default avatarMatt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8555)
parent 8d4f150f

crypto/ec/ec_lib.c

+2 −1
Original line number Diff line number Diff line
@@ -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) {

crypto/ec/ec_mult.c

+1 −1
Original line number Diff line number Diff line
@@ -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;

    }

doc/man1/ecparam.pod

+6 −0
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@ B<openssl ecparam> 
[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>

test/ectest.c

+84 −33
Original line number Diff line number Diff line
@@ -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;
@@ -1625,32 +1630,18 @@ static int check_named_curve(int id) 
        || !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;

}