Don't store client-supported ECs in heap-allocated buffer

So far, the client-proposed list of elliptic curves was stored for the
duration of the entire handshake in a heap-allocated buffer referenced
from mbedtls_ssl_handshake_params::curves. It is used in the following
places:
1) When the server chooses a suitable ciphersuite, it checks that
   it has a certificate matching the ciphersuite; in particular, if
   the ciphersuite involves ECDHE, the server needs an EC certificate
   with a curve suitable for the client.
2) When performing the ECDHE key exchange, the server choose one
   curve among those proposed by the client which matches the server's
   own supported curve configuration.

This commit removes the hold back the entire client-side curve list
during the handshake, by performing (1) and (2) on during ClientHello
parsing, and in case of (2) only remembering the curve chosen for ECDHE
within mbedtls_ssl_handshake_params.

include/mbedtls/ssl_internal.h

@@ -383,7 +383,7 @@ struct mbedtls_ssl_handshake_params
 #endif /* MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED */
 #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \
     defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED)
-    const mbedtls_ecp_curve_info **curves;      /*!<  Supported elliptic curves */
+    mbedtls_ecp_curve_info const *curve_info;  /*!< Info for EC for ECDHE. */
 #endif
 #if defined(MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED)
     unsigned char *psk;                 /*!<  PSK from the callback         */

library/ssl_srv.c

@@ -279,12 +279,11 @@ static int ssl_parse_signature_algorithms_ext( mbedtls_ssl_context *ssl,
 #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \
     defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED)
 static int ssl_parse_supported_elliptic_curves( mbedtls_ssl_context *ssl,
-                                                const unsigned char *buf,
+                        const unsigned char *buf, size_t len,
-                                                size_t len )
+                        mbedtls_ecp_group_id curve_ids[ MBEDTLS_ECP_DP_MAX ] )
 {
     size_t list_size, our_size;
     const unsigned char *p;
-    const mbedtls_ecp_curve_info *curve_info, **curves;
 
     if ( len < 2 ) {
         MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) );
@@ -302,44 +301,38 @@ static int ssl_parse_supported_elliptic_curves( mbedtls_ssl_context *ssl,
         return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO );
     }
 
-    /* Should never happen unless client duplicates the extension */
+    p = buf + 2;
-    if( ssl->handshake->curves != NULL )
-    {
-        MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) );
-        mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL,
-                                        MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR );
-        return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO );
-    }
-
-    /* Don't allow our peer to make us allocate too much memory,
-     * and leave room for a final 0 */
-    our_size = list_size / 2 + 1;
-    if( our_size > MBEDTLS_ECP_DP_MAX )
     our_size = MBEDTLS_ECP_DP_MAX;
 
-    if( ( curves = mbedtls_calloc( our_size, sizeof( *curves ) ) ) == NULL )
+    /* Leave room for final 0-entry */
-    {
-        mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL,
-                                        MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR );
-        return( MBEDTLS_ERR_SSL_ALLOC_FAILED );
-    }
-
-    ssl->handshake->curves = curves;
-
-    p = buf + 2;
     while( list_size > 0 && our_size > 1 )
     {
-        curve_info = mbedtls_ecp_curve_info_from_tls_id( ( p[0] << 8 ) | p[1] );
+        uint16_t const tls_id = ( p[0] << 8 ) | p[1];
+        mbedtls_ecp_curve_info const * const info =
+            mbedtls_ecp_curve_info_from_tls_id( tls_id );
 
-        if( curve_info != NULL )
+        if( info != NULL )
         {
-            *curves++ = curve_info;
+            mbedtls_ecp_group_id const *gid;
+            /* Remember the first curve that we also support. */
+            for( gid = ssl->conf->curve_list;
+                 *gid != MBEDTLS_ECP_DP_NONE; gid++ )
+            {
+                if( info->grp_id != *gid )
+                    continue;
+
+                if( ssl->handshake->curve_info == NULL )
+                    ssl->handshake->curve_info = info;
+            }
+
+            *curve_ids++ = info->grp_id;
             our_size--;
         }
 
         list_size -= 2;
         p += 2;
     }
+    *curve_ids = MBEDTLS_ECP_DP_NONE;
 
     return( 0 );
 }
