Merge pull request #3424 from ronald-cron-arm/ssl_write_client_hello-2.7

[Backport 2.7] Bounds checks in ssl_write_client_hello

ChangeLog.d/uniformize_bounds_checks.txt

@@ -0,0 +1,9 @@
+Bugfix
+   * Add additional bounds checks in ssl_write_client_hello() preventing
+     output buffer overflow if the configuration declared a buffer that was
+     too small.
+Changes
+   * Abort the ClientHello writing function as soon as some extension doesn't
+     fit into the record buffer. Previously, such extensions were silently
+     dropped. As a consequence, the TLS handshake now fails when the output
+     buffer is not large enough to hold the ClientHello.

include/mbedtls/error.h

@@ -94,6 +94,7 @@
  * RSA       4   11
  * ECP       4   9 (Started from top)
  * MD        5   5
+ * SSL       5   1 (Started from 0x5E80)
  * CIPHER    6   8
  * SSL       6   17 (Started from top)
  * SSL       7   31

include/mbedtls/ssl.h

@@ -110,6 +110,7 @@
 #define MBEDTLS_ERR_SSL_UNEXPECTED_RECORD                 -0x6700  /**< Record header looks valid but is not expected. */
 #define MBEDTLS_ERR_SSL_NON_FATAL                         -0x6680  /**< The alert message received indicates a non-fatal error. */
 #define MBEDTLS_ERR_SSL_INVALID_VERIFY_HASH               -0x6600  /**< Couldn't set the hash for verifying CertificateVerify */
+#define MBEDTLS_ERR_SSL_BAD_CONFIG                        -0x5E80  /**< Invalid value in SSL config */
 
 /*
  * Various constants
@@ -124,6 +125,9 @@
 #define MBEDTLS_SSL_TRANSPORT_DATAGRAM          1   /*!< DTLS     */
 
 #define MBEDTLS_SSL_MAX_HOST_NAME_LEN           255 /*!< Maximum host name defined in RFC 1035 */
+#define MBEDTLS_SSL_MAX_ALPN_NAME_LEN           255 /*!< Maximum size in bytes of a protocol name in alpn ext., RFC 7301 */
+
+#define MBEDTLS_SSL_MAX_ALPN_LIST_LEN           65535 /*!< Maximum size in bytes of list in alpn ext., RFC 7301          */
 
 /* RFC 6066 section 4, see also mfl_code_to_length in ssl_tls.c
  * NONE must be zero so that memset()ing structure to zero works */

include/mbedtls/ssl_internal.h

@@ -156,6 +156,12 @@
                         + MBEDTLS_SSL_PADDING_ADD                \
                         )
 
+/* Maximum size in bytes of list in sig-hash algorithm ext., RFC 5246 */
+#define MBEDTLS_SSL_MAX_SIG_HASH_ALG_LIST_LEN  65534
+
+/* Maximum size in bytes of list in supported elliptic curve ext., RFC 4492 */
+#define MBEDTLS_SSL_MAX_CURVE_LIST_LEN         65535
+
 /*
  * Check that we obey the standard's message size bounds
  */
@@ -184,6 +190,41 @@
 #define MBEDTLS_TLS_EXT_SUPPORTED_POINT_FORMATS_PRESENT (1 << 0)
 #define MBEDTLS_TLS_EXT_ECJPAKE_KKPP_OK                 (1 << 1)
 
