Merge remote-tracking branch 'restricted/pr/552' into development

Ensure this merge passes tests by auto-generating query_config.c, adding
MBEDTLS_ECDH_LEGACY_CONTEXT to it.

* restricted/pr/552:
  Fix mbedtls_ecdh_get_params with new ECDH context
  Test undefining MBEDTLS_ECDH_LEGACY_CONTEXT in all.sh
  Define MBEDTLS_ECDH_LEGACY_CONTEXT in config.h
  Add changelog entry for mbedtls_ecdh_get_params robustness
  Fix ecdh_get_params with mismatching group
  Add test case for ecdh_get_params with mismatching group
  Add test case for ecdh_calc_secret
  Fix typo in documentation

Author: Patater

ChangeLog

@@ -50,6 +50,14 @@ API Changes
      mbedtls_ssl_session structure which otherwise stores the peer's
      certificate.
 
+Security
+   * Make mbedtls_ecdh_get_params return an error if the second key
+     belongs to a different group from the first. Before, if an application
+     passed keys that belonged to different group, the first key's data was
+     interpreted according to the second group, which could lead to either
+     an error or a meaningless output from mbedtls_ecdh_get_params. In the
+     latter case, this could expose at most 5 bits of the private key.
+
 Bugfix
    * Fix a compilation issue with mbedtls_ecp_restart_ctx not being defined
      when MBEDTLS_ECP_ALT is defined. Reported by jwhui. Fixes #2242.

include/mbedtls/check_config.h

@@ -125,6 +125,11 @@
 #error "MBEDTLS_ECP_RESTARTABLE defined, but it cannot coexist with an alternative or PSA-based ECP implementation"
 #endif
 
+#if defined(MBEDTLS_ECP_RESTARTABLE)           && \
+    ! defined(MBEDTLS_ECDH_LEGACY_CONTEXT)
+#error "MBEDTLS_ECP_RESTARTABLE defined, but not MBEDTLS_ECDH_LEGACY_CONTEXT"
+#endif
+
 #if defined(MBEDTLS_ECDSA_DETERMINISTIC) && !defined(MBEDTLS_HMAC_DRBG_C)
 #error "MBEDTLS_ECDSA_DETERMINISTIC defined, but not all prerequisites"
 #endif

include/mbedtls/config.h

@@ -760,10 +760,39 @@
  *
  * \note  This option only works with the default software implementation of
  *        elliptic curve functionality. It is incompatible with
- *        MBEDTLS_ECP_ALT, MBEDTLS_ECDH_XXX_ALT and MBEDTLS_ECDSA_XXX_ALT.
+ *        MBEDTLS_ECP_ALT, MBEDTLS_ECDH_XXX_ALT, MBEDTLS_ECDSA_XXX_ALT
+ *        and MBEDTLS_ECDH_LEGACY_CONTEXT.
  */
 //#define MBEDTLS_ECP_RESTARTABLE
 