@@ -743,16 +736,18 @@ static int ssl_parse_alpn_ext( mbedtls_ssl_context *ssl,
  */
 #if defined(MBEDTLS_ECDSA_C)
 static int ssl_check_key_curve( mbedtls_pk_context *pk,
-                                const mbedtls_ecp_curve_info **curves )
+                          mbedtls_ecp_group_id const *acceptable_ec_grp_ids )
 {
-    const mbedtls_ecp_curve_info **crv = curves;
     mbedtls_ecp_group_id grp_id = mbedtls_pk_ec( *pk )->grp.id;
 
-    while( *crv != NULL )
+    if( acceptable_ec_grp_ids == NULL )
+        return( -1 );
+
+    while( *acceptable_ec_grp_ids != MBEDTLS_ECP_DP_NONE )
     {
-        if( (*crv)->grp_id == grp_id )
+        if( *acceptable_ec_grp_ids == grp_id )
             return( 0 );
-        crv++;
+        acceptable_ec_grp_ids++;
     }
 
     return( -1 );
@@ -764,7 +759,9 @@ static int ssl_check_key_curve( mbedtls_pk_context *pk,
  * return 0 on success and -1 on failure.
  */
 static int ssl_pick_cert( mbedtls_ssl_context *ssl,
-                          const mbedtls_ssl_ciphersuite_t * ciphersuite_info )
+                          const mbedtls_ssl_ciphersuite_t * ciphersuite_info,
+                          mbedtls_ecp_group_id const *acceptable_ec_grp_ids )
+
 {
     mbedtls_ssl_key_cert *cur, *list, *fallback = NULL;
     mbedtls_pk_type_t pk_alg =
@@ -829,11 +826,13 @@ static int ssl_pick_cert( mbedtls_ssl_context *ssl,
 
 #if defined(MBEDTLS_ECDSA_C)
         if( pk_alg == MBEDTLS_PK_ECDSA &&
-            ssl_check_key_curve( pk, ssl->handshake->curves ) != 0 )
+            ssl_check_key_curve( pk, acceptable_ec_grp_ids ) != 0 )
         {
             MBEDTLS_SSL_DEBUG_MSG( 3, ( "certificate mismatch: elliptic curve" ) );
             match = 0;
         }
+#else
+        ((void) acceptable_ec_grp_ids);
 #endif
 
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
@@ -921,7 +920,8 @@ static int ssl_pick_cert( mbedtls_ssl_context *ssl,
  * Sets ciphersuite_info only if the suite matches.
  */
 static int ssl_ciphersuite_match( mbedtls_ssl_context *ssl, int suite_id,
-                                  const mbedtls_ssl_ciphersuite_t **ciphersuite_info )
+                           const mbedtls_ssl_ciphersuite_t **ciphersuite_info,
+                           mbedtls_ecp_group_id const *acceptable_ec_grp_ids )
 {
     const mbedtls_ssl_ciphersuite_t *suite_info;
 
@@ -971,11 +971,9 @@ static int ssl_ciphersuite_match( mbedtls_ssl_context *ssl, int suite_id,
     }
 #endif
 
-
 #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C)
     if( mbedtls_ssl_ciphersuite_uses_ec( suite_info ) &&
-        ( ssl->handshake->curves == NULL ||
+        ssl->handshake->curve_info == NULL )
-          ssl->handshake->curves[0] == NULL ) )
     {
         MBEDTLS_SSL_DEBUG_MSG( 3, ( "ciphersuite mismatch: "
                             "no common elliptic curve" ) );
@@ -1023,12 +1021,14 @@ static int ssl_ciphersuite_match( mbedtls_ssl_context *ssl, int suite_id,
      * - try the next ciphersuite if we don't
      * This must be done last since we modify the key_cert list.
      */
-    if( ssl_pick_cert( ssl, suite_info ) != 0 )
+    if( ssl_pick_cert( ssl, suite_info, acceptable_ec_grp_ids ) != 0 )
     {
         MBEDTLS_SSL_DEBUG_MSG( 3, ( "ciphersuite mismatch: "
                             "no suitable certificate" ) );
         return( 0 );
     }
+#else
+    ((void) acceptable_ec_grp_ids);
 #endif
 
     *ciphersuite_info = suite_info;
@@ -1252,8 +1252,11 @@ static int ssl_parse_client_hello_v2( mbedtls_ssl_context *ssl )
             got_common_suite = 1;
 
             if( ( ret = ssl_ciphersuite_match( ssl, ciphersuites[i],
-                                               &ciphersuite_info ) ) != 0 )
+                                               &ciphersuite_info,
+                                               NULL ) ) )
+            {
                 return( ret );
+            }
 
             if( ciphersuite_info != NULL )
                 goto have_ciphersuite_v2;
@@ -1323,6 +1326,13 @@ static int ssl_parse_client_hello( mbedtls_ssl_context *ssl )
     const mbedtls_ssl_ciphersuite_t *ciphersuite_info;
     int major, minor;
 
+#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \
+    defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED)
+    mbedtls_ecp_group_id acceptable_ec_grp_ids[ MBEDTLS_ECP_DP_MAX ];
+#else
+    mbedtls_ecp_group_id * acceptable_ec_grp_ids = NULL;
+#endif
+
     /* If there is no signature-algorithm extension present,
      * we need to fall back to the default values for allowed
      * signature-hash pairs. */