+/**
+ * \brief        This function checks if the remaining size in a buffer is
+ *               greater or equal than a needed space.
+ *
+ * \param cur    Pointer to the current position in the buffer.
+ * \param end    Pointer to one past the end of the buffer.
+ * \param need   Needed space in bytes.
+ *
+ * \return       Zero if the needed space is available in the buffer, non-zero
+ *               otherwise.
+ */
+static inline int mbedtls_ssl_chk_buf_ptr( const uint8_t *cur,
+                                           const uint8_t *end, size_t need )
+{
+    return( ( cur > end ) || ( need > (size_t)( end - cur ) ) );
+}
+
+/**
+ * \brief        This macro checks if the remaining size in a buffer is
+ *               greater or equal than a needed space. If it is not the case,
+ *               it returns an SSL_BUFFER_TOO_SMALL error.
+ *
+ * \param cur    Pointer to the current position in the buffer.
+ * \param end    Pointer to one past the end of the buffer.
+ * \param need   Needed space in bytes.
+ *
+ */
+#define MBEDTLS_SSL_CHK_BUF_PTR( cur, end, need )                        \
+    do {                                                                 \
+        if( mbedtls_ssl_chk_buf_ptr( ( cur ), ( end ), ( need ) ) != 0 ) \
+        {                                                                \
+            return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL );                  \
+        }                                                                \
+    } while( 0 )
+
 #ifdef __cplusplus
 extern "C" {
 #endif

library/error.c

@@ -495,6 +495,8 @@ void mbedtls_strerror( int ret, char *buf, size_t buflen )
             mbedtls_snprintf( buf, buflen, "SSL - The alert message received indicates a non-fatal error" );
         if( use_ret == -(MBEDTLS_ERR_SSL_INVALID_VERIFY_HASH) )
             mbedtls_snprintf( buf, buflen, "SSL - Couldn't set the hash for verifying CertificateVerify" );
+        if( use_ret == -(MBEDTLS_ERR_SSL_BAD_CONFIG) )
+            mbedtls_snprintf( buf, buflen, "SSL - Invalid value in SSL config" );
 #endif /* MBEDTLS_SSL_TLS_C */
 
 #if defined(MBEDTLS_X509_USE_C) || defined(MBEDTLS_X509_CREATE_C)

library/ssl_cookie.c

@@ -137,8 +137,7 @@ static int ssl_cookie_hmac( mbedtls_md_context_t *hmac_ctx,
 {
     unsigned char hmac_out[COOKIE_MD_OUTLEN];
 
-    if( (size_t)( end - *p ) < COOKIE_HMAC_LEN )
+    MBEDTLS_SSL_CHK_BUF_PTR( *p, end, COOKIE_HMAC_LEN );
-        return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL );
 
     if( mbedtls_md_hmac_reset(  hmac_ctx ) != 0 ||
         mbedtls_md_hmac_update( hmac_ctx, time, 4 ) != 0 ||
@@ -168,8 +167,7 @@ int mbedtls_ssl_cookie_write( void *p_ctx,
     if( ctx == NULL || cli_id == NULL )
         return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );
 
-    if( (size_t)( end - *p ) < COOKIE_LEN )
+    MBEDTLS_SSL_CHK_BUF_PTR( *p, end, COOKIE_LEN );
-        return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL );
 
 #if defined(MBEDTLS_HAVE_TIME)
     t = (unsigned long) mbedtls_time( NULL );

library/ssl_ticket.c

@@ -35,6 +35,7 @@
 #define mbedtls_free      free
 #endif
 
+#include "mbedtls/ssl_internal.h"
 #include "mbedtls/ssl_ticket.h"
 
 #include <string.h>
@@ -58,6 +59,19 @@ void mbedtls_ssl_ticket_init( mbedtls_ssl_ticket_context *ctx )
 
 #define MAX_KEY_BYTES 32    /* 256 bits */
 
+#define TICKET_KEY_NAME_BYTES    4
+#define TICKET_IV_BYTES         12
+#define TICKET_CRYPT_LEN_BYTES   2
+#define TICKET_AUTH_TAG_BYTES   16
+
+#define TICKET_MIN_LEN ( TICKET_KEY_NAME_BYTES  +        \
+                         TICKET_IV_BYTES        +        \
+                         TICKET_CRYPT_LEN_BYTES +        \
+                         TICKET_AUTH_TAG_BYTES )
+#define TICKET_ADD_DATA_LEN ( TICKET_KEY_NAME_BYTES  +        \
+                              TICKET_IV_BYTES        +        \
+                              TICKET_CRYPT_LEN_BYTES )
+
 /*
  * Generate/update a key
  */
