From af07365b498b4cf183493a86dcfd768b3a5e8eaa Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 12 Nov 2015 23:09:30 -0800 Subject: [PATCH] Check for overflow when parsing a CBS with d2i_*. Until we've done away with the d2i_* stack completely, boundaries need to be mindful of the type mismatch. d2i_* takes a long, not a size_t. Change-Id: If02f9ca2cfde02d0929ac18275d09bf5df400f3a Reviewed-on: https://boringssl-review.googlesource.com/6491 Reviewed-by: Adam Langley --- crypto/pkcs8/pkcs8.c | 18 ++++++++++++++---- crypto/x509/pkcs7.c | 11 +++++++++-- ssl/s3_clnt.c | 6 ++++-- ssl/s3_srvr.c | 3 ++- ssl/ssl_asn1.c | 6 +++++- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/crypto/pkcs8/pkcs8.c b/crypto/pkcs8/pkcs8.c index c0978815..31a34a7c 100644 --- a/crypto/pkcs8/pkcs8.c +++ b/crypto/pkcs8/pkcs8.c @@ -773,13 +773,14 @@ static int PKCS12_handle_content_info(CBS *content_info, unsigned depth, goto err; } - if (OBJ_cbs2nid(&contents_type) != NID_pkcs7_data) { + if (OBJ_cbs2nid(&contents_type) != NID_pkcs7_data || + CBS_len(&ai) > LONG_MAX) { OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); goto err; } inp = CBS_data(&ai); - algor = d2i_X509_ALGOR(NULL, &inp, CBS_len(&ai)); + algor = d2i_X509_ALGOR(NULL, &inp, (long)CBS_len(&ai)); if (algor == NULL) { goto err; } @@ -822,9 +823,14 @@ static int PKCS12_handle_content_info(CBS *content_info, unsigned depth, goto err; } + if (CBS_len(&wrapped_contents) > LONG_MAX) { + OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); + goto err; + } + /* encrypted isn't actually an X.509 signature, but it has the same * structure as one and so |X509_SIG| is reused to store it. */ - encrypted = d2i_X509_SIG(NULL, &inp, CBS_len(&wrapped_contents)); + encrypted = d2i_X509_SIG(NULL, &inp, (long)CBS_len(&wrapped_contents)); if (encrypted == NULL) { OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); goto err; @@ -861,8 +867,12 @@ static int PKCS12_handle_content_info(CBS *content_info, unsigned depth, } if (OBJ_cbs2nid(&cert_type) == NID_x509Certificate) { + if (CBS_len(&cert) > LONG_MAX) { + OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); + goto err; + } const uint8_t *inp = CBS_data(&cert); - X509 *x509 = d2i_X509(NULL, &inp, CBS_len(&cert)); + X509 *x509 = d2i_X509(NULL, &inp, (long)CBS_len(&cert)); if (!x509) { OPENSSL_PUT_ERROR(PKCS8, PKCS8_R_BAD_PKCS12_DATA); goto err; diff --git a/crypto/x509/pkcs7.c b/crypto/x509/pkcs7.c index 2087f948..9e6a52f2 100644 --- a/crypto/x509/pkcs7.c +++ b/crypto/x509/pkcs7.c @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -114,8 +115,11 @@ int PKCS7_get_certificates(STACK_OF(X509) *out_certs, CBS *cbs) { goto err; } + if (CBS_len(&cert) > LONG_MAX) { + goto err; + } inp = CBS_data(&cert); - x509 = d2i_X509(NULL, &inp, CBS_len(&cert)); + x509 = d2i_X509(NULL, &inp, (long)CBS_len(&cert)); if (!x509) { goto err; } @@ -181,8 +185,11 @@ int PKCS7_get_CRLs(STACK_OF(X509_CRL) *out_crls, CBS *cbs) { goto err; } + if (CBS_len(&crl_data) > LONG_MAX) { + goto err; + } inp = CBS_data(&crl_data); - crl = d2i_X509_CRL(NULL, &inp, CBS_len(&crl_data)); + crl = d2i_X509_CRL(NULL, &inp, (long)CBS_len(&crl_data)); if (!crl) { goto err; } diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index eb96780b..352dae30 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -1003,8 +1003,9 @@ int ssl3_get_server_certificate(SSL *s) { OPENSSL_PUT_ERROR(SSL, SSL_R_CERT_LENGTH_MISMATCH); goto f_err; } + /* A u24 length cannot overflow a long. */ data = CBS_data(&certificate); - x = d2i_X509(NULL, &data, CBS_len(&certificate)); + x = d2i_X509(NULL, &data, (long)CBS_len(&certificate)); if (x == NULL) { al = SSL_AD_BAD_CERTIFICATE; OPENSSL_PUT_ERROR(SSL, ERR_R_ASN1_LIB); @@ -1410,7 +1411,8 @@ int ssl3_get_certificate_request(SSL *s) { data = CBS_data(&distinguished_name); - xn = d2i_X509_NAME(NULL, &data, CBS_len(&distinguished_name)); + /* A u16 length cannot overflow a long. */ + xn = d2i_X509_NAME(NULL, &data, (long)CBS_len(&distinguished_name)); if (xn == NULL) { ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); OPENSSL_PUT_ERROR(SSL, ERR_R_ASN1_LIB); diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 8cfa0e6c..1616b22d 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -2193,8 +2193,9 @@ int ssl3_get_client_certificate(SSL *s) { } is_first_certificate = 0; + /* A u24 length cannot overflow a long. */ data = CBS_data(&certificate); - x = d2i_X509(NULL, &data, CBS_len(&certificate)); + x = d2i_X509(NULL, &data, (long)CBS_len(&certificate)); if (x == NULL) { al = SSL_AD_BAD_CERTIFICATE; OPENSSL_PUT_ERROR(SSL, ERR_R_ASN1_LIB); diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c index 0ad4a11b..5ec33eb4 100644 --- a/ssl/ssl_asn1.c +++ b/ssl/ssl_asn1.c @@ -492,8 +492,12 @@ static int SSL_SESSION_parse_u32(CBS *cbs, uint32_t *out, unsigned tag, } static X509 *parse_x509(CBS *cbs) { + if (CBS_len(cbs) > LONG_MAX) { + OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION); + return NULL; + } const uint8_t *ptr = CBS_data(cbs); - X509 *ret = d2i_X509(NULL, &ptr, CBS_len(cbs)); + X509 *ret = d2i_X509(NULL, &ptr, (long)CBS_len(cbs)); if (ret == NULL) { return NULL; }