+/**
+ * \def MBEDTLS_ECDH_LEGACY_CONTEXT
+ *
+ * Use a backward compatible ECDH context.
+ *
+ * Mbed TLS supports two formats for ECDH contexts (#mbedtls_ecdh_context
+ * defined in `ecdh.h`). For most applications, the choice of format makes
+ * no difference, since all library functions can work with either format,
+ * except that the new format is incompatible with MBEDTLS_ECP_RESTARTABLE.
+
+ * The new format used when this option is disabled is smaller
+ * (56 bytes on a 32-bit platform). In future versions of the library, it
+ * will support alternative implementations of ECDH operations.
+ * The new format is incompatible with applications that access
+ * context fields directly and with restartable ECP operations.
+ *
+ * Define this macro if you enable MBEDTLS_ECP_RESTARTABLE or if you
+ * want to access ECDH context fields directly. Otherwise you should
+ * comment out this macro definition.
+ *
+ * This option has no effect if #MBEDTLS_ECDH_C is not enabled.
+ *
+ * \note This configuration option is experimental. Future versions of the
+ *       library may modify the way the ECDH context layout is configured
+ *       and may modify the layout of the new context type.
+ */
+#define MBEDTLS_ECDH_LEGACY_CONTEXT
+
 /**
  * \def MBEDTLS_ECDSA_DETERMINISTIC
  *

include/mbedtls/ecdh.h

@@ -42,18 +42,6 @@
 
 #include "ecp.h"
 
-/*
- * Use a backward compatible ECDH context.
- *
- * This flag is always enabled for now and future versions might add a
- * configuration option that conditionally undefines this flag.
- * The configuration option in question may have a different name.
- *
- * Features undefining this flag, must have a warning in their description in
- * config.h stating that the feature breaks backward compatibility.
- */
-#define MBEDTLS_ECDH_LEGACY_CONTEXT
-
 #ifdef __cplusplus
 extern "C" {
 #endif

include/mbedtls/ecp.h

@@ -482,7 +482,7 @@ void mbedtls_ecp_point_init( mbedtls_ecp_point *pt );
  *
  * \note            After this function is called, domain parameters
  *                  for various ECP groups can be loaded through the
- *                  mbedtls_ecp_load() or mbedtls_ecp_tls_read_group()
+ *                  mbedtls_ecp_group_load() or mbedtls_ecp_tls_read_group()
  *                  functions.
  */
 void mbedtls_ecp_group_init( mbedtls_ecp_group *grp );

library/ecdh.c

@@ -49,6 +49,16 @@
 typedef mbedtls_ecdh_context mbedtls_ecdh_context_mbed;
 #endif
 
+static mbedtls_ecp_group_id mbedtls_ecdh_grp_id(
+    const mbedtls_ecdh_context *ctx )
+{
+#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT)
+    return( ctx->grp.id );
+#else
+    return( ctx->grp_id );
+#endif
+}
+
 #if !defined(MBEDTLS_ECDH_GEN_PUBLIC_ALT)
 /*
  * Generate public key (restartable version)
@@ -442,8 +452,21 @@ int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx,
     ECDH_VALIDATE_RET( side == MBEDTLS_ECDH_OURS ||
                        side == MBEDTLS_ECDH_THEIRS );
 
-    if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 )
-        return( ret );
+    if( mbedtls_ecdh_grp_id( ctx ) == MBEDTLS_ECP_DP_NONE )
+    {
+        /* This is the first call to get_params(). Set up the context
+         * for use with the group. */
+        if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 )
+            return( ret );
+    }
+    else
+    {
+        /* This is not the first call to get_params(). Check that the
+         * current key's group is the same as the context's, which was set
+         * from the first key's group. */
+        if( mbedtls_ecdh_grp_id( ctx ) != key->grp.id )
+            return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA );
+    }
 
 #if defined(MBEDTLS_ECDH_LEGACY_CONTEXT)
     return( ecdh_get_params_internal( ctx, key, side ) );

library/version_features.c

@@ -351,6 +351,9 @@ static const char *features[] = {
 #if defined(MBEDTLS_ECP_RESTARTABLE)
     "MBEDTLS_ECP_RESTARTABLE",
 #endif /* MBEDTLS_ECP_RESTARTABLE */
+#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT)
+    "MBEDTLS_ECDH_LEGACY_CONTEXT",
+#endif /* MBEDTLS_ECDH_LEGACY_CONTEXT */
 #if defined(MBEDTLS_ECDSA_DETERMINISTIC)
     "MBEDTLS_ECDSA_DETERMINISTIC",
 #endif /* MBEDTLS_ECDSA_DETERMINISTIC */

programs/ssl/query_config.c

@@ -978,6 +978,14 @@ int query_config( const char *config )
     }
 #endif /* MBEDTLS_ECP_RESTARTABLE */
 
