Don't allow the specifiedCurve form of ECParameters in SPKIs.

Although RFC 3279 allows both, per RFC 5912, keys must use a named curve
rather than spelling out the curve parameters. Although we do not allow
arbitrary curves, we do have to (pretty hackishly) recognize built-in
curves in ECPrivateKeys.

It seems the cause of this was that OpenSSL, unless you set asn1_flag on
the EC_GROUP, likes to encode keys by spelling out the parameters. This
is in violation of RFC 5915, though probably not in violation of one of
the other redundant ECC specifications. For more fun, it appears
asn1_flag defaults to *off* in the API and *on* in the command-line
tools.

I think the original cause was these defaults meant the pre-BoringSSL
Android/OpenSSL Chromium port wrote out Channel ID keys in this format.
By now this should no longer by an issue, but it'll warrant a bit more
investigation to be sure we can drop it.

For now, keep this logic out of SPKIs by not calling d2i_ECParameters.
d2i_ECParameters is a fairly pointless function when only named curves
are allowed. In testing other implementations, none of Firefox, Safari,
or IE11/Win will parse such certificates (i.e. the error is fatal and
unbypassable). Likewise, because Mac and Windows' underlying libraries
reject this, Chrome on Mac and Windows already rejects such things. Thus
this change should be compatible.

The following is the certificate and key I constructed to test with:

-----BEGIN CERTIFICATE-----
MIICwjCCAmqgAwIBAgIJANlMBNpJfb/rMAkGByqGSM49BAEwRTELMAkGA1UEBhMC
QVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdp
dHMgUHR5IEx0ZDAeFw0xNDA0MjMyMzIxNTdaFw0xNDA1MjMyMzIxNTdaMEUxCzAJ
BgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5l
dCBXaWRnaXRzIFB0eSBMdGQwggFLMIIBAwYHKoZIzj0CATCB9wIBATAsBgcqhkjO
PQEBAiEA/////wAAAAEAAAAAAAAAAAAAAAD///////////////8wWwQg/////wAA
AAEAAAAAAAAAAAAAAAD///////////////wEIFrGNdiqOpPns+u9VXaYhrxlHQaw
zFOw9jvOPD4n0mBLAxUAxJ02CIbnBJNqZnjhE50mt4GffpAEQQRrF9Hy4SxCR/i8
5uVjpEDydwN9gS3rM6D0oTlF2JjClk/jQuL+Gn+bjufrSnwPnhYrzjNXazFezsu2
QGg3v1H1AiEA/////wAAAAD//////////7zm+q2nF56E87nKwvxjJVECAQEDQgAE
5itp4r9ln5e+Lx4NlIpM1Zdrt6keDUb73ampHp3culoB59aXqAoY+cPEox5W4nyD
SNsWGhz1HX7xlC1Lz3IiwaNQME4wHQYDVR0OBBYEFKuE0qyrlfCCThZ4B1VXX+Qm
jYLRMB8GA1UdIwQYMBaAFKuE0qyrlfCCThZ4B1VXX+QmjYLRMAwGA1UdEwQFMAMB
Af8wCQYHKoZIzj0EAQNHADBEAiBATB6aVJxDD6YAxEM4vf6Sbg2Ty334ldXpkNwc
TF+SngIgZ/f59kgDLf6YA04iLw1fUv5Wf1nLYJWwgrRFON5+zvw=
-----END CERTIFICATE-----
-----BEGIN EC PARAMETERS-----
MIH3AgEBMCwGByqGSM49AQECIQD/////AAAAAQAAAAAAAAAAAAAAAP//////////
/////zBbBCD/////AAAAAQAAAAAAAAAAAAAAAP///////////////AQgWsY12Ko6
k+ez671VdpiGvGUdBrDMU7D2O848PifSYEsDFQDEnTYIhucEk2pmeOETnSa3gZ9+
kARBBGsX0fLhLEJH+Lzm5WOkQPJ3A32BLeszoPShOUXYmMKWT+NC4v4af5uO5+tK
fA+eFivOM1drMV7Oy7ZAaDe/UfUCIQD/////AAAAAP//////////vOb6racXnoTz
ucrC/GMlUQIBAQ==
-----END EC PARAMETERS-----
-----BEGIN EC PRIVATE KEY-----
MHcCAQEEIAcPCHJ61KBKnN1ZyU2JaHcItW/JXTB3DujRyc4Ki7RqoAoGCCqGSM49
AwEHoUQDQgAE5itp4r9ln5e+Lx4NlIpM1Zdrt6keDUb73ampHp3culoB59aXqAoY
+cPEox5W4nyDSNsWGhz1HX7xlC1Lz3IiwQ==
-----END EC PRIVATE KEY-----

