Commit cea83f9f authored by Emilia Kasper's avatar Emilia Kasper Committed by Matt Caswell
Browse files

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: default avatarPaul Dale <paul.dale@oracle.com>
Reviewed-by: default avatarMatt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8750)
parent f937540e

crypto/ec/ec2_oct.c

+4 −6
Original line number Diff line number Diff line
@@ -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:

crypto/ec/ec_lib.c

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

crypto/ec/ecp_oct.c

+4 −6
Original line number Diff line number Diff line
@@ -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:

crypto/ec/ectest.c

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

}