From fc6f579a9e625caa0c8b93d9716ddbc558e21a29 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 20 May 2018 14:33:49 -0400 Subject: [PATCH] Fix explicit EC curve encoding. Per SEC 1, the curve coefficients must be padded up to size. See C.2's definition of Curve, C.1's definition of FieldElement, and 2.3.5's definition of how to encode the field elements in http://www.secg.org/sec1-v2.pdf. This comes up for P-521, where b needs a leading zero. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/6314) --- crypto/ec/ec_asn1.c | 67 +++++++++++++++---------------------- test/ectest.c | 81 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 102 insertions(+), 46 deletions(-) diff --git a/crypto/ec/ec_asn1.c b/crypto/ec/ec_asn1.c index c7ed01effa..2ce4d160f8 100644 --- a/crypto/ec/ec_asn1.c +++ b/crypto/ec/ec_asn1.c @@ -367,10 +367,8 @@ static int ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve) { int ok = 0, nid; BIGNUM *tmp_1 = NULL, *tmp_2 = NULL; - unsigned char *buffer_1 = NULL, *buffer_2 = NULL, - *a_buf = NULL, *b_buf = NULL; - size_t len_1, len_2; - unsigned char char_zero = 0; + unsigned char *a_buf = NULL, *b_buf = NULL; + size_t len; if (!group || !curve || !curve->a || !curve->b) return 0; @@ -398,44 +396,26 @@ static int ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve) } } #endif - len_1 = (size_t)BN_num_bytes(tmp_1); - len_2 = (size_t)BN_num_bytes(tmp_2); - - if (len_1 == 0) { - /* len_1 == 0 => a == 0 */ - a_buf = &char_zero; - len_1 = 1; - } else { - if ((buffer_1 = OPENSSL_malloc(len_1)) == NULL) { - ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_MALLOC_FAILURE); - goto err; - } - if ((len_1 = BN_bn2bin(tmp_1, buffer_1)) == 0) { - ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_BN_LIB); - goto err; - } - a_buf = buffer_1; + /* + * Per SEC 1, the curve coefficients must be padded up to size. See C.2's + * definition of Curve, C.1's definition of FieldElement, and 2.3.5's + * definition of how to encode the field elements. + */ + len = ((size_t)EC_GROUP_get_degree(group) + 7) / 8; + if ((a_buf = OPENSSL_malloc(len)) == NULL + || (b_buf = OPENSSL_malloc(len)) == NULL) { + ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_MALLOC_FAILURE); + goto err; } - - if (len_2 == 0) { - /* len_2 == 0 => b == 0 */ - b_buf = &char_zero; - len_2 = 1; - } else { - if ((buffer_2 = OPENSSL_malloc(len_2)) == NULL) { - ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_MALLOC_FAILURE); - goto err; - } - if ((len_2 = BN_bn2bin(tmp_2, buffer_2)) == 0) { - ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_BN_LIB); - goto err; - } - b_buf = buffer_2; + if (BN_bn2binpad(tmp_1, a_buf, len) < 0 + || BN_bn2binpad(tmp_2, b_buf, len) < 0) { + ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_BN_LIB); + goto err; } /* set a and b */ - if (!ASN1_OCTET_STRING_set(curve->a, a_buf, len_1) || - !ASN1_OCTET_STRING_set(curve->b, b_buf, len_2)) { + if (!ASN1_OCTET_STRING_set(curve->a, a_buf, len) + || !ASN1_OCTET_STRING_set(curve->b, b_buf, len)) { ECerr(EC_F_EC_ASN1_GROUP2CURVE, ERR_R_ASN1_LIB); goto err; } @@ -462,8 +442,8 @@ static int ec_asn1_group2curve(const EC_GROUP *group, X9_62_CURVE *curve) ok = 1; err: - OPENSSL_free(buffer_1); - OPENSSL_free(buffer_2); + OPENSSL_free(a_buf); + OPENSSL_free(b_buf); BN_free(tmp_1); BN_free(tmp_2); return ok; @@ -611,7 +591,12 @@ EC_GROUP *EC_GROUP_new_from_ecparameters(const ECPARAMETERS *params) goto err; } - /* now extract the curve parameters a and b */ + /* + * Now extract the curve parameters a and b. Note that, although SEC 1 + * specifies the length of their encodings, historical versions of OpenSSL + * encoded them incorrectly, so we must accept any length for backwards + * compatibility. + */ if (!params->curve || !params->curve->a || !params->curve->a->data || !params->curve->b || !params->curve->b->data) { diff --git a/test/ectest.c b/test/ectest.c index 1c31cce8d6..73e8aa8732 100644 --- a/test/ectest.c +++ b/test/ectest.c @@ -1407,20 +1407,91 @@ err: } # endif +static const unsigned char p521_named[] = { + 0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x23, +}; + +static const unsigned char p521_explicit[] = { + 0x30, 0x82, 0x01, 0xc3, 0x02, 0x01, 0x01, 0x30, 0x4d, 0x06, 0x07, 0x2a, + 0x86, 0x48, 0xce, 0x3d, 0x01, 0x01, 0x02, 0x42, 0x01, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0x30, 0x81, 0x9f, 0x04, 0x42, 0x01, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xfc, 0x04, 0x42, 0x00, 0x51, 0x95, 0x3e, 0xb9, 0x61, 0x8e, 0x1c, 0x9a, + 0x1f, 0x92, 0x9a, 0x21, 0xa0, 0xb6, 0x85, 0x40, 0xee, 0xa2, 0xda, 0x72, + 0x5b, 0x99, 0xb3, 0x15, 0xf3, 0xb8, 0xb4, 0x89, 0x91, 0x8e, 0xf1, 0x09, + 0xe1, 0x56, 0x19, 0x39, 0x51, 0xec, 0x7e, 0x93, 0x7b, 0x16, 0x52, 0xc0, + 0xbd, 0x3b, 0xb1, 0xbf, 0x07, 0x35, 0x73, 0xdf, 0x88, 0x3d, 0x2c, 0x34, + 0xf1, 0xef, 0x45, 0x1f, 0xd4, 0x6b, 0x50, 0x3f, 0x00, 0x03, 0x15, 0x00, + 0xd0, 0x9e, 0x88, 0x00, 0x29, 0x1c, 0xb8, 0x53, 0x96, 0xcc, 0x67, 0x17, + 0x39, 0x32, 0x84, 0xaa, 0xa0, 0xda, 0x64, 0xba, 0x04, 0x81, 0x85, 0x04, + 0x00, 0xc6, 0x85, 0x8e, 0x06, 0xb7, 0x04, 0x04, 0xe9, 0xcd, 0x9e, 0x3e, + 0xcb, 0x66, 0x23, 0x95, 0xb4, 0x42, 0x9c, 0x64, 0x81, 0x39, 0x05, 0x3f, + 0xb5, 0x21, 0xf8, 0x28, 0xaf, 0x60, 0x6b, 0x4d, 0x3d, 0xba, 0xa1, 0x4b, + 0x5e, 0x77, 0xef, 0xe7, 0x59, 0x28, 0xfe, 0x1d, 0xc1, 0x27, 0xa2, 0xff, + 0xa8, 0xde, 0x33, 0x48, 0xb3, 0xc1, 0x85, 0x6a, 0x42, 0x9b, 0xf9, 0x7e, + 0x7e, 0x31, 0xc2, 0xe5, 0xbd, 0x66, 0x01, 0x18, 0x39, 0x29, 0x6a, 0x78, + 0x9a, 0x3b, 0xc0, 0x04, 0x5c, 0x8a, 0x5f, 0xb4, 0x2c, 0x7d, 0x1b, 0xd9, + 0x98, 0xf5, 0x44, 0x49, 0x57, 0x9b, 0x44, 0x68, 0x17, 0xaf, 0xbd, 0x17, + 0x27, 0x3e, 0x66, 0x2c, 0x97, 0xee, 0x72, 0x99, 0x5e, 0xf4, 0x26, 0x40, + 0xc5, 0x50, 0xb9, 0x01, 0x3f, 0xad, 0x07, 0x61, 0x35, 0x3c, 0x70, 0x86, + 0xa2, 0x72, 0xc2, 0x40, 0x88, 0xbe, 0x94, 0x76, 0x9f, 0xd1, 0x66, 0x50, + 0x02, 0x42, 0x01, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xfa, + 0x51, 0x86, 0x87, 0x83, 0xbf, 0x2f, 0x96, 0x6b, 0x7f, 0xcc, 0x01, 0x48, + 0xf7, 0x09, 0xa5, 0xd0, 0x3b, 0xb5, 0xc9, 0xb8, 0x89, 0x9c, 0x47, 0xae, + 0xbb, 0x6f, 0xb7, 0x1e, 0x91, 0x38, 0x64, 0x09, 0x02, 0x01, 0x01, +}; + static int parameter_test(void) { EC_GROUP *group = NULL, *group2 = NULL; ECPARAMETERS *ecparameters = NULL; - int r; + unsigned char *buf = NULL; + int r = 0, len; + + if (!TEST_ptr(group = EC_GROUP_new_by_curve_name(NID_secp112r1)) + || !TEST_ptr(ecparameters = EC_GROUP_get_ecparameters(group, NULL)) + || !TEST_ptr(group2 = EC_GROUP_new_from_ecparameters(ecparameters)) + || !TEST_int_eq(EC_GROUP_cmp(group, group2, NULL), 0)) + goto err; + + EC_GROUP_free(group); + group = NULL; - r = TEST_ptr(group = EC_GROUP_new_by_curve_name(NID_secp112r1)) - && TEST_ptr(ecparameters = EC_GROUP_get_ecparameters(group, NULL)) - && TEST_ptr(group2 = EC_GROUP_new_from_ecparameters(ecparameters)) - && TEST_int_eq(EC_GROUP_cmp(group, group2, NULL), 0); + /* Test the named curve encoding, which should be default. */ + if (!TEST_ptr(group = EC_GROUP_new_by_curve_name(NID_secp521r1)) + || !TEST_true((len = i2d_ECPKParameters(group, &buf)) >= 0) + || !TEST_mem_eq(buf, len, p521_named, sizeof(p521_named))) + goto err; + + OPENSSL_free(buf); + buf = NULL; + + /* + * Test the explicit encoding. P-521 requires correctly zero-padding the + * curve coefficients. + */ + EC_GROUP_set_asn1_flag(group, OPENSSL_EC_EXPLICIT_CURVE); + if (!TEST_true((len = i2d_ECPKParameters(group, &buf)) >= 0) + || !TEST_mem_eq(buf, len, p521_explicit, sizeof(p521_explicit))) + goto err; + r = 1; +err: EC_GROUP_free(group); EC_GROUP_free(group2); ECPARAMETERS_free(ecparameters); + OPENSSL_free(buf); return r; } #endif -- 2.34.1