+#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT)
+    if( strcmp( "MBEDTLS_ECDH_LEGACY_CONTEXT", config ) == 0 )
+    {
+        MACRO_EXPANSION_TO_STR( MBEDTLS_ECDH_LEGACY_CONTEXT );
+        return( 0 );
+    }
+#endif /* MBEDTLS_ECDH_LEGACY_CONTEXT */
+
 #if defined(MBEDTLS_ECDSA_DETERMINISTIC)
     if( strcmp( "MBEDTLS_ECDSA_DETERMINISTIC", config ) == 0 )
     {

tests/scripts/all.sh

@@ -678,6 +678,23 @@ component_test_rsa_no_crt () {
     if_build_succeeded tests/compat.sh -t RSA
 }
 
+component_test_new_ecdh_context () {
+    msg "build: new ECDH context (ASan build)" # ~ 6 min
+    scripts/config.pl unset MBEDTLS_ECDH_LEGACY_CONTEXT
+    CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan .
+    make
+
+    msg "test: new ECDH context - main suites (inc. selftests) (ASan build)" # ~ 50s
+    make test
+
+    msg "test: new ECDH context - ECDH-related part of ssl-opt.sh (ASan build)" # ~ 5s
+    if_build_succeeded tests/ssl-opt.sh -f ECDH
+
+    msg "test: new ECDH context - compat.sh with some ECDH ciphersuites (ASan build)" # ~ 3 min
+    # Exclude some symmetric ciphers that are redundant here to gain time.
+    if_build_succeeded tests/compat.sh -f ECDH -V NO -e 'ARCFOUR\|ARIA\|CAMELLIA\|CHACHA\|DES\|RC4'
+}
+
 component_test_small_ssl_out_content_len () {
     msg "build: small SSL_OUT_CONTENT_LEN (ASan build)"
     scripts/config.pl set MBEDTLS_SSL_IN_CONTENT_LEN 16384

tests/suites/test_suite_ecdh.data

@@ -79,3 +79,19 @@ ecdh_restart:MBEDTLS_ECP_DP_SECP256R1:"C88F01F510D9AC3F70A292DAA2316DE544E9AAB8A
 ECDH exchange legacy context
 depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED
 ecdh_exchange_legacy:MBEDTLS_ECP_DP_SECP192R1
+
+ECDH calc_secret: ours first, SECP256R1 (RFC 5903)
+depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED
+ecdh_exchange_calc_secret:MBEDTLS_ECP_DP_SECP256R1:"c6ef9c5d78ae012a011164acb397ce2088685d8f06bf9be0b283ab46476bee53":"04dad0b65394221cf9b051e1feca5787d098dfe637fc90b9ef945d0c37725811805271a0461cdb8252d61f1c456fa3e59ab1f45b33accf5f58389e0577b8990bb3":0:"d6840f6b42f6edafd13116e0e12565202fef8e9ece7dce03812464d04b9442de"
+
+ECDH calc_secret: theirs first, SECP256R1 (RFC 5903)
+depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED
+ecdh_exchange_calc_secret:MBEDTLS_ECP_DP_SECP256R1:"c6ef9c5d78ae012a011164acb397ce2088685d8f06bf9be0b283ab46476bee53":"04dad0b65394221cf9b051e1feca5787d098dfe637fc90b9ef945d0c37725811805271a0461cdb8252d61f1c456fa3e59ab1f45b33accf5f58389e0577b8990bb3":1:"d6840f6b42f6edafd13116e0e12565202fef8e9ece7dce03812464d04b9442de"
+
+ECDH get_params with mismatched groups: our BP256R1, their SECP256R1
+depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECP_DP_BP256R1_ENABLED
+ecdh_exchange_get_params_fail:MBEDTLS_ECP_DP_BP256R1:"1234567812345678123456781234567812345678123456781234567812345678":MBEDTLS_ECP_DP_SECP256R1:"04dad0b65394221cf9b051e1feca5787d098dfe637fc90b9ef945d0c37725811805271a0461cdb8252d61f1c456fa3e59ab1f45b33accf5f58389e0577b8990bb3":0:MBEDTLS_ERR_ECP_BAD_INPUT_DATA
+
+ECDH get_params with mismatched groups: their SECP256R1, our BP256R1
+depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECP_DP_BP256R1_ENABLED
+ecdh_exchange_get_params_fail:MBEDTLS_ECP_DP_BP256R1:"1234567812345678123456781234567812345678123456781234567812345678":MBEDTLS_ECP_DP_SECP256R1:"04dad0b65394221cf9b051e1feca5787d098dfe637fc90b9ef945d0c37725811805271a0461cdb8252d61f1c456fa3e59ab1f45b33accf5f58389e0577b8990bb3":1:MBEDTLS_ERR_ECP_BAD_INPUT_DATA

tests/suites/test_suite_ecdh.function

@@ -1,5 +1,41 @@
 /* BEGIN_HEADER */
 #include "mbedtls/ecdh.h"
+
+static int load_public_key( int grp_id, data_t *point,
+                            mbedtls_ecp_keypair *ecp )
+{
+    int ok = 0;
+    TEST_ASSERT( mbedtls_ecp_group_load( &ecp->grp, grp_id ) == 0 );
+    TEST_ASSERT( mbedtls_ecp_point_read_binary( &ecp->grp,
+                                                &ecp->Q,
+                                                point->x,
+                                                point->len ) == 0 );
+    TEST_ASSERT( mbedtls_ecp_check_pubkey( &ecp->grp,
+                                           &ecp->Q ) == 0 );
+    ok = 1;
+exit:
+    return( ok );
+}
+
+static int load_private_key( int grp_id, data_t *private_key,
+                             mbedtls_ecp_keypair *ecp,
+                             rnd_pseudo_info *rnd_info )
+{
+    int ok = 0;
+    TEST_ASSERT( mbedtls_ecp_group_load( &ecp->grp, grp_id ) == 0 );
+    TEST_ASSERT( mbedtls_mpi_read_binary( &ecp->d,
+                                          private_key->x,
+                                          private_key->len ) == 0 );
+    TEST_ASSERT( mbedtls_ecp_check_privkey( &ecp->grp, &ecp->d ) == 0 );
+    /* Calculate the public key from the private key. */
+    TEST_ASSERT( mbedtls_ecp_mul( &ecp->grp, &ecp->Q, &ecp->d,
+                                  &ecp->grp.G,
+                                  &rnd_pseudo_rand, rnd_info ) == 0 );
+    ok = 1;
+exit:
+    return( ok );
+}
+
 /* END_HEADER */
 
 /* BEGIN_DEPENDENCIES
@@ -464,3 +500,107 @@ exit:
     mbedtls_ecdh_free( &cli );
 }
 /* END_CASE */
+
+/* BEGIN_CASE */
+void ecdh_exchange_calc_secret( int grp_id,
+                                data_t *our_private_key,
+                                data_t *their_point,
+                                int ours_first,
+                                data_t *expected )
+{
+    rnd_pseudo_info rnd_info;
+    mbedtls_ecp_keypair our_key;
+    mbedtls_ecp_keypair their_key;
+    mbedtls_ecdh_context ecdh;
+    unsigned char shared_secret[MBEDTLS_ECP_MAX_BYTES];
+    size_t shared_secret_length = 0;
+
+    memset( &rnd_info, 0x00, sizeof( rnd_pseudo_info ) );
+    mbedtls_ecdh_init( &ecdh );
+    mbedtls_ecp_keypair_init( &our_key );
+    mbedtls_ecp_keypair_init( &their_key );
+
+    if( ! load_private_key( grp_id, our_private_key, &our_key, &rnd_info ) )
+        goto exit;
+    if( ! load_public_key( grp_id, their_point, &their_key ) )
+        goto exit;
+
+    /* Import the keys to the ECDH calculation. */
+    if( ours_first )
+    {
+        TEST_ASSERT( mbedtls_ecdh_get_params(
+                         &ecdh, &our_key, MBEDTLS_ECDH_OURS ) == 0 );
+        TEST_ASSERT( mbedtls_ecdh_get_params(
+                         &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) == 0 );
+    }
+    else
+    {
+        TEST_ASSERT( mbedtls_ecdh_get_params(
+                         &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) == 0 );
+        TEST_ASSERT( mbedtls_ecdh_get_params(
+                         &ecdh, &our_key, MBEDTLS_ECDH_OURS ) == 0 );
+    }
+
+    /* Perform the ECDH calculation. */
+    TEST_ASSERT( mbedtls_ecdh_calc_secret(
+                     &ecdh,
+                     &shared_secret_length,
+                     shared_secret, sizeof( shared_secret ),
+                     &rnd_pseudo_rand, &rnd_info ) == 0 );
+    TEST_ASSERT( shared_secret_length == expected->len );
+    TEST_ASSERT( memcmp( expected->x, shared_secret,
+                         shared_secret_length ) == 0 );
+
+exit:
+    mbedtls_ecdh_free( &ecdh );
+    mbedtls_ecp_keypair_free( &our_key );
+    mbedtls_ecp_keypair_free( &their_key );
+}
+/* END_CASE */
+
+/* BEGIN_CASE */
+void ecdh_exchange_get_params_fail( int our_grp_id,
+                                    data_t *our_private_key,
+                                    int their_grp_id,
+                                    data_t *their_point,
+                                    int ours_first,
+                                    int expected_ret )
+{
+    rnd_pseudo_info rnd_info;
+    mbedtls_ecp_keypair our_key;
+    mbedtls_ecp_keypair their_key;
+    mbedtls_ecdh_context ecdh;
+
+    memset( &rnd_info, 0x00, sizeof( rnd_pseudo_info ) );
+    mbedtls_ecdh_init( &ecdh );
+    mbedtls_ecp_keypair_init( &our_key );
+    mbedtls_ecp_keypair_init( &their_key );
+
+    if( ! load_private_key( our_grp_id, our_private_key, &our_key, &rnd_info ) )
+        goto exit;
+    if( ! load_public_key( their_grp_id, their_point, &their_key ) )
+        goto exit;
+
+    if( ours_first )
+    {
+        TEST_ASSERT( mbedtls_ecdh_get_params(
+                         &ecdh, &our_key, MBEDTLS_ECDH_OURS ) == 0 );
+        TEST_ASSERT( mbedtls_ecdh_get_params(
+                         &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) ==
+                     expected_ret );
+    }
+    else
+    {
+        TEST_ASSERT( mbedtls_ecdh_get_params(
+                         &ecdh, &their_key, MBEDTLS_ECDH_THEIRS ) == 0 );
+        TEST_ASSERT( mbedtls_ecdh_get_params(
+                         &ecdh, &our_key, MBEDTLS_ECDH_OURS ) ==
+                     expected_ret );
+    }
+
+exit:
+    mbedtls_ecdh_free( &ecdh );
+    mbedtls_ecp_keypair_free( &our_key );
+    mbedtls_ecp_keypair_free( &their_key );
+}
+/* END_CASE */