RT 4242: reject invalid EC point coordinates

This is a backport of commit 1e2012b7 to 1.0.2. This hardening change
was made to 1.1.0 but was not backported to 1.0.2. Recent CVEs in user
applications have shown this additional hardening in 1.0.2 would be
beneficial.

E.g. see the patch for CVE-2019-9498
https://w1.fi/security/2019-4/0011-EAP-pwd-server-Verify-received-scalar-and-element.patch

and CVE-2019-9499
https://w1.fi/security/2019-4/0013-EAP-pwd-client-Verify-received-scalar-and-element.patch

The original commit had this description:

We already test in EC_POINT_oct2point that points are on the curve. To
be on the safe side, move this check to
EC_POINT_set_affine_coordinates_* so as to also check point coordinates
received through some other method.

We do not check projective coordinates, though, as
- it's unlikely that applications would be receiving this primarily
  internal representation from untrusted sources, and
- it's possible that the projective setters are used in a setting where
  performance matters.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8750)

diff --git a/crypto/ec/ec2_oct.c b/crypto/ec/ec2_oct.c
index 6f2f7ca514b8b5de63310bad2e9bc9940cfe23e2..b3e71c4d56fcfa95116b3299c6276ad55d838513 100644 (file)
--- a/crypto/ec/ec2_oct.c
+++ b/crypto/ec/ec2_oct.c
@@ -383,16 +383,14 @@ int ec_GF2m_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
             }
         }
 
+        /*
+         * EC_POINT_set_affine_coordinates_GF2m is responsible for checking that
+         * the point is on the curve.
+         */
         if (!EC_POINT_set_affine_coordinates_GF2m(group, point, x, y, ctx))
             goto err;
     }
 
-    /* test required by X9.62 */
-    if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
-        ECerr(EC_F_EC_GF2M_SIMPLE_OCT2POINT, EC_R_POINT_IS_NOT_ON_CURVE);
-        goto err;
-    }
-
     ret = 1;
 
  err:

diff --git a/crypto/ec/ec_lib.c b/crypto/ec/ec_lib.c
index df56484b5ee27a6450df0bbebd055f0f083ce67e..c01e0f072256d1c15b1ee95b5b375d1cae50278d 100644 (file)
--- a/crypto/ec/ec_lib.c
+++ b/crypto/ec/ec_lib.c
@@ -872,7 +872,15 @@ int EC_POINT_set_affine_coordinates_GFp(const EC_GROUP *group,
               EC_R_INCOMPATIBLE_OBJECTS);
         return 0;
     }
-    return group->meth->point_set_affine_coordinates(group, point, x, y, ctx);
+    if (!group->meth->point_set_affine_coordinates(group, point, x, y, ctx))
+        return 0;
+
+    if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
+        ECerr(EC_F_EC_POINT_SET_AFFINE_COORDINATES_GFP,
+              EC_R_POINT_IS_NOT_ON_CURVE);
+        return 0;
+    }
+    return 1;
 }
 
 #ifndef OPENSSL_NO_EC2M
@@ -890,7 +898,15 @@ int EC_POINT_set_affine_coordinates_GF2m(const EC_GROUP *group,
               EC_R_INCOMPATIBLE_OBJECTS);
         return 0;
     }
-    return group->meth->point_set_affine_coordinates(group, point, x, y, ctx);
+    if (!group->meth->point_set_affine_coordinates(group, point, x, y, ctx))
+        return 0;
+
+    if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
+        ECerr(EC_F_EC_POINT_SET_AFFINE_COORDINATES_GF2M,
+              EC_R_POINT_IS_NOT_ON_CURVE);
+        return 0;
+    }
+    return 1;
 }
 #endif
 

diff --git a/crypto/ec/ecp_oct.c b/crypto/ec/ecp_oct.c
index 1bc3f39ad15ff971045bd774e64afd99a722ca0a..941f0ecfb5b03320f93f4c5491561794d8dfca31 100644 (file)
--- a/crypto/ec/ecp_oct.c
+++ b/crypto/ec/ecp_oct.c
@@ -408,16 +408,14 @@ int ec_GFp_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
             }
         }
 
