From e67f7a7683c4ef0242cb4e8ddaefb4b81e64acbb Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 Oct 2018 13:31:49 +0100 Subject: [PATCH 1/4] Use brackets around shift operations Use `( x >> y ) & z` instead of `x >> y & z`. Both are equivalent by operator precedence, but the former is more readable and the commonly used idiom in the library. --- library/ssl_ticket.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index ad2d5264546e..e80df28159ce 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -192,9 +192,9 @@ static int ssl_save_session( const mbedtls_ssl_session *session, if( left < 3 + cert_len ) return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); - *p++ = (unsigned char)( cert_len >> 16 & 0xFF ); - *p++ = (unsigned char)( cert_len >> 8 & 0xFF ); - *p++ = (unsigned char)( cert_len & 0xFF ); + *p++ = (unsigned char)( ( cert_len >> 16 ) & 0xFF ); + *p++ = (unsigned char)( ( cert_len >> 8 ) & 0xFF ); + *p++ = (unsigned char)( ( cert_len ) & 0xFF ); if( session->peer_cert != NULL ) memcpy( p, session->peer_cert->raw.p, cert_len ); From fa95a6ad22ed7e187362b3e923fcedf90e0f4244 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 Oct 2018 13:33:02 +0100 Subject: [PATCH 2/4] Fix unsafe bounds checks in ssl_load_session() Fixes #659 reported by Guido Vranken. --- library/ssl_ticket.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index e80df28159ce..e6d593d1e7fe 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -219,14 +219,14 @@ static int ssl_load_session( mbedtls_ssl_session *session, size_t cert_len; #endif /* MBEDTLS_X509_CRT_PARSE_C */ - if( p + sizeof( mbedtls_ssl_session ) > end ) + if( sizeof( mbedtls_ssl_session ) > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); memcpy( session, p, sizeof( mbedtls_ssl_session ) ); p += sizeof( mbedtls_ssl_session ); #if defined(MBEDTLS_X509_CRT_PARSE_C) - if( p + 3 > end ) + if( 3 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); cert_len = ( p[0] << 16 ) | ( p[1] << 8 ) | p[2]; @@ -240,7 +240,7 @@ static int ssl_load_session( mbedtls_ssl_session *session, { int ret; - if( p + cert_len > end ) + if( cert_len > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); session->peer_cert = mbedtls_calloc( 1, sizeof( mbedtls_x509_crt ) ); From 52adf349facffaa79cd6babd79ecac455d194ee7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 Oct 2018 13:41:37 +0100 Subject: [PATCH 3/4] Indentation fix --- library/ssl_ticket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index e6d593d1e7fe..555c7b63bf95 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -251,7 +251,7 @@ static int ssl_load_session( mbedtls_ssl_session *session, mbedtls_x509_crt_init( session->peer_cert ); if( ( ret = mbedtls_x509_crt_parse_der( session->peer_cert, - p, cert_len ) ) != 0 ) + p, cert_len ) ) != 0 ) { mbedtls_x509_crt_free( session->peer_cert ); mbedtls_free( session->peer_cert ); From 7cf2857828dd2a97c70cc441465f9c342e4e774d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 Oct 2018 13:41:43 +0100 Subject: [PATCH 4/4] Adapt ChangeLog --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 0a9dc4f8d0b5..7f9563afa536 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,9 @@ Bugfix MBEDTLS_THREADING_C is defined. Found by TrinityTonic, #1095 * Fix a bug in the update function for SSL ticket keys which previously invalidated keys of a lifetime of less than a 1s. Fixes #1968. + * Fix an unsafe bounds check when restoring an SSL session from a ticket. + This could lead to a buffer overflow, but only in case ticket authentication + was broken. Reported and fix suggested by Guido Vranken in #659. Changes * Add tests for session resumption in DTLS.