@@ -1862,7 +1872,9 @@ read_record_header:
             case MBEDTLS_TLS_EXT_SUPPORTED_ELLIPTIC_CURVES:
                 MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported elliptic curves extension" ) );
 
-                ret = ssl_parse_supported_elliptic_curves( ssl, ext + 4, ext_size );
+                ret = ssl_parse_supported_elliptic_curves( ssl, ext + 4,
+                                                      ext_size,
+                                                      acceptable_ec_grp_ids );
                 if( ret != 0 )
                     return( ret );
                 break;
@@ -2133,8 +2145,11 @@ read_record_header:
             got_common_suite = 1;
 
             if( ( ret = ssl_ciphersuite_match( ssl, ciphersuites[i],
-                                               &ciphersuite_info ) ) != 0 )
+                                               &ciphersuite_info,
+                                               acceptable_ec_grp_ids ) ) != 0 )
+            {
                 return( ret );
+            }
 
             if( ciphersuite_info != NULL )
                 goto have_ciphersuite;
@@ -3241,28 +3256,19 @@ static int ssl_prepare_server_key_exchange( mbedtls_ssl_context *ssl,
          *     ECPoint      public;
          * } ServerECDHParams;
          */
-        const mbedtls_ecp_curve_info **curve = NULL;
+        const mbedtls_ecp_curve_info *curve = ssl->handshake->curve_info;
-        const mbedtls_ecp_group_id *gid;
         int ret;
         size_t len = 0;
 
-        /* Match our preference list against the offered curves */
+        if( curve == NULL )
-        for( gid = ssl->conf->curve_list; *gid != MBEDTLS_ECP_DP_NONE; gid++ )
-            for( curve = ssl->handshake->curves; *curve != NULL; curve++ )
-                if( (*curve)->grp_id == *gid )
-                    goto curve_matching_done;
-
-curve_matching_done:
-        if( curve == NULL || *curve == NULL )
         {
             MBEDTLS_SSL_DEBUG_MSG( 1, ( "no matching curve for ECDHE" ) );
             return( MBEDTLS_ERR_SSL_NO_CIPHER_CHOSEN );
         }
-
+        MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDHE curve: %s", curve->name ) );
-        MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDHE curve: %s", (*curve)->name ) );
 
         if( ( ret = mbedtls_ecdh_setup( &ssl->handshake->ecdh_ctx,
-                                        (*curve)->grp_id ) ) != 0 )
+                                        curve->grp_id ) ) != 0 )
         {
             MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ecp_group_load", ret );
             return( ret );

library/ssl_tls.c

@@ -10546,12 +10546,6 @@ void mbedtls_ssl_handshake_free( mbedtls_ssl_context *ssl )
 #endif
 #endif
 
-#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \
-    defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED)
-    /* explicit void pointer cast for buggy MS compiler */
-    mbedtls_free( (void *) handshake->curves );
-#endif
-
 #if defined(MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED)
     if( handshake->psk != NULL )
     {