+        /*
+         * EC_POINT_set_affine_coordinates_GFp is responsible for checking that
+         * the point is on the curve.
+         */
         if (!EC_POINT_set_affine_coordinates_GFp(group, point, x, y, ctx))
             goto err;
     }
 
-    /* test required by X9.62 */
-    if (EC_POINT_is_on_curve(group, point, ctx) <= 0) {
-        ECerr(EC_F_EC_GFP_SIMPLE_OCT2POINT, EC_R_POINT_IS_NOT_ON_CURVE);
-        goto err;
-    }
-
     ret = 1;
 
  err:

diff --git a/crypto/ec/ectest.c b/crypto/ec/ectest.c
index 5e1ef50933839bd9503be94551cecc8ecbb712aa..c3cdac1af0318eafb848d0fd470bc9eee5583458 100644 (file)
--- a/crypto/ec/ectest.c
+++ b/crypto/ec/ectest.c
@@ -325,7 +325,7 @@ static void prime_field_tests(void)
     EC_GROUP *P_160 = NULL, *P_192 = NULL, *P_224 = NULL, *P_256 =
         NULL, *P_384 = NULL, *P_521 = NULL;
     EC_POINT *P, *Q, *R;
-    BIGNUM *x, *y, *z;
+    BIGNUM *x, *y, *z, *yplusone;
     unsigned char buf[100];
     size_t i, len;
     int k;
@@ -405,7 +405,8 @@ static void prime_field_tests(void)
     x = BN_new();
     y = BN_new();
     z = BN_new();
-    if (!x || !y || !z)
+    yplusone = BN_new();
+    if (x == NULL || y == NULL || z == NULL || yplusone == NULL)
         ABORT;
 
     if (!BN_hex2bn(&x, "D"))
@@ -542,6 +543,14 @@ static void prime_field_tests(void)
         ABORT;
     if (!BN_hex2bn(&y, "23a628553168947d59dcc912042351377ac5fb32"))
         ABORT;
+    if (!BN_add(yplusone, y, BN_value_one()))
+        ABORT;
+    /*
+     * When (x, y) is on the curve, (x, y + 1) is, as it happens, not,
+     * and therefore setting the coordinates should fail.
+     */
+    if (EC_POINT_set_affine_coordinates_GFp(group, P, x, yplusone, ctx))
+        ABORT;
     if (!EC_POINT_set_affine_coordinates_GFp(group, P, x, y, ctx))
         ABORT;
     if (EC_POINT_is_on_curve(group, P, ctx) <= 0)
@@ -613,6 +622,15 @@ static void prime_field_tests(void)
     if (0 != BN_cmp(y, z))
         ABORT;
 
+    if (!BN_add(yplusone, y, BN_value_one()))
+        ABORT;
+    /*
+     * When (x, y) is on the curve, (x, y + 1) is, as it happens, not,
+     * and therefore setting the coordinates should fail.
+     */
+    if (EC_POINT_set_affine_coordinates_GFp(group, P, x, yplusone, ctx))
+        ABORT;
+
     fprintf(stdout, "verify degree ...");
     if (EC_GROUP_get_degree(group) != 192)
         ABORT;
@@ -668,6 +686,15 @@ static void prime_field_tests(void)
     if (0 != BN_cmp(y, z))
         ABORT;
 
+    if (!BN_add(yplusone, y, BN_value_one()))
+        ABORT;
+    /*
+     * When (x, y) is on the curve, (x, y + 1) is, as it happens, not,
+     * and therefore setting the coordinates should fail.
+     */
+    if (EC_POINT_set_affine_coordinates_GFp(group, P, x, yplusone, ctx))
+        ABORT;
+
     fprintf(stdout, "verify degree ...");
     if (EC_GROUP_get_degree(group) != 224)
         ABORT;
