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

diff --git a/crypto/ec/ec_lib.c b/crypto/ec/ec_lib.c
index b2ae11d8acdaafccd53f3153ce6b6e8971f241e1..762cac4febe646c438a5937a415d79492df6d4c6 100644 (file)
--- 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 76dc524ba98bb9a77a9dbe584ba48cd322248f60..755d64400a18269768c73ddb551978d82cf12c64 100644 (file)
--- 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 ed39788bd7d6d740dec77c254b1feb225ee57f21..bfed11f1bce36f193e9ca8f1492e1b58dbe6a224 100644 (file)
--- a/doc/man1/ecparam.pod
+++ b/doc/man1/ecparam.pod
@@ -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>

diff --git a/test/ectest.c b/test/ectest.c
index c8cb752301cb4a703f1fa64b902a004387d4f1e4..355e912f3a293fc8bcd138d0015ddab3d8409dd9 100644 (file)
--- 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;
 }