From 0a2c9938a512bef20e990e6b9180c3eb2a7dffd7 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 24 Dec 2015 19:38:03 -0500 Subject: [PATCH] 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 --- crypto/evp/p_ec_asn1.c | 102 +++++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 49 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,6 +127,59 @@ err: return 0; } +static int eckey_pub_decode(EVP_PKEY *pkey, X509_PUBKEY *pubkey) { + const uint8_t *p = NULL; + void *pval; + int ptype, pklen; + EC_KEY *eckey = NULL; + X509_ALGOR *palg; + + if (!X509_PUBKEY_get0_param(NULL, &p, &pklen, &palg, pubkey)) { + return 0; + } + X509_ALGOR_get0(NULL, &ptype, &pval, palg); + + 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; + } + + /* We have parameters now set public key */ + if (!o2i_ECPublicKey(&eckey, &p, pklen)) { + OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR); + goto err; + } + + EVP_PKEY_assign_EC_KEY(pkey, eckey); + return 1; + +err: + if (eckey) { + EC_KEY_free(eckey); + } + return 0; +} + +static int eckey_pub_cmp(const EVP_PKEY *a, const EVP_PKEY *b) { + int r; + const EC_GROUP *group = EC_KEY_get0_group(b->pkey.ec); + const EC_POINT *pa = EC_KEY_get0_public_key(a->pkey.ec), + *pb = EC_KEY_get0_public_key(b->pkey.ec); + r = EC_POINT_cmp(group, pa, pb, NULL); + if (r == 0) { + return 1; + } else if (r == 1) { + return 0; + } else { + return -2; + } +} + static EC_KEY *eckey_type2param(int ptype, void *pval) { EC_KEY *eckey = NULL; @@ -163,55 +216,6 @@ err: return NULL; } -static int eckey_pub_decode(EVP_PKEY *pkey, X509_PUBKEY *pubkey) { - const uint8_t *p = NULL; - void *pval; - int ptype, pklen; - EC_KEY *eckey = NULL; - X509_ALGOR *palg; - - if (!X509_PUBKEY_get0_param(NULL, &p, &pklen, &palg, pubkey)) { - return 0; - } - X509_ALGOR_get0(NULL, &ptype, &pval, palg); - - eckey = eckey_type2param(ptype, pval); - if (!eckey) { - OPENSSL_PUT_ERROR(EVP, ERR_R_EC_LIB); - return 0; - } - - /* We have parameters now set public key */ - if (!o2i_ECPublicKey(&eckey, &p, pklen)) { - OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR); - goto err; - } - - EVP_PKEY_assign_EC_KEY(pkey, eckey); - return 1; - -err: - if (eckey) { - EC_KEY_free(eckey); - } - return 0; -} - -static int eckey_pub_cmp(const EVP_PKEY *a, const EVP_PKEY *b) { - int r; - const EC_GROUP *group = EC_KEY_get0_group(b->pkey.ec); - const EC_POINT *pa = EC_KEY_get0_public_key(a->pkey.ec), - *pb = EC_KEY_get0_public_key(b->pkey.ec); - r = EC_POINT_cmp(group, pa, pb, NULL); - if (r == 0) { - return 1; - } else if (r == 1) { - return 0; - } else { - return -2; - } -} - static int eckey_priv_decode(EVP_PKEY *pkey, PKCS8_PRIV_KEY_INFO *p8) { const uint8_t *p = NULL; void *pval;