From cea83f9f7825309379db3fea77f19edf0c5b1e13 Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Fri, 3 Jun 2016 14:42:04 +0200 Subject: [PATCH] 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 Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/8750) --- crypto/ec/ec2_oct.c | 10 ++--- crypto/ec/ec_lib.c | 20 +++++++++- crypto/ec/ecp_oct.c | 10 ++--- crypto/ec/ectest.c | 96 ++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 116 insertions(+), 20 deletions(-) diff --git a/crypto/ec/ec2_oct.c b/crypto/ec/ec2_oct.c index 6f2f7ca514..b3e71c4d56 100644 --- 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 df56484b5e..c01e0f0722 100644 --- 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 1bc3f39ad1..941f0ecfb5 100644 --- 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 5e1ef50933..c3cdac1af0 100644 --- 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); } -- 2.25.1