@@ -728,6 +755,15 @@ static void prime_field_tests(void)
     if (0 != BN_cmp(y, z))
         ABORT;
 
+    if (!BN_add(yplusone, y, BN_value_one()))
+        ABORT;
+    /*
+     * When (x, y) is on the curve, (x, y + 1) is, as it happens, not,
+     * and therefore setting the coordinates should fail.
+     */
+    if (EC_POINT_set_affine_coordinates_GFp(group, P, x, yplusone, ctx))
+        ABORT;
+
     fprintf(stdout, "verify degree ...");
     if (EC_GROUP_get_degree(group) != 256)
         ABORT;
@@ -783,6 +819,15 @@ static void prime_field_tests(void)
     if (0 != BN_cmp(y, z))
         ABORT;
 
+    if (!BN_add(yplusone, y, BN_value_one()))
+        ABORT;
+    /*
+     * When (x, y) is on the curve, (x, y + 1) is, as it happens, not,
+     * and therefore setting the coordinates should fail.
+     */
+    if (EC_POINT_set_affine_coordinates_GFp(group, P, x, yplusone, ctx))
+        ABORT;
+
     fprintf(stdout, "verify degree ...");
     if (EC_GROUP_get_degree(group) != 384)
         ABORT;
@@ -844,6 +889,15 @@ static void prime_field_tests(void)
     if (0 != BN_cmp(y, z))
         ABORT;
 
+    if (!BN_add(yplusone, y, BN_value_one()))
+        ABORT;
+    /*
+     * When (x, y) is on the curve, (x, y + 1) is, as it happens, not,
+     * and therefore setting the coordinates should fail.
+     */
+    if (EC_POINT_set_affine_coordinates_GFp(group, P, x, yplusone, ctx))
+        ABORT;
+
     fprintf(stdout, "verify degree ...");
     if (EC_GROUP_get_degree(group) != 521)
         ABORT;
@@ -858,6 +912,10 @@ static void prime_field_tests(void)
 
     /* more tests using the last curve */
 
+    /* Restore the point that got mangled in the (x, y + 1) test. */
+    if (!EC_POINT_set_affine_coordinates_GFp(group, P, x, y, ctx))
+        ABORT;
+
     if (!EC_POINT_copy(Q, P))
         ABORT;
     if (EC_POINT_is_at_infinity(group, Q))
@@ -987,6 +1045,7 @@ static void prime_field_tests(void)
     BN_free(x);
     BN_free(y);
     BN_free(z);
+    BN_free(yplusone);
 
     if (P_160)
         EC_GROUP_free(P_160);
@@ -1007,6 +1066,13 @@ static void prime_field_tests(void)
 # ifdef OPENSSL_EC_BIN_PT_COMP
 #  define CHAR2_CURVE_TEST_INTERNAL(_name, _p, _a, _b, _x, _y, _y_bit, _order, _cof, _degree, _variable) \
         if (!BN_hex2bn(&x, _x)) ABORT; \
+        if (!BN_hex2bn(&y, _y)) ABORT; \
+        if (!BN_add(yplusone, y, BN_value_one())) ABORT;        \
+        /* \
+         * When (x, y) is on the curve, (x, y + 1) is, as it happens, not, \
+         * and therefore setting the coordinates should fail. \
+         */ \
+        if (EC_POINT_set_affine_coordinates_GF2m(group, P, x, yplusone, ctx)) ABORT; \
         if (!EC_POINT_set_compressed_coordinates_GF2m(group, P, x, _y_bit, ctx)) ABORT; \
         if (EC_POINT_is_on_curve(group, P, ctx) <= 0) ABORT; \
         if (!BN_hex2bn(&z, _order)) ABORT; \
@@ -1025,6 +1091,12 @@ static void prime_field_tests(void)
 #  define CHAR2_CURVE_TEST_INTERNAL(_name, _p, _a, _b, _x, _y, _y_bit, _order, _cof, _degree, _variable) \
         if (!BN_hex2bn(&x, _x)) ABORT; \
         if (!BN_hex2bn(&y, _y)) ABORT; \
