From ca9a538aa0f2ebdd261783efa032e69a2ea17fbc Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Thu, 8 Jan 2015 12:26:55 -0800 Subject: [PATCH] Fix various certificate fingerprint issues. By using non-DER or invalid encodings outside the signed portion of a certificate the fingerprint can be changed without breaking the signature. Although no details of the signed portion of the certificate can be changed this can cause problems with some applications: e.g. those using the certificate fingerprint for blacklists. 1. Reject signatures with non zero unused bits. If the BIT STRING containing the signature has non zero unused bits reject the signature. All current signature algorithms require zero unused bits. 2. Check certificate algorithm consistency. Check the AlgorithmIdentifier inside TBS matches the one in the certificate signature. NB: this will result in signature failure errors for some broken certificates. 3. Check DSA/ECDSA signatures use DER. Reencode DSA/ECDSA signatures and compare with the original received signature. Return an error if there is a mismatch. This will reject various cases including garbage after signature (thanks to Antti Karjalainen and Tuomo Untinen from the Codenomicon CROSS program for discovering this case) and use of BER or invalid ASN.1 INTEGERs (negative or with leading zeroes). CVE-2014-8275 (Imported from upstream's 85cfc188c06bd046420ae70dd6e302f9efe022a9 and 4c52816d35681c0533c25fdd3abb4b7c6962302d) Change-Id: Ic901aea8ea6457df27dc542a11c30464561e322b Reviewed-on: https://boringssl-review.googlesource.com/2783 Reviewed-by: David Benjamin Reviewed-by: Adam Langley --- crypto/asn1/a_type.c | 2 +- crypto/dsa/dsa.c | 13 ++++++++++++- crypto/ecdsa/ecdsa.c | 15 ++++++++++++++- crypto/x509/a_verify.c | 6 ++++++ crypto/x509/x509_error.c | 1 + crypto/x509/x_algor.c | 12 ++++++++++++ crypto/x509/x_all.c | 2 ++ include/openssl/asn1.h | 2 +- include/openssl/x509.h | 5 ++--- 9 files changed, 51 insertions(+), 7 deletions(-) diff --git a/crypto/asn1/a_type.c b/crypto/asn1/a_type.c index 62bffdebb..75a17d5c8 100644 --- a/crypto/asn1/a_type.c +++ b/crypto/asn1/a_type.c @@ -111,7 +111,7 @@ int ASN1_TYPE_set1(ASN1_TYPE *a, int type, const void *value) } /* Returns 0 if they are equal, != 0 otherwise. */ -int ASN1_TYPE_cmp(ASN1_TYPE *a, ASN1_TYPE *b) +int ASN1_TYPE_cmp(const ASN1_TYPE *a, const ASN1_TYPE *b) { int result = -1; diff --git a/crypto/dsa/dsa.c b/crypto/dsa/dsa.c index 25d15c054..47b902ace 100644 --- a/crypto/dsa/dsa.c +++ b/crypto/dsa/dsa.c @@ -243,13 +243,21 @@ int DSA_verify(int type, const uint8_t *digest, size_t digest_len, const uint8_t *sig, size_t sig_len, const DSA *dsa) { DSA_SIG *s = NULL; int ret = -1, valid; + uint8_t *der = NULL; s = DSA_SIG_new(); if (s == NULL) { goto err; } - if (d2i_DSA_SIG(&s, &sig, sig_len) == NULL) { + const uint8_t *sigp = sig; + if (d2i_DSA_SIG(&s, &sigp, sig_len) == NULL || sigp != sig + sig_len) { + goto err; + } + + /* Ensure that the signature uses DER and doesn't have trailing garbage. */ + int der_len = i2d_DSA_SIG(s, &der); + if (der_len < 0 || (size_t)der_len != sig_len || memcmp(sig, der, sig_len)) { goto err; } @@ -260,6 +268,9 @@ int DSA_verify(int type, const uint8_t *digest, size_t digest_len, ret = valid; err: + if (der != NULL) { + OPENSSL_free(der); + } if (s) { DSA_SIG_free(s); } diff --git a/crypto/ecdsa/ecdsa.c b/crypto/ecdsa/ecdsa.c index ddc3e6173..644246257 100644 --- a/crypto/ecdsa/ecdsa.c +++ b/crypto/ecdsa/ecdsa.c @@ -74,18 +74,31 @@ int ECDSA_verify(int type, const uint8_t *digest, size_t digest_len, const uint8_t *sig, size_t sig_len, EC_KEY *eckey) { ECDSA_SIG *s; int ret = 0; + uint8_t *der = NULL; if (eckey->ecdsa_meth && eckey->ecdsa_meth->verify) { return eckey->ecdsa_meth->verify(digest, digest_len, sig, sig_len, eckey); } s = ECDSA_SIG_new(); - if (s == NULL || d2i_ECDSA_SIG(&s, &sig, sig_len) == NULL) { + const uint8_t *sigp = sig; + if (s == NULL || d2i_ECDSA_SIG(&s, &sigp, sig_len) == NULL || + sigp != sig + sig_len) { goto err; } + + /* Ensure that the signature uses DER and doesn't have trailing garbage. */ + const int der_len = i2d_ECDSA_SIG(s, &der); + if (der_len < 0 || (size_t) der_len != sig_len || memcmp(sig, der, sig_len)) { + goto err; + } + ret = ECDSA_do_verify(digest, digest_len, s, eckey); err: + if (der != NULL) { + OPENSSL_free(der); + } if (s != NULL) { ECDSA_SIG_free(s); } diff --git a/crypto/x509/a_verify.c b/crypto/x509/a_verify.c index e728863fe..72e0a627c 100644 --- a/crypto/x509/a_verify.c +++ b/crypto/x509/a_verify.c @@ -84,6 +84,12 @@ int ASN1_item_verify(const ASN1_ITEM *it, X509_ALGOR *a, return 0; } + if (signature->type == V_ASN1_BIT_STRING && signature->flags & 0x7) + { + OPENSSL_PUT_ERROR(X509, ASN1_item_verify, X509_R_INVALID_BIT_STRING_BITS_LEFT); + return 0; + } + EVP_MD_CTX_init(&ctx); if (!EVP_DigestVerifyInitFromAlgorithm(&ctx, a, pkey)) diff --git a/crypto/x509/x509_error.c b/crypto/x509/x509_error.c index d521281a0..222a40a3d 100644 --- a/crypto/x509/x509_error.c +++ b/crypto/x509/x509_error.c @@ -94,6 +94,7 @@ const ERR_STRING_DATA X509_error_string_data[] = { {ERR_PACK(ERR_LIB_X509, 0, X509_R_CRL_VERIFY_FAILURE), "CRL_VERIFY_FAILURE"}, {ERR_PACK(ERR_LIB_X509, 0, X509_R_ERR_ASN1_LIB), "ERR_ASN1_LIB"}, {ERR_PACK(ERR_LIB_X509, 0, X509_R_IDP_MISMATCH), "IDP_MISMATCH"}, + {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_BIT_STRING_BITS_LEFT), "INVALID_BIT_STRING_BITS_LEFT"}, {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_DIRECTORY), "INVALID_DIRECTORY"}, {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_FIELD_NAME), "INVALID_FIELD_NAME"}, {ERR_PACK(ERR_LIB_X509, 0, X509_R_INVALID_TRUST), "INVALID_TRUST"}, diff --git a/crypto/x509/x_algor.c b/crypto/x509/x_algor.c index 238b0f34a..ae694e3d0 100644 --- a/crypto/x509/x_algor.c +++ b/crypto/x509/x_algor.c @@ -140,3 +140,15 @@ void X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md) X509_ALGOR_set0(alg, OBJ_nid2obj(EVP_MD_type(md)), param_type, NULL); } + +/* X509_ALGOR_cmp returns 0 if |a| and |b| are equal and non-zero otherwise. */ +int X509_ALGOR_cmp(const X509_ALGOR *a, const X509_ALGOR *b) + { + int rv; + rv = OBJ_cmp(a->algorithm, b->algorithm); + if (rv) + return rv; + if (!a->parameter && !b->parameter) + return 0; + return ASN1_TYPE_cmp(a->parameter, b->parameter); + } diff --git a/crypto/x509/x_all.c b/crypto/x509/x_all.c index cbc46e27c..785fd1e4c 100644 --- a/crypto/x509/x_all.c +++ b/crypto/x509/x_all.c @@ -69,6 +69,8 @@ extern const ASN1_ITEM RSAPublicKey_it; int X509_verify(X509 *a, EVP_PKEY *r) { + if (X509_ALGOR_cmp(a->sig_alg, a->cert_info->signature)) + return 0; return(ASN1_item_verify(ASN1_ITEM_rptr(X509_CINF),a->sig_alg, a->signature,a->cert_info,r)); } diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index 9c606c074..fc12a7553 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h @@ -739,7 +739,7 @@ DECLARE_ASN1_FUNCTIONS_fname(ASN1_TYPE, ASN1_ANY, ASN1_TYPE) OPENSSL_EXPORT int ASN1_TYPE_get(ASN1_TYPE *a); OPENSSL_EXPORT void ASN1_TYPE_set(ASN1_TYPE *a, int type, void *value); OPENSSL_EXPORT int ASN1_TYPE_set1(ASN1_TYPE *a, int type, const void *value); -OPENSSL_EXPORT int ASN1_TYPE_cmp(ASN1_TYPE *a, ASN1_TYPE *b); +OPENSSL_EXPORT int ASN1_TYPE_cmp(const ASN1_TYPE *a, const ASN1_TYPE *b); OPENSSL_EXPORT ASN1_OBJECT * ASN1_OBJECT_new(void ); OPENSSL_EXPORT void ASN1_OBJECT_free(ASN1_OBJECT *a); diff --git a/include/openssl/x509.h b/include/openssl/x509.h index d9c2ae135..475f74731 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -720,6 +720,7 @@ OPENSSL_EXPORT int X509_ALGOR_set0(X509_ALGOR *alg, const ASN1_OBJECT *aobj, int OPENSSL_EXPORT void X509_ALGOR_get0(ASN1_OBJECT **paobj, int *pptype, void **ppval, X509_ALGOR *algor); OPENSSL_EXPORT void X509_ALGOR_set_md(X509_ALGOR *alg, const EVP_MD *md); +OPENSSL_EXPORT int X509_ALGOR_cmp(const X509_ALGOR *a, const X509_ALGOR *b); OPENSSL_EXPORT X509_NAME *X509_NAME_dup(X509_NAME *xn); OPENSSL_EXPORT X509_NAME_ENTRY *X509_NAME_ENTRY_dup(X509_NAME_ENTRY *ne); @@ -834,9 +835,6 @@ OPENSSL_EXPORT X509_INFO * X509_INFO_new(void); OPENSSL_EXPORT void X509_INFO_free(X509_INFO *a); OPENSSL_EXPORT char * X509_NAME_oneline(X509_NAME *a,char *buf,int size); -OPENSSL_EXPORT int ASN1_verify(i2d_of_void *i2d, X509_ALGOR *algor1, - ASN1_BIT_STRING *signature,char *data,EVP_PKEY *pkey); - OPENSSL_EXPORT int ASN1_digest(i2d_of_void *i2d,const EVP_MD *type,char *data, unsigned char *md,unsigned int *len); @@ -1306,5 +1304,6 @@ OPENSSL_EXPORT int PKCS7_bundle_certificates( #define X509_R_CRL_ALREADY_DELTA 135 #define X509_R_ERR_ASN1_LIB 136 #define X509_R_AKID_MISMATCH 137 +#define X509_R_INVALID_BIT_STRING_BITS_LEFT 138 #endif