BUG=522228

Change-Id: I3723411a633dc07c4640027de07500293f8f7913
Reviewed-on: https://boringssl-review.googlesource.com/6853
Reviewed-by: Adam Langley <alangley@gmail.com>

1 files changed, 42 insertions, 38 deletions

diff --git a/crypto/evp/p_ec_asn1.c b/crypto/evp/p_ec_asn1.c
index f40b9764..eeecc333 100644
--- a/crypto/evp/p_ec_asn1.c
+++ b/crypto/evp/p_ec_asn1.c
@@ -127,42 +127,6 @@ err:
   return 0;
 }
 
-static EC_KEY *eckey_type2param(int ptype, void *pval) {
-  EC_KEY *eckey = NULL;
-
-  if (ptype == V_ASN1_SEQUENCE) {
-    ASN1_STRING *pstr = pval;
-    const uint8_t *pm = pstr->data;
-    int pmlen = pstr->length;
-
-    eckey = d2i_ECParameters(NULL, &pm, pmlen);
-    if (eckey == NULL) {
-      OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
-      goto err;
-    }
-  } else if (ptype == V_ASN1_OBJECT) {
-    ASN1_OBJECT *poid = pval;
-
-    /* type == V_ASN1_OBJECT => the parameters are given
-     * by an asn1 OID */
-    eckey = EC_KEY_new_by_curve_name(OBJ_obj2nid(poid));
-    if (eckey == NULL) {
-      goto err;
-    }
-  } else {
-    OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
-    goto err;
-  }
-
-  return eckey;
-
-err:
-  if (eckey) {
-    EC_KEY_free(eckey);
-  }
-  return NULL;
-}
-
 static int eckey_pub_decode(EVP_PKEY *pkey, X509_PUBKEY *pubkey) {
   const uint8_t *p = NULL;
   void *pval;
@@ -175,8 +139,12 @@ static int eckey_pub_decode(EVP_PKEY *pkey, X509_PUBKEY *pubkey) {
   }
   X509_ALGOR_get0(NULL, &ptype, &pval, palg);
 
-  eckey = eckey_type2param(ptype, pval);
-  if (!eckey) {
+  if (ptype != V_ASN1_OBJECT) {
+    OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
+    return 0;
+  }
+  eckey = EC_KEY_new_by_curve_name(OBJ_obj2nid((ASN1_OBJECT *)pval));
+  if (eckey == NULL) {
     OPENSSL_PUT_ERROR(EVP, ERR_R_EC_LIB);
     return 0;
   }
@@ -212,6 +180,42 @@ static int eckey_pub_cmp(const EVP_PKEY *a, const EVP_PKEY *b) {
   }
 }
 
+static EC_KEY *eckey_type2param(int ptype, void *pval) {
+  EC_KEY *eckey = NULL;
+
+  if (ptype == V_ASN1_SEQUENCE) {
+    ASN1_STRING *pstr = pval;
+    const uint8_t *pm = pstr->data;
+    int pmlen = pstr->length;
+
+    eckey = d2i_ECParameters(NULL, &pm, pmlen);
+    if (eckey == NULL) {
+      OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
+      goto err;
+    }
+  } else if (ptype == V_ASN1_OBJECT) {
+    ASN1_OBJECT *poid = pval;
+
+    /* type == V_ASN1_OBJECT => the parameters are given
+     * by an asn1 OID */
+    eckey = EC_KEY_new_by_curve_name(OBJ_obj2nid(poid));
+    if (eckey == NULL) {
+      goto err;
+    }
+  } else {
+    OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
+    goto err;
+  }
+
+  return eckey;
+
+err:
+  if (eckey) {
+    EC_KEY_free(eckey);
+  }
+  return NULL;
+}
+
 static int eckey_priv_decode(EVP_PKEY *pkey, PKCS8_PRIV_KEY_INFO *p8) {
   const uint8_t *p = NULL;
   void *pval;