+        if (!BN_add(yplusone, y, BN_value_one())) ABORT;        \
+        /* \
+         * When (x, y) is on the curve, (x, y + 1) is, as it happens, not, \
+         * and therefore setting the coordinates should fail. \
+         */ \
+        if (EC_POINT_set_affine_coordinates_GF2m(group, P, x, yplusone, ctx)) ABORT; \
         if (!EC_POINT_set_affine_coordinates_GF2m(group, P, x, y, ctx)) ABORT; \
         if (EC_POINT_is_on_curve(group, P, ctx) <= 0) ABORT; \
         if (!BN_hex2bn(&z, _order)) ABORT; \
@@ -1062,7 +1134,7 @@ static void char2_field_tests(void)
     EC_GROUP *C2_B163 = NULL, *C2_B233 = NULL, *C2_B283 = NULL, *C2_B409 =
         NULL, *C2_B571 = NULL;
     EC_POINT *P, *Q, *R;
-    BIGNUM *x, *y, *z, *cof;
+    BIGNUM *x, *y, *z, *cof, *yplusone;
     unsigned char buf[100];
     size_t i, len;
     int k;
@@ -1076,7 +1148,7 @@ static void char2_field_tests(void)
     p = BN_new();
     a = BN_new();
     b = BN_new();
-    if (!p || !a || !b)
+    if (p == NULL || a == NULL || b == NULL)
         ABORT;
 
     if (!BN_hex2bn(&p, "13"))
@@ -1142,7 +1214,8 @@ static void char2_field_tests(void)
     y = BN_new();
     z = BN_new();
     cof = BN_new();
-    if (!x || !y || !z || !cof)
+    yplusone = BN_new();
+    if (x == NULL || y == NULL || z == NULL || cof == NULL || yplusone == NULL)
         ABORT;
 
     if (!BN_hex2bn(&x, "6"))
@@ -1504,6 +1577,7 @@ static void char2_field_tests(void)
     BN_free(y);
     BN_free(z);
     BN_free(cof);
+    BN_free(yplusone);
 
     if (C2_K163)
         EC_GROUP_free(C2_K163);
@@ -1672,7 +1746,7 @@ static const struct nistp_test_params nistp_tests_params[] = {
 static void nistp_single_test(const struct nistp_test_params *test)
 {
     BN_CTX *ctx;
-    BIGNUM *p, *a, *b, *x, *y, *n, *m, *order;
+    BIGNUM *p, *a, *b, *x, *y, *n, *m, *order, *yplusone;
     EC_GROUP *NISTP;
     EC_POINT *G, *P, *Q, *Q_CHECK;
 
@@ -1687,6 +1761,7 @@ static void nistp_single_test(const struct nistp_test_params *test)
     m = BN_new();
     n = BN_new();
     order = BN_new();
+    yplusone = BN_new();
 
     NISTP = EC_GROUP_new(test->meth());
     if (!NISTP)
@@ -1709,6 +1784,14 @@ static void nistp_single_test(const struct nistp_test_params *test)
         ABORT;
     if (!BN_hex2bn(&y, test->Qy))
         ABORT;
+    if (!BN_add(yplusone, y, BN_value_one()))
+        ABORT;
+    /*
+     * When (x, y) is on the curve, (x, y + 1) is, as it happens, not,
+     * and therefore setting the coordinates should fail.
+     */
+    if (EC_POINT_set_affine_coordinates_GFp(NISTP, Q_CHECK, x, yplusone, ctx))
+        ABORT;
     if (!EC_POINT_set_affine_coordinates_GFp(NISTP, Q_CHECK, x, y, ctx))
         ABORT;
     if (!BN_hex2bn(&x, test->Gx))
@@ -1811,6 +1894,7 @@ static void nistp_single_test(const struct nistp_test_params *test)
     BN_free(x);
     BN_free(y);
     BN_free(order);
+    BN_free(yplusone);
     BN_CTX_free(ctx);
 }