From acbb0501186b1751373eafae862ddfd778dda11f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 10 Dec 2015 13:57:27 +0100 Subject: [PATCH 1/3] Make documentation more explicit on TLS errors fixes #358 --- include/mbedtls/ssl.h | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 810409c654..8fd0ee99a4 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -2167,7 +2167,8 @@ int mbedtls_ssl_get_session( const mbedtls_ssl_context *ssl, mbedtls_ssl_session * \note If this function returns something other than 0 or * MBEDTLS_ERR_SSL_WANT_READ/WRITE, then the ssl context * becomes unusable, and you should either free it or call - * \c mbedtls_ssl_session_reset() on it before re-using it. + * \c mbedtls_ssl_session_reset() on it before re-using it for + * a new connection; the current connection must be closed. * * \note If DTLS is in use, then you may choose to handle * MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED specially for logging @@ -2183,6 +2184,12 @@ int mbedtls_ssl_handshake( mbedtls_ssl_context *ssl ); * the following state after execution of this function. * Do not call this function if state is MBEDTLS_SSL_HANDSHAKE_OVER. * + * \note If this function returns something other than 0 or + * MBEDTLS_ERR_SSL_WANT_READ/WRITE, then the ssl context + * becomes unusable, and you should either free it or call + * \c mbedtls_ssl_session_reset() on it before re-using it for + * a new connection; the current connection must be closed. + * * \param ssl SSL context * * \return 0 if successful, or @@ -2201,6 +2208,12 @@ int mbedtls_ssl_handshake_step( mbedtls_ssl_context *ssl ); * \param ssl SSL context * * \return 0 if successful, or any mbedtls_ssl_handshake() return value. + * + * \note If this function returns something other than 0 or + * MBEDTLS_ERR_SSL_WANT_READ/WRITE, then the ssl context + * becomes unusable, and you should either free it or call + * \c mbedtls_ssl_session_reset() on it before re-using it for + * a new connection; the current connection must be closed. */ int mbedtls_ssl_renegotiate( mbedtls_ssl_context *ssl ); #endif /* MBEDTLS_SSL_RENEGOTIATION */ @@ -2218,6 +2231,13 @@ int mbedtls_ssl_renegotiate( mbedtls_ssl_context *ssl ); * MBEDTLS_ERR_SSL_CLIENT_RECONNECT (see below), or * another negative error code. * + * \note If this function returns something other than a positive + * value or MBEDTLS_ERR_SSL_WANT_READ/WRITE or + * MBEDTLS_ERR_SSL_CLIENT_RECONNECT, then the ssl context + * becomes unusable, and you should either free it or call + * \c mbedtls_ssl_session_reset() on it before re-using it for + * a new connection; the current connection must be closed. + * * \note When this function return MBEDTLS_ERR_SSL_CLIENT_RECONNECT * (which can only happen server-side), it means that a client * is initiating a new connection using the same source port. @@ -2251,6 +2271,12 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) * or MBEDTLS_ERR_SSL_WANT_WRITE of MBEDTLS_ERR_SSL_WANT_READ, * or another negative error code. * + * \note If this function returns something other than a positive + * value or MBEDTLS_ERR_SSL_WANT_READ/WRITE, the ssl context + * becomes unusable, and you should either free it or call + * \c mbedtls_ssl_session_reset() on it before re-using it for + * a new connection; the current connection must be closed. + * * \note When this function returns MBEDTLS_ERR_SSL_WANT_WRITE/READ, * it must be called later with the *same* arguments, * until it returns a positive value. @@ -2274,6 +2300,12 @@ int mbedtls_ssl_write( mbedtls_ssl_context *ssl, const unsigned char *buf, size_ * \param message The alert message (SSL_ALERT_MSG_*) * * \return 0 if successful, or a specific SSL error code. + * + * \note If this function returns something other than 0 or + * MBEDTLS_ERR_SSL_WANT_READ/WRITE, then the ssl context + * becomes unusable, and you should either free it or call + * \c mbedtls_ssl_session_reset() on it before re-using it for + * a new connection; the current connection must be closed. */ int mbedtls_ssl_send_alert_message( mbedtls_ssl_context *ssl, unsigned char level, @@ -2282,6 +2314,14 @@ int mbedtls_ssl_send_alert_message( mbedtls_ssl_context *ssl, * \brief Notify the peer that the connection is being closed * * \param ssl SSL context + * + * \return 0 if successful, or a specific SSL error code. + * + * \note If this function returns something other than 0 or + * MBEDTLS_ERR_SSL_WANT_READ/WRITE, then the ssl context + * becomes unusable, and you should either free it or call + * \c mbedtls_ssl_session_reset() on it before re-using it for + * a new connection; the current connection must be closed. */ int mbedtls_ssl_close_notify( mbedtls_ssl_context *ssl ); From 7f17155ac6c7cd7e191aa090bfe2547094c327c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 10 Dec 2015 14:36:25 +0100 Subject: [PATCH 2/3] Avoid seemingly-possible overflow By looking just at that test, it looks like 2 + dn_size could overflow. In fact that can't happen as that would mean we've read a CA cert of size is too big to be represented by a size_t. However, it's best for code to be more obviously free of overflow without having to reason about the bigger picture. --- library/ssl_srv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 9afd399895..6b5b461e17 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -2584,7 +2584,9 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) { dn_size = crt->subject_raw.len; - if( end < p || (size_t)( end - p ) < 2 + dn_size ) + if( end < p || + (size_t)( end - p ) < dn_size || + (size_t)( end - p ) < 2 + dn_size ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "skipping CAs: buffer too short" ) ); break; From 1e07562da40d9bdb20ba9b14f9138bba96d1220b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 10 Dec 2015 14:46:25 +0100 Subject: [PATCH 3/3] Fix wrong length limit in GCM See for example page 8 of http://csrc.nist.gov/publications/nistpubs/800-38D/SP-800-38D.pdf The previous constant probably came from a typo as it was 2^26 - 2^5 instead of 2^36 - 2^5. Clearly the intention was to allow for a constant bigger than 2^32 as the ull suffix and cast to uint64_t show. fixes #362 --- ChangeLog | 5 +++++ library/gcm.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 8a736f971f..4526254da8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.2.1 released 2015-12-xx + +Bugfix + * Fix over-restricive length limit in GCM. Found by Andreas-N. #362 + = mbed TLS 2.2.0 released 2015-11-04 Security diff --git a/library/gcm.c b/library/gcm.c index 4298254d2b..aaacf97d61 100644 --- a/library/gcm.c +++ b/library/gcm.c @@ -362,7 +362,7 @@ int mbedtls_gcm_update( mbedtls_gcm_context *ctx, /* Total length is restricted to 2^39 - 256 bits, ie 2^36 - 2^5 bytes * Also check for possible overflow */ if( ctx->len + length < ctx->len || - (uint64_t) ctx->len + length > 0x03FFFFE0ull ) + (uint64_t) ctx->len + length > 0xFFFFFFFE0ull ) { return( MBEDTLS_ERR_GCM_BAD_INPUT ); }