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 <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6314)

diff --git a/crypto/ec/ec_asn1.c b/crypto/ec/ec_asn1.c
index c7ed01effaa5e7eabab161a9fe80cd6272097585..2ce4d160f8dccbdf07eb09f8ab9a820e7bb67212 100644 (file)
--- 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 1c31cce8d6c17cc086d16e4ddcd5a62f5470e9da..73e8aa8732bdd14dbd46f584dcb89f0e121bddf5 100644 (file)
--- 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