@@ -282,6 +296,7 @@ static int ssl_load_session( mbedtls_ssl_session *session,
  * The key_name, iv, and length of encrypted_state are the additional
  * authenticated data.
  */
+
 int mbedtls_ssl_ticket_write( void *p_ticket,
                               const mbedtls_ssl_session *session,
                               unsigned char *start,
@@ -293,9 +308,9 @@ int mbedtls_ssl_ticket_write( void *p_ticket,
     mbedtls_ssl_ticket_context *ctx = p_ticket;
     mbedtls_ssl_ticket_key *key;
     unsigned char *key_name = start;
-    unsigned char *iv = start + 4;
+    unsigned char *iv = start + TICKET_KEY_NAME_BYTES;
-    unsigned char *state_len_bytes = iv + 12;
+    unsigned char *state_len_bytes = iv + TICKET_IV_BYTES;
-    unsigned char *state = state_len_bytes + 2;
+    unsigned char *state = state_len_bytes + TICKET_CRYPT_LEN_BYTES;
     unsigned char *tag;
     size_t clear_len, ciph_len;
 
@@ -306,8 +321,7 @@ int mbedtls_ssl_ticket_write( void *p_ticket,
 
     /* We need at least 4 bytes for key_name, 12 for IV, 2 for len 16 for tag,
      * in addition to session itself, that will be checked when writing it. */
-    if( end - start < 4 + 12 + 2 + 16 )
+    MBEDTLS_SSL_CHK_BUF_PTR( start, end, TICKET_MIN_LEN );
-        return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL );
 
 #if defined(MBEDTLS_THREADING_C)
     if( ( ret = mbedtls_mutex_lock( &ctx->mutex ) ) != 0 )
@@ -321,9 +335,9 @@ int mbedtls_ssl_ticket_write( void *p_ticket,
 
     *ticket_lifetime = ctx->ticket_lifetime;
 
-    memcpy( key_name, key->name, 4 );
+    memcpy( key_name, key->name, TICKET_KEY_NAME_BYTES );
 
-    if( ( ret = ctx->f_rng( ctx->p_rng, iv, 12 ) ) != 0 )
+    if( ( ret = ctx->f_rng( ctx->p_rng, iv, TICKET_IV_BYTES ) ) != 0 )
         goto cleanup;
 
     /* Dump session state */
@@ -339,8 +353,11 @@ int mbedtls_ssl_ticket_write( void *p_ticket,
     /* Encrypt and authenticate */
     tag = state + clear_len;
     if( ( ret = mbedtls_cipher_auth_encrypt( &key->ctx,
-                    iv, 12, key_name, 4 + 12 + 2,
+                    iv, TICKET_IV_BYTES,
-                    state, clear_len, state, &ciph_len, tag, 16 ) ) != 0 )
+                    /* Additional data: key name, IV and length */
+                    key_name, TICKET_ADD_DATA_LEN,
+                    state, clear_len, state, &ciph_len,
+                    tag, TICKET_AUTH_TAG_BYTES ) ) != 0 )
     {
         goto cleanup;
     }
@@ -350,7 +367,7 @@ int mbedtls_ssl_ticket_write( void *p_ticket,
         goto cleanup;
     }
 
-    *tlen = 4 + 12 + 2 + 16 + ciph_len;
+    *tlen = TICKET_MIN_LEN + ciph_len;
 
 cleanup:
 #if defined(MBEDTLS_THREADING_C)
@@ -389,17 +406,16 @@ int mbedtls_ssl_ticket_parse( void *p_ticket,
     mbedtls_ssl_ticket_context *ctx = p_ticket;
     mbedtls_ssl_ticket_key *key;
     unsigned char *key_name = buf;
-    unsigned char *iv = buf + 4;
+    unsigned char *iv = buf + TICKET_KEY_NAME_BYTES;
-    unsigned char *enc_len_p = iv + 12;
+    unsigned char *enc_len_p = iv + TICKET_IV_BYTES;
-    unsigned char *ticket = enc_len_p + 2;
+    unsigned char *ticket = enc_len_p + TICKET_CRYPT_LEN_BYTES;
     unsigned char *tag;
     size_t enc_len, clear_len;
 
     if( ctx == NULL || ctx->f_rng == NULL )
         return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );
 
-    /* See mbedtls_ssl_ticket_write() */
+    if( len < TICKET_MIN_LEN )
-    if( len < 4 + 12 + 2 + 16 )
         return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );
 
 #if defined(MBEDTLS_THREADING_C)
@@ -413,7 +429,7 @@ int mbedtls_ssl_ticket_parse( void *p_ticket,
     enc_len = ( enc_len_p[0] << 8 ) | enc_len_p[1];
     tag = ticket + enc_len;
 
-    if( len != 4 + 12 + 2 + enc_len + 16 )
+    if( len != TICKET_MIN_LEN + enc_len )
     {
         ret = MBEDTLS_ERR_SSL_BAD_INPUT_DATA;
         goto cleanup;
@@ -429,9 +445,13 @@ int mbedtls_ssl_ticket_parse( void *p_ticket,
     }
 
     /* Decrypt and authenticate */
-    if( ( ret = mbedtls_cipher_auth_decrypt( &key->ctx, iv, 12,
+    if( ( ret = mbedtls_cipher_auth_decrypt( &key->ctx,
-                    key_name, 4 + 12 + 2, ticket, enc_len,
+                    iv, TICKET_IV_BYTES,
-                    ticket, &clear_len, tag, 16 ) ) != 0 )
+                    /* Additional data: key name, IV and length */
+                    key_name, TICKET_ADD_DATA_LEN,
+                    ticket, enc_len,
+                    ticket, &clear_len,
+                    tag, TICKET_AUTH_TAG_BYTES ) ) != 0 )
     {
         if( ret == MBEDTLS_ERR_CIPHER_AUTH_FAILED )
             ret = MBEDTLS_ERR_SSL_INVALID_MAC;

library/ssl_tls.c

@@ -6526,7 +6526,9 @@ int mbedtls_ssl_conf_alpn_protocols( mbedtls_ssl_config *conf, const char **prot
         cur_len = strlen( *p );
         tot_len += cur_len;
 
-        if( cur_len == 0 || cur_len > 255 || tot_len > 65535 )
+        if( ( cur_len == 0 ) ||
+            ( cur_len > MBEDTLS_SSL_MAX_ALPN_NAME_LEN ) ||
+            ( tot_len > MBEDTLS_SSL_MAX_ALPN_LIST_LEN ) )
             return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA );
     }