From 9543373668c933027eb2976307db91ff71529b1e 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 985b7cd50..97ea6419c 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -188,9 +188,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 137015c1b12c4cbf7ca60c49911f59f48b5600e4 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 97ea6419c..5a5445f2f 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -215,14 +215,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]; @@ -236,7 +236,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 6934e9b2887e65887d9d27c5a3e12442b840648c 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 5a5445f2f..8492c19a8 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -247,7 +247,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 a7d2fa7891ab22c886d803604b9e2453eda2fd8d 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 513f24f3a..58a51ff9d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,9 @@ Bugfix invalidated keys of a lifetime of less than a 1s. Fixes #1968. * Fix failure in hmac_drbg in the benchmark sample application, when MBEDTLS_THREADING_C is defined. Found by TrinityTonic, #1095 + * 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.