Commit 1e2012b7 authored by Emilia Kasper's avatar Emilia Kasper
Browse files

RT 4242: reject invalid EC point coordinates 



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 avatarRich Salz <rsalz@openssl.org>
parent 6670d55a

crypto/ec/ec2_oct.c

+4 −6
Original line number Diff line number Diff line
@@ -334,16 +334,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
@@ -700,7 +700,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
@@ -718,7 +726,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
@@ -355,16 +355,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:

test/ectest.c

+90 −6
Original line number Diff line number Diff line
@@ -201,7 +201,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;
@@ -279,7 +279,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"))
@@ -404,6 +405,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)
@@ -475,6 +484,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;
@@ -530,6 +548,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;
@@ -590,6 +617,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;
@@ -645,6 +681,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;
@@ -706,6 +751,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;
@@ -720,6 +774,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))
@@ -829,6 +887,7 @@ static void prime_field_tests(void) 
    BN_free(x);

    BN_free(y);

    BN_free(z);

    BN_free(yplusone);



    EC_GROUP_free(P_160);

    EC_GROUP_free(P_192);
@@ -843,6 +902,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; \
@@ -861,6 +927,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; \
@@ -898,7 +970,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;
@@ -910,7 +982,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"))
@@ -976,7 +1048,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"))
@@ -1304,6 +1377,7 @@ static void char2_field_tests(void) 
    BN_free(y);

    BN_free(z);

    BN_free(cof);

    BN_free(yplusone);



    EC_GROUP_free(C2_K163);

    EC_GROUP_free(C2_B163);
@@ -1480,7 +1554,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;
@@ -1495,6 +1569,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)
@@ -1517,6 +1592,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))
@@ -1615,6 +1698,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);

}