commit ec92ec471cd78b236613151d5cd8a43b260a286c
author David Benjamin <davidben@google.com> Sat Sep 14 21:43:04 2019 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Mon Sep 16 16:55:31 2019 +0000
tree bd75a85da43f2b5620808596b9f5e42b15454e8b
parent b82f945ebcc9252ea79d2df9e38ee45afae9535c

Parse explicit EC curves more strictly.

Wycheproof has a series of ECDH tests for whether we reject misspelled
explicit versions of named curves in public keys, including the wrong
cofactor. We pass those tests easily because we reject those in public
keys altogether, consistent with RFC 5480.

However, we do parse explicit curves for private keys, for compatibility
with keys produced by older OpenSSLs with unfortunate defaults. Were
that parser enabled for public keys too, we would trip some of these
Wycheproof tests because we ignore the cofactor.

Tighten the parser up. If the cofactor is not one, ignore the curve.
Also syntax-check the seed, even though we ignore it.

Change-Id: I39936e027a72d2dc5532beb2407575ad8042d4c9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37484
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/ec_extra/ec_asn1.c

diff --git a/crypto/ec_extra/ec_asn1.c b/crypto/ec_extra/ec_asn1.c
index 31988f3..9769d01 100644
--- a/crypto/ec_extra/ec_asn1.c
+++ b/crypto/ec_extra/ec_asn1.c
@@ -264,7 +264,8 @@
                                       CBS *out_base_y, CBS *out_order) {
   // See RFC 3279, section 2.3.5. Note that RFC 3279 calls this structure an
   // ECParameters while RFC 5480 calls it a SpecifiedECDomain.
-  CBS params, field_id, field_type, curve, base;
+  CBS params, field_id, field_type, curve, base, cofactor;
+  int has_cofactor;
   uint64_t version;
   if (!CBS_get_asn1(in, &params, CBS_ASN1_SEQUENCE) ||
       !CBS_get_asn1_uint64(&params, &version) ||
@@ -272,7 +273,8 @@
       !CBS_get_asn1(&params, &field_id, CBS_ASN1_SEQUENCE) ||
       !CBS_get_asn1(&field_id, &field_type, CBS_ASN1_OBJECT) ||
       CBS_len(&field_type) != sizeof(kPrimeField) ||
-      OPENSSL_memcmp(CBS_data(&field_type), kPrimeField, sizeof(kPrimeField)) != 0 ||
+      OPENSSL_memcmp(CBS_data(&field_type), kPrimeField, sizeof(kPrimeField)) !=
+          0 ||
       !CBS_get_asn1(&field_id, out_prime, CBS_ASN1_INTEGER) ||
       !is_unsigned_integer(out_prime) ||
       CBS_len(&field_id) != 0 ||
@@ -280,16 +282,26 @@
       !CBS_get_asn1(&curve, out_a, CBS_ASN1_OCTETSTRING) ||
       !CBS_get_asn1(&curve, out_b, CBS_ASN1_OCTETSTRING) ||
       // |curve| has an optional BIT STRING seed which we ignore.
+      !CBS_get_optional_asn1(&curve, NULL, NULL, CBS_ASN1_BITSTRING) ||
+      CBS_len(&curve) != 0 ||
       !CBS_get_asn1(&params, &base, CBS_ASN1_OCTETSTRING) ||
       !CBS_get_asn1(&params, out_order, CBS_ASN1_INTEGER) ||
-      !is_unsigned_integer(out_order)) {
+      !is_unsigned_integer(out_order) ||
+      !CBS_get_optional_asn1(&params, &cofactor, &has_cofactor,
+                             CBS_ASN1_INTEGER) ||
+      CBS_len(&params) != 0) {
     OPENSSL_PUT_ERROR(EC, EC_R_DECODE_ERROR);
     return 0;
   }
 
-  // |params| has an optional cofactor which we ignore. With the optional seed
-  // in |curve|, a group already has arbitrarily many encodings. Parse enough to
-  // uniquely determine the curve.
+  if (has_cofactor) {
+    // We only support prime-order curves so the cofactor must be one.
+    if (CBS_len(&cofactor) != 1 ||
+        CBS_data(&cofactor)[0] != 1) {
+      OPENSSL_PUT_ERROR(EC, EC_R_UNKNOWN_GROUP);
+      return 0;
+    }
+  }
 
   // Require that the base point use uncompressed form.
   uint8_t form;

crypto/evp/evp_tests.txt

diff --git a/crypto/evp/evp_tests.txt b/crypto/evp/evp_tests.txt
index 2ac51d3..0c890fd 100644
--- a/crypto/evp/evp_tests.txt
+++ b/crypto/evp/evp_tests.txt
@@ -73,6 +73,32 @@
 Input = 308190020100301306072a8648ce3d020106082a8648ce3d0301070476307402010104208a872fb62893c4d1ffc5b9f0f91758069f8352e08fa05a49f8db926cb5728725a00706052b81040022a144034200042c150f429ce70f216c252cf5e062ce1f639cd5d165c7f89424072c27197d78b33b920e95cdb664e990dcf0cfea0d94e2a8e6af9d0e58056e653104925b9fe6c9
 Error = GROUP_MISMATCH
 
+# The same key, but with the curve spelled explicitly.
+PrivateKey = P-256-ExplicitParameters
+Type = EC
+Input = 308201610201003081ec06072a8648ce3d02013081e0020101302c06072a8648ce3d0101022100ffffffff00000001000000000000000000000000ffffffffffffffffffffffff30440420ffffffff00000001000000000000000000000000fffffffffffffffffffffffc04205ac635d8aa3a93e7b3ebbd55769886bc651d06b0cc53b0f63bce3c3e27d2604b0441046b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c2964fe342e2fe1a7f9b8ee7eb4a7c0f9e162bce33576b315ececbb6406837bf51f5022100ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551020101046d306b02010104208a872fb62893c4d1ffc5b9f0f91758069f8352e08fa05a49f8db926cb5728725a144034200042c150f429ce70f216c252cf5e062ce1f639cd5d165c7f89424072c27197d78b33b920e95cdb664e990dcf0cfea0d94e2a8e6af9d0e58056e653104925b9fe6c9
+Output = 308187020100301306072a8648ce3d020106082a8648ce3d030107046d306b02010104208a872fb62893c4d1ffc5b9f0f91758069f8352e08fa05a49f8db926cb5728725a144034200042c150f429ce70f216c252cf5e062ce1f639cd5d165c7f89424072c27197d78b33b920e95cdb664e990dcf0cfea0d94e2a8e6af9d0e58056e653104925b9fe6c9
+ExpectNoRawPrivate
+ExpectNoRawPublic
+
+# The same as above, but with the optional cofactor omitted.
+PrivateKey = P-256-ExplicitParameters-NoCofactor
+Type = EC
+Input = 3082015e0201003081e906072a8648ce3d02013081dd020101302c06072a8648ce3d0101022100ffffffff00000001000000000000000000000000ffffffffffffffffffffffff30440420ffffffff00000001000000000000000000000000fffffffffffffffffffffffc04205ac635d8aa3a93e7b3ebbd55769886bc651d06b0cc53b0f63bce3c3e27d2604b0441046b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c2964fe342e2fe1a7f9b8ee7eb4a7c0f9e162bce33576b315ececbb6406837bf51f5022100ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551046d306b02010104208a872fb62893c4d1ffc5b9f0f91758069f8352e08fa05a49f8db926cb5728725a144034200042c150f429ce70f216c252cf5e062ce1f639cd5d165c7f89424072c27197d78b33b920e95cdb664e990dcf0cfea0d94e2a8e6af9d0e58056e653104925b9fe6c9
+Output = 308187020100301306072a8648ce3d020106082a8648ce3d030107046d306b02010104208a872fb62893c4d1ffc5b9f0f91758069f8352e08fa05a49f8db926cb5728725a144034200042c150f429ce70f216c252cf5e062ce1f639cd5d165c7f89424072c27197d78b33b920e95cdb664e990dcf0cfea0d94e2a8e6af9d0e58056e653104925b9fe6c9
+ExpectNoRawPrivate
+ExpectNoRawPublic
+
+# The same as above, but the cofactor is zero instead of one.
+PrivateKey = P-256-ExplicitParameters-CofactorZero
+Input = 308201610201003081ec06072a8648ce3d02013081e0020101302c06072a8648ce3d0101022100ffffffff00000001000000000000000000000000ffffffffffffffffffffffff30440420ffffffff00000001000000000000000000000000fffffffffffffffffffffffc04205ac635d8aa3a93e7b3ebbd55769886bc651d06b0cc53b0f63bce3c3e27d2604b0441046b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c2964fe342e2fe1a7f9b8ee7eb4a7c0f9e162bce33576b315ececbb6406837bf51f5022100ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551020100046d306b02010104208a872fb62893c4d1ffc5b9f0f91758069f8352e08fa05a49f8db926cb5728725a144034200042c150f429ce70f216c252cf5e062ce1f639cd5d165c7f89424072c27197d78b33b920e95cdb664e990dcf0cfea0d94e2a8e6af9d0e58056e653104925b9fe6c9
+Error = UNKNOWN_GROUP
+
+# The same as above, but the cofactor is two instead of one.
+PrivateKey = P-256-ExplicitParameters-CofactorTwo
+Input = 308201610201003081ec06072a8648ce3d02013081e0020101302c06072a8648ce3d0101022100ffffffff00000001000000000000000000000000ffffffffffffffffffffffff30440420ffffffff00000001000000000000000000000000fffffffffffffffffffffffc04205ac635d8aa3a93e7b3ebbd55769886bc651d06b0cc53b0f63bce3c3e27d2604b0441046b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c2964fe342e2fe1a7f9b8ee7eb4a7c0f9e162bce33576b315ececbb6406837bf51f5022100ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551020102046d306b02010104208a872fb62893c4d1ffc5b9f0f91758069f8352e08fa05a49f8db926cb5728725a144034200042c150f429ce70f216c252cf5e062ce1f639cd5d165c7f89424072c27197d78b33b920e95cdb664e990dcf0cfea0d94e2a8e6af9d0e58056e653104925b9fe6c9
+Error = UNKNOWN_GROUP
+
 # The public half of the same key encoded as a PublicKey.
 PublicKey = P-256-SPKI
 Type = EC