From 7b4b2ac378c6a93512fa22f1820a2a0201686e1e 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/9] Fix stack buffer overflow in pkcs12 --- ChangeLog | 8 ++++++++ library/pkcs12.c | 9 ++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 44f440819..735e44300 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,13 @@ mbed TLS ChangeLog (Sorted per branch, date) + += mbed TLS 1.3.14 reladsed 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. + = mbed TLS 1.3.13 reladsed 2015-09-17 Security diff --git a/library/pkcs12.c b/library/pkcs12.c index f84fd52cd..dff01a778 100644 --- a/library/pkcs12.c +++ b/library/pkcs12.c @@ -87,6 +87,8 @@ static int pkcs12_parse_pbe_params( asn1_buf *params, 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, @@ -95,7 +97,10 @@ static int pkcs12_pbe_derive_key_iv( asn1_buf *pbe_params, md_type_t md_type, int ret, iterations; asn1_buf salt; size_t i; - 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) ); @@ -126,6 +131,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 59efb6a1b9019dde5076d318d1b5130d06b52bf2 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/9] 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 | 6 +++++- library/bignum.c | 9 +++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 735e44300..329e563ec 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,12 +1,16 @@ mbed TLS ChangeLog (Sorted per branch, date) - = mbed TLS 1.3.14 reladsed 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. + * 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. = mbed TLS 1.3.13 reladsed 2015-09-17 diff --git a/library/bignum.c b/library/bignum.c index f479bc9ed..2b1155bb9 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -39,6 +39,7 @@ #include "polarssl/bn_mul.h" #include +#include #if defined(POLARSSL_PLATFORM_C) #include "polarssl/platform.h" @@ -61,9 +62,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 @@ -414,6 +416,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 9bf29bee22d85d370388683e708412bf0eecfc43 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/9] 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 329e563ec..89caddbc4 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. = mbed TLS 1.3.13 reladsed 2015-09-17 diff --git a/library/base64.c b/library/base64.c index ac922a474..2f7bb1428 100644 --- a/library/base64.c +++ b/library/base64.c @@ -190,7 +190,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 5060484cc..054fcffb8 100644 --- a/library/pem.c +++ b/library/pem.c @@ -317,6 +317,9 @@ int pem_read_buffer( pem_context *ctx, const char *header, const char *footer, ( 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 5aff029f9d2b8db78da64655c6808a1dfac6f64f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 28 Sep 2015 18:09:45 +0200 Subject: [PATCH 4/9] Fix potential double-free in ssl_set_psk() --- ChangeLog | 3 +++ library/ssl_tls.c | 2 ++ 2 files changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 89caddbc4..14ae034f2 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 potential double-free if ssl_set_psk() is called repeatedly on + the same ssl_context object and some memory allocations fail. + Found by Guido Vranken. Can not be forced remotely. = mbed TLS 1.3.13 reladsed 2015-09-17 diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 96e867b69..f16bb5310 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4064,7 +4064,9 @@ int ssl_set_psk( ssl_context *ssl, const unsigned char *psk, size_t psk_len, ( ssl->psk_identity = polarssl_malloc( psk_identity_len ) ) == NULL ) { polarssl_free( ssl->psk ); + polarssl_free( ssl->psk_identity ); ssl->psk = NULL; + ssl->psk_identity = NULL; return( POLARSSL_ERR_SSL_MALLOC_FAILED ); } From 48ec2c7b5e77b5c7e0057d1218eb7ef4f989b87e 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 5/9] 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 14ae034f2..651ee4fc6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -18,6 +18,9 @@ Security * Fix potential double-free if ssl_set_psk() is called repeatedly on the same ssl_context object and some memory allocations fail. Found by Guido Vranken. Can not be forced remotely. + * 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. = mbed TLS 1.3.13 reladsed 2015-09-17 diff --git a/include/polarssl/base64.h b/include/polarssl/base64.h index 0f1e8549b..dd11bdab6 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 *olen 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 2f7bb1428..c74351158 100644 --- a/library/base64.c +++ b/library/base64.c @@ -91,15 +91,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 6d6018383e352fac95df95401189a0dbf6d976bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 1 Oct 2015 18:20:55 +0200 Subject: [PATCH 6/9] Fix typos in ChangeLog and comments --- ChangeLog | 2 +- include/polarssl/base64.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 651ee4fc6..4e3ed30ff 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 potential double-free if ssl_set_psk() is called repeatedly on diff --git a/include/polarssl/base64.h b/include/polarssl/base64.h index dd11bdab6..f86219248 100644 --- a/include/polarssl/base64.h +++ b/include/polarssl/base64.h @@ -46,7 +46,7 @@ extern "C" { * *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 *olen is set to SIZE_T_MAX. + * 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 From c7e61a2e3fd58cb8d3eda9ce4fd422a0bb3a6843 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 1 Oct 2015 18:22:54 +0200 Subject: [PATCH 7/9] Fix more typos in ChangeLog --- ChangeLog | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4e3ed30ff..b53c11878 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,6 @@ mbed TLS ChangeLog (Sorted per branch, date) -= mbed TLS 1.3.14 reladsed 2015-10-?? += mbed TLS 1.3.14 released 2015-10-?? Security * Fix stack buffer overflow in pkcs12 decryption (used by @@ -22,7 +22,7 @@ Security buffer is 512MB or larger on 32-bit platforms. Found by Guido Vranken. Not trigerrable remotely in TLS. -= mbed TLS 1.3.13 reladsed 2015-09-17 += mbed TLS 1.3.13 released 2015-09-17 Security * Fix possible client-side NULL pointer dereference (read) when the client From de9c8a573499746d838cece37e6f72702145a129 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 8/9] 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 b53c11878..b26b4b034 100644 --- a/ChangeLog +++ b/ChangeLog @@ -21,6 +21,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. = mbed TLS 1.3.13 released 2015-09-17 diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 379a3abea..82fa2d46e 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -2300,6 +2300,7 @@ static int ssl_write_certificate_request( ssl_context *ssl ) size_t ct_len, sa_len; /* including length bytes */ unsigned char *buf, *p; const x509_crt *crt; + const unsigned char * const end = ssl->out_msg + SSL_MAX_CONTENT_LEN; SSL_DEBUG_MSG( 2, ( "=> write certificate request" ) ); @@ -2406,10 +2407,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 fa647a75a127048e72c0bd1256e8f74a8689a912 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 9/9] 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 f86219248..bdd0c4106 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 c74351158..7de87e51c 100644 --- a/library/base64.c +++ b/library/base64.c @@ -75,6 +75,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 */ @@ -93,9 +95,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 2b1155bb9..c97f16d36 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -39,7 +39,6 @@ #include "polarssl/bn_mul.h" #include -#include #if defined(POLARSSL_PLATFORM_C) #include "polarssl/platform.h" @@ -60,6 +59,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 @@ -416,7 +417,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 );