From 73011bba956e27b6c5a934cfbeaf3d2c429c9c2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 28 Sep 2015 18:34:48 +0200 Subject: [PATCH 1/7] Fix stack buffer overflow in pkcs12 --- ChangeLog | 7 +++++++ library/pkcs12.c | 9 ++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 6f8181b5e..b496e0213 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,12 @@ PolarSSL ChangeLog += Version 1.2.16 released 2015-10-?? + +Security + * Fix stack buffer overflow in pkcs12 decryption (used by + mbedtls_pk_parse_key(file)() when the password is > 129 bytes. + Found by Guido Vranken. Not triggerable remotely. + = Version 1.2.16 released 2015-09-17 Security diff --git a/library/pkcs12.c b/library/pkcs12.c index 8e42d2035..498a3febd 100644 --- a/library/pkcs12.c +++ b/library/pkcs12.c @@ -80,6 +80,8 @@ static int pkcs12_parse_pbe_params( unsigned char **p, return( 0 ); } +#define PKCS12_MAX_PWDLEN 128 + static int pkcs12_pbe_derive_key_iv( asn1_buf *pbe_params, md_type_t md_type, const unsigned char *pwd, size_t pwdlen, unsigned char *key, size_t keylen, @@ -89,7 +91,10 @@ static int pkcs12_pbe_derive_key_iv( asn1_buf *pbe_params, md_type_t md_type, asn1_buf salt; size_t i; unsigned char *p, *end; - unsigned char unipwd[258]; + unsigned char unipwd[PKCS12_MAX_PWDLEN * 2 + 2]; + + if( pwdlen > PKCS12_MAX_PWDLEN ) + return( POLARSSL_ERR_PKCS12_BAD_INPUT_DATA ); memset(&salt, 0, sizeof(asn1_buf)); memset(&unipwd, 0, sizeof(unipwd)); @@ -122,6 +127,8 @@ static int pkcs12_pbe_derive_key_iv( asn1_buf *pbe_params, md_type_t md_type, return( 0 ); } +#undef PKCS12_MAX_PWDLEN + int pkcs12_pbe_sha1_rc4_128( asn1_buf *pbe_params, int mode, const unsigned char *pwd, size_t pwdlen, const unsigned char *data, size_t len, From 9b75305d6a77383ddcacea46106aac55513d4a9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 28 Sep 2015 13:48:04 +0200 Subject: [PATCH 2/7] Fix potential buffer overflow in mpi_read_string() Found by Guido Vranken. Two possible integer overflows (during << 2 or addition in BITS_TO_LIMB()) could result in far too few memory to be allocated, then overflowing the buffer in the subsequent for loop. Both integer overflows happen when slen is close to or greater than SIZE_T_MAX >> 2 (ie 2^30 on a 32 bit system). Note: one could also avoid those overflows by changing BITS_TO_LIMB(s << 2) to CHARS_TO_LIMB(s >> 1) but the solution implemented looks more robust with respect to future code changes. --- ChangeLog | 5 +++++ library/bignum.c | 9 +++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index b496e0213..0f8d4b844 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,6 +6,11 @@ Security * Fix stack buffer overflow in pkcs12 decryption (used by mbedtls_pk_parse_key(file)() when the password is > 129 bytes. Found by Guido Vranken. Not triggerable remotely. + * Fix potential buffer overflow in mbedtls_mpi_read_string(). + Found by Guido Vranken. Not exploitable remotely in the context of TLS, + but might be in other uses. On 32 bit machines, requires reading a string + of close to or larger than 1GB to exploit; on 64 bit machines, would require + reading a string of close to or larger than 2^62 bytes. = Version 1.2.16 released 2015-09-17 diff --git a/library/bignum.c b/library/bignum.c index bbadd01ce..9544dc991 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -34,6 +34,7 @@ #include "polarssl/bignum.h" #include "polarssl/bn_mul.h" +#include #include /* Implementation that should never be optimized out by the compiler */ @@ -47,9 +48,10 @@ static void polarssl_zeroize( void *v, size_t n ) { /* * Convert between bits/chars and number of limbs + * Divide first in order to avoid potential overflows */ -#define BITS_TO_LIMBS(i) (((i) + biL - 1) / biL) -#define CHARS_TO_LIMBS(i) (((i) + ciL - 1) / ciL) +#define BITS_TO_LIMBS(i) ( (i) / biL + ( (i) % biL != 0 ) ) +#define CHARS_TO_LIMBS(i) ( (i) / ciL + ( (i) % ciL != 0 ) ) /* * Initialize one MPI @@ -287,6 +289,9 @@ int mpi_read_string( mpi *X, int radix, const char *s ) if( radix == 16 ) { + if( slen > SIZE_T_MAX >> 2 ) + return( POLARSSL_ERR_MPI_BAD_INPUT_DATA ); + n = BITS_TO_LIMBS( slen << 2 ); MPI_CHK( mpi_grow( X, n ) ); From b73ce45b3f87672f5dfcc4e136ae8e9771c5552d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 28 Sep 2015 18:27:15 +0200 Subject: [PATCH 3/7] Fix potential random malloc in pem_read() --- ChangeLog | 4 ++++ library/base64.c | 3 +++ library/pem.c | 3 +++ 3 files changed, 10 insertions(+) diff --git a/ChangeLog b/ChangeLog index 0f8d4b844..d58409089 100644 --- a/ChangeLog +++ b/ChangeLog @@ -11,6 +11,10 @@ Security but might be in other uses. On 32 bit machines, requires reading a string of close to or larger than 1GB to exploit; on 64 bit machines, would require reading a string of close to or larger than 2^62 bytes. + * Fix potential random memory allocation in mbedtls_pem_read_buffer() + on crafted PEM input data. Found an fix provided by Guid Vranken. + Not triggerable remotely in TLS. Triggerable remotely if you accept PEM + data from an untrusted source. = Version 1.2.16 released 2015-09-17 diff --git a/library/base64.c b/library/base64.c index c94995b9e..dba4c231c 100644 --- a/library/base64.c +++ b/library/base64.c @@ -176,7 +176,10 @@ int base64_decode( unsigned char *dst, size_t *dlen, } if( n == 0 ) + { + *dlen = 0; return( 0 ); + } n = ((n * 6) + 7) >> 3; n -= j; diff --git a/library/pem.c b/library/pem.c index 5c973ac25..81098eea5 100644 --- a/library/pem.c +++ b/library/pem.c @@ -287,6 +287,9 @@ int pem_read_buffer( pem_context *ctx, char *header, char *footer, const unsigne #endif /* POLARSSL_MD5_C && (POLARSSL_AES_C || POLARSSL_DES_C) */ } + if( s1 == s2 ) + return( POLARSSL_ERR_PEM_INVALID_DATA ); + len = 0; ret = base64_decode( NULL, &len, s1, s2 - s1 ); From e4e4be77bed48deb01e61dff07f2d3231a9b9938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 30 Sep 2015 16:30:28 +0200 Subject: [PATCH 4/7] Fix potential overflow in base64_encode --- ChangeLog | 3 +++ include/polarssl/base64.h | 3 +++ library/base64.c | 11 ++++++----- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index d58409089..cb0ba22e4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -15,6 +15,9 @@ Security on crafted PEM input data. Found an fix provided by Guid Vranken. Not triggerable remotely in TLS. Triggerable remotely if you accept PEM data from an untrusted source. + * Fix possible heap buffer overflow in base64_encode() when the input + buffer is 512MB or larger on 32-bit platforms. + Found by Guido Vranken. Not trigerrable remotely in TLS. = Version 1.2.16 released 2015-09-17 diff --git a/include/polarssl/base64.h b/include/polarssl/base64.h index db95cb1de..8b772dfdd 100644 --- a/include/polarssl/base64.h +++ b/include/polarssl/base64.h @@ -25,6 +25,7 @@ #define POLARSSL_BASE64_H #include +#include #define POLARSSL_ERR_BASE64_BUFFER_TOO_SMALL -0x002A /**< Output buffer too small. */ #define POLARSSL_ERR_BASE64_INVALID_CHARACTER -0x002C /**< Invalid character in input. */ @@ -44,6 +45,8 @@ extern "C" { * \return 0 if successful, or POLARSSL_ERR_BASE64_BUFFER_TOO_SMALL. * *dlen is always updated to reflect the amount * of data that has (or would have) been written. + * If that length cannot be represented, then no data is + * written to the buffer and *dlen is set to SIZE_T_MAX. * * \note Call this function with *dlen = 0 to obtain the * required buffer size in *dlen diff --git a/library/base64.c b/library/base64.c index dba4c231c..c492cc0c7 100644 --- a/library/base64.c +++ b/library/base64.c @@ -77,15 +77,16 @@ int base64_encode( unsigned char *dst, size_t *dlen, return( 0 ); } - n = (slen << 3) / 6; + n = slen / 3 + ( slen % 3 != 0 ); - switch( (slen << 3) - (n * 6) ) + if( n > ( SIZE_T_MAX - 1 ) / 4 ) { - case 2: n += 3; break; - case 4: n += 2; break; - default: break; + *dlen = SIZE_T_MAX; + return( POLARSSL_ERR_BASE64_BUFFER_TOO_SMALL ); } + n *= 4; + if( *dlen < n + 1 ) { *dlen = n + 1; From 9a656a0aaa2ee184fc59a2a4dd1dcf6d1d536c43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 1 Oct 2015 18:16:46 +0200 Subject: [PATCH 5/7] Fix typos in ChangeLog --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index cb0ba22e4..a7c12a074 100644 --- a/ChangeLog +++ b/ChangeLog @@ -12,7 +12,7 @@ Security of close to or larger than 1GB to exploit; on 64 bit machines, would require reading a string of close to or larger than 2^62 bytes. * Fix potential random memory allocation in mbedtls_pem_read_buffer() - on crafted PEM input data. Found an fix provided by Guid Vranken. + on crafted PEM input data. Found and fix provided by Guido Vranken. Not triggerable remotely in TLS. Triggerable remotely if you accept PEM data from an untrusted source. * Fix possible heap buffer overflow in base64_encode() when the input From d64f1ad98b40417d583b80e23b646737cf086dcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 2 Oct 2015 11:16:47 +0200 Subject: [PATCH 6/7] Fix potential overflow in CertificateRequest --- ChangeLog | 4 ++++ library/ssl_srv.c | 11 ++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index a7c12a074..6c735e7c1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -18,6 +18,10 @@ Security * Fix possible heap buffer overflow in base64_encode() when the input buffer is 512MB or larger on 32-bit platforms. Found by Guido Vranken. Not trigerrable remotely in TLS. + * Fix potential heap buffer overflow in servers that perform client + authentication against a crafted CA cert. Cannot be triggered remotely + unless you allow third parties to pick trust CAs for client auth. + Found by Guido Vranken. = Version 1.2.16 released 2015-09-17 diff --git a/library/ssl_srv.c b/library/ssl_srv.c index cd9802a15..2f4ae6925 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -923,6 +923,7 @@ static int ssl_write_certificate_request( ssl_context *ssl ) size_t n = 0, dn_size, total_dn_size; unsigned char *buf, *p; const x509_cert *crt; + const unsigned char * const end = ssl->out_msg + SSL_MAX_CONTENT_LEN; SSL_DEBUG_MSG( 2, ( "=> write certificate request" ) ); @@ -987,10 +988,14 @@ static int ssl_write_certificate_request( ssl_context *ssl ) total_dn_size = 0; while( crt != NULL && crt->version != 0) { - if( p - buf > 4096 ) - break; - dn_size = crt->subject_raw.len; + + if( end < p || (size_t)( end - p ) < 2 + dn_size ) + { + SSL_DEBUG_MSG( 1, ( "skipping CAs: buffer too short" ) ); + break; + } + *p++ = (unsigned char)( dn_size >> 8 ); *p++ = (unsigned char)( dn_size ); memcpy( p, crt->subject_raw.p, dn_size ); From 42571ddb4ef2d17d57a4969a8fc4d1de3f840636 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 Oct 2015 15:23:11 +0100 Subject: [PATCH 7/7] Fix references to non-standard SIZE_T_MAX Turns out C99 doesn't define SIZE_T_MAX, so let's not use it. --- include/polarssl/base64.h | 1 - library/base64.c | 6 ++++-- library/bignum.c | 5 +++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/include/polarssl/base64.h b/include/polarssl/base64.h index 8b772dfdd..32634f8c9 100644 --- a/include/polarssl/base64.h +++ b/include/polarssl/base64.h @@ -25,7 +25,6 @@ #define POLARSSL_BASE64_H #include -#include #define POLARSSL_ERR_BASE64_BUFFER_TOO_SMALL -0x002A /**< Output buffer too small. */ #define POLARSSL_ERR_BASE64_INVALID_CHARACTER -0x002C /**< Invalid character in input. */ diff --git a/library/base64.c b/library/base64.c index c492cc0c7..2b43a940a 100644 --- a/library/base64.c +++ b/library/base64.c @@ -61,6 +61,8 @@ static const unsigned char base64_dec_map[128] = 49, 50, 51, 127, 127, 127, 127, 127 }; +#define BASE64_SIZE_T_MAX ( (size_t) -1 ) /* SIZE_T_MAX is not standard */ + /* * Encode a buffer into base64 format */ @@ -79,9 +81,9 @@ int base64_encode( unsigned char *dst, size_t *dlen, n = slen / 3 + ( slen % 3 != 0 ); - if( n > ( SIZE_T_MAX - 1 ) / 4 ) + if( n > ( BASE64_SIZE_T_MAX - 1 ) / 4 ) { - *dlen = SIZE_T_MAX; + *dlen = BASE64_SIZE_T_MAX; return( POLARSSL_ERR_BASE64_BUFFER_TOO_SMALL ); } diff --git a/library/bignum.c b/library/bignum.c index 9544dc991..14a3dc7cf 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -34,7 +34,6 @@ #include "polarssl/bignum.h" #include "polarssl/bn_mul.h" -#include #include /* Implementation that should never be optimized out by the compiler */ @@ -46,6 +45,8 @@ static void polarssl_zeroize( void *v, size_t n ) { #define biL (ciL << 3) /* bits in limb */ #define biH (ciL << 2) /* half limb size */ +#define MPI_SIZE_T_MAX ( (size_t) -1 ) /* SIZE_T_MAX is not standard */ + /* * Convert between bits/chars and number of limbs * Divide first in order to avoid potential overflows @@ -289,7 +290,7 @@ int mpi_read_string( mpi *X, int radix, const char *s ) if( radix == 16 ) { - if( slen > SIZE_T_MAX >> 2 ) + if( slen > MPI_SIZE_T_MAX >> 2 ) return( POLARSSL_ERR_MPI_BAD_INPUT_DATA ); n = BITS_TO_LIMBS( slen << 2 );