From 2b7ad6472bb9f6c5e4df300ae8482936dbc11c34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 6 Dec 2022 10:42:44 +0100 Subject: [PATCH 01/13] Document all effects of MBEDTLS_ECP_RESTARTABLE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It might not be obvious that this option goes beyond adding new functions, but also automagically modifies the behaviour of TLS in some circumstances. Moreover, the exact modifications and circumstances were not documented anywhere outside the ChangeLog. Fix that. While at it, adjust the test that checks no restartable behaviour with other key exchanges, to use a key exchange that allows cert-based client authentication so that we can check that this is not restartable either. We don't have any automated test checking that the server is never affected. That would require adding an ec_max_ops command-line option to ssl_server2 that never has any effect, just to check that it indeed doesn't. I'm not sure that's worth it. I tested manually and could confirm that the server never has restartable behaviour, even for the parts that are shared between client and server such as cert chain verification. Note (from re-reading the code): all restartable behaviour is controlled by the flag ssl->handshake->ecrs_enabled which is only client-side with the ECDHE-ECDSA key exchange (TLS 1.2). Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/mbedtls_config.h | 26 ++++++++++++++++++++++++-- tests/ssl-opt.sh | 14 ++++++++++---- 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 826ab64594..b5f0bf2b4e 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -690,11 +690,33 @@ * This is useful in non-threaded environments if you want to avoid blocking * for too long on ECC (and, hence, X.509 or SSL/TLS) operations. * - * Uncomment this macro to enable restartable ECC computations. + * This option: + * - Adds xxx_restartable() variants of existing operations in the + * following modules, with corresponding restart context types: + * - ECP: scalar multiplication (mult), linear combination (muladd); + * - ECDSA: signature generation & verification; + * - PK: signature generation & verification; + * - X509: certificate chain verification. + * - Adds mbedtls_ecdh_enable_restart() in the ECDH module. + * - Changes the behaviour of TLS 1.2 clients (not servers) when using the + * ECDHE-ECDSA key exchange (not other key exchanges) to make all ECC + * computations restartable: + * - ECDH operations from the key exchange; + * - verification of the server's key exchange signature; + * - verification of the server's certificate chain; + * - generation of our signature if client authentication is used, with an + * ECC key/certificate. + * + * \note In the cases above, the usual SSL/TLS functions, such as + * mbedtls_ssl_handshake(), can now return + * MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS. * * \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, MBEDTLS_ECDSA_XXX_ALT. + * MBEDTLS_ECP_ALT, MBEDTLS_ECDH_XXX_ALT, MBEDTLS_ECDSA_XXX_ALT, + * and MBEDTLS_USE_PSA_CRYPTO. + * + * Uncomment this macro to enable restartable ECC computations. */ //#define MBEDTLS_ECP_RESTARTABLE diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 1fe8baeb65..154ba348ef 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8493,13 +8493,19 @@ run_test "EC restart: TLS, max_ops=1000 no client auth" \ -c "mbedtls_ecdh_make_public.*4b00" \ -C "mbedtls_pk_sign.*4b00" + +# Restartable is only for ECDHE-ECDSA, with another ciphersuite we expect no +# restartable behaviour at all (not even client auth). +# This is the same as "EC restart: TLS, max_ops=1000" except with ECDHE-RSA, +# and all 4 assertions negated. requires_config_enabled MBEDTLS_ECP_RESTARTABLE requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 -run_test "EC restart: TLS, max_ops=1000, ECDHE-PSK" \ - "$P_SRV curves=secp256r1 psk=abc123" \ - "$P_CLI force_ciphersuite=TLS-ECDHE-PSK-WITH-AES-128-CBC-SHA256 \ - psk=abc123 debug_level=1 ec_max_ops=1000" \ +run_test "EC restart: TLS, max_ops=1000, ECDHE-RSA" \ + "$P_SRV curves=secp256r1 auth_mode=required" \ + "$P_CLI force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-128-GCM-SHA256 \ + key_file=data_files/server5.key crt_file=data_files/server5.crt \ + debug_level=1 ec_max_ops=1000" \ 0 \ -C "x509_verify_cert.*4b00" \ -C "mbedtls_pk_verify.*4b00" \ From ad27b8074f4196102291645d6aacaa96e21a4dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 Dec 2022 12:54:11 +0100 Subject: [PATCH 02/13] Declare ECP_RESTARTABLE and USE_PSA compatible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is only the beginning: - some test failures in test_suite_pk, test_suite_x509 and ssl-opt.sh will be fixed in the next few commits; - then the interactions between those options will be documented and tested. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/check_config.h | 5 ++--- include/mbedtls/mbedtls_config.h | 3 ++- scripts/config.py | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 7f55580878..e2f8e62700 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -114,15 +114,14 @@ #endif #if defined(MBEDTLS_ECP_RESTARTABLE) && \ - ( defined(MBEDTLS_USE_PSA_CRYPTO) || \ - defined(MBEDTLS_ECDH_COMPUTE_SHARED_ALT) || \ + ( defined(MBEDTLS_ECDH_COMPUTE_SHARED_ALT) || \ defined(MBEDTLS_ECDH_GEN_PUBLIC_ALT) || \ defined(MBEDTLS_ECDSA_SIGN_ALT) || \ defined(MBEDTLS_ECDSA_VERIFY_ALT) || \ defined(MBEDTLS_ECDSA_GENKEY_ALT) || \ defined(MBEDTLS_ECP_INTERNAL_ALT) || \ defined(MBEDTLS_ECP_ALT) ) -#error "MBEDTLS_ECP_RESTARTABLE defined, but it cannot coexist with an alternative or PSA-based ECP implementation" +#error "MBEDTLS_ECP_RESTARTABLE defined, but it cannot coexist with an alternative ECP implementation" #endif #if defined(MBEDTLS_ECDSA_DETERMINISTIC) && !defined(MBEDTLS_HMAC_DRBG_C) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index b5f0bf2b4e..219dd4539e 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -707,6 +707,8 @@ * - generation of our signature if client authentication is used, with an * ECC key/certificate. * + * TODO: document interation with USE_PSA_CRYPTO + * * \note In the cases above, the usual SSL/TLS functions, such as * mbedtls_ssl_handshake(), can now return * MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS. @@ -1928,7 +1930,6 @@ * before calling any function from the SSL/TLS, X.509 or PK modules. * * Requires: MBEDTLS_PSA_CRYPTO_C. - * Conflicts with: MBEDTLS_ECP_RESTARTABLE * * Uncomment this to enable internal use of PSA Crypto and new associated APIs. */ diff --git a/scripts/config.py b/scripts/config.py index 7e58acd0a4..a53c470f07 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -194,7 +194,6 @@ EXCLUDE_FROM_FULL = frozenset([ 'MBEDTLS_DEPRECATED_WARNING', # conflicts with deprecated options 'MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED', # influences the use of ECDH in TLS 'MBEDTLS_ECP_NO_FALLBACK', # removes internal ECP implementation - 'MBEDTLS_ECP_RESTARTABLE', # incompatible with USE_PSA_CRYPTO 'MBEDTLS_ENTROPY_FORCE_SHA256', # interacts with CTR_DRBG_128_BIT_KEY 'MBEDTLS_HAVE_SSE2', # hardware dependency 'MBEDTLS_MEMORY_BACKTRACE', # depends on MEMORY_BUFFER_ALLOC_C From 79ae7eb4d1bd58f9d871cf665722a57e47f384e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 Dec 2022 12:55:51 +0100 Subject: [PATCH 03/13] Use deterministic ECDSA in PSA when we do in legacy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the two failing cases in test_suite_pk when ECP_RESTARTABLE and USE_PSA_CRYPTO are both enabled. The two failing cases where ECDSA restartable sign/verify: ECDSA, max_ops=0 (disabled) ECDSA restartable sign/verify: ECKEY, max_ops=0 (disabled) associated with test function pk_sign_verify_restart(). The failure was caused by the interaction of several things that are each reasonable on their own: 1. The test function relies on ECDSA restartable, which is reasonable as it allows making sure that the generated signature is correct with a simple memcmp(). 2. The implementation of pk_sign_restartable() has a shortcut to dispatch to the sign function (as opposed to sign_restartable) when restart is disabled (max_ops == 0). 3. When USE_PSA is enabled, the sign function dispatches to PSA, which so far always used ECDSA (non-deterministic) even when the non-PSA version would use deterministic ECDSA. This could be fixed by changing any of those. I chose (3) because I think it makes sense that when PK dispatches to PSA instead of legacy this should not change which version of ECDSA is selected. OTOH, I think it makes sense to keep (2), because that means more opportunities to dispatch to PSA. Signed-off-by: Manuel Pégourié-Gonnard --- library/pk_wrap.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/pk_wrap.c b/library/pk_wrap.c index 5de8fa65f7..00abffb2df 100644 --- a/library/pk_wrap.c +++ b/library/pk_wrap.c @@ -1162,8 +1162,13 @@ static int ecdsa_sign_wrap( void *ctx_arg, mbedtls_md_type_t md_alg, size_t key_len; unsigned char buf[MBEDTLS_PK_ECP_PRV_DER_MAX_BYTES]; unsigned char *p; +#if defined(MBEDTLS_ECDSA_DETERMINISTIC) + psa_algorithm_t psa_sig_md = + PSA_ALG_DETERMINISTIC_ECDSA( mbedtls_hash_info_psa_from_md( md_alg ) ); +#else psa_algorithm_t psa_sig_md = PSA_ALG_ECDSA( mbedtls_hash_info_psa_from_md( md_alg ) ); +#endif size_t curve_bits; psa_ecc_family_t curve = mbedtls_ecc_group_to_psa( ctx->grp.id, &curve_bits ); From cc6e0a650fa94a733d93237869c26450a9dd2680 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 Dec 2022 12:55:05 +0100 Subject: [PATCH 04/13] Fix missing initialisation of PSA Crypto MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the two failures in test_suite_x509parse when both ECP_RESTARTABLE and USE_PSA_CRYPTO are enabled. The failure happened because the operation is dispatched to PSA when restart is disabled (max_ops == 0). Previously it was correct for this test function not to initialize PSA, because it depends on ECP_RESTARTABLE which used to conflict with USE_PSA_CRYPTO, but that's no longer the case. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509parse.function | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 2585720ed5..dc36b81665 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -579,6 +579,8 @@ void x509_verify_restart( char *crt_file, char *ca_file, mbedtls_x509_crt_init( &crt ); mbedtls_x509_crt_init( &ca ); + USE_PSA_INIT( ); + TEST_ASSERT( mbedtls_x509_crt_parse_file( &crt, crt_file ) == 0 ); TEST_ASSERT( mbedtls_x509_crt_parse_file( &ca, ca_file ) == 0 ); @@ -607,6 +609,7 @@ exit: mbedtls_x509_crt_restart_free( &rs_ctx ); mbedtls_x509_crt_free( &crt ); mbedtls_x509_crt_free( &ca ); + USE_PSA_DONE( ); } /* END_CASE */ From 55a188b420a16cfcb3409fa94945c446cb3e56fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 6 Dec 2022 12:00:33 +0100 Subject: [PATCH 05/13] Clarify the "restart vs use PSA" situation in TLS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- .../psa-migration/psa-limitations.md | 7 +- docs/use-psa-crypto.md | 21 ++-- include/mbedtls/mbedtls_config.h | 13 +- tests/ssl-opt.sh | 116 +++++++++++++++++- 4 files changed, 136 insertions(+), 21 deletions(-) diff --git a/docs/architecture/psa-migration/psa-limitations.md b/docs/architecture/psa-migration/psa-limitations.md index e565b283e9..c3680231d5 100644 --- a/docs/architecture/psa-migration/psa-limitations.md +++ b/docs/architecture/psa-migration/psa-limitations.md @@ -17,8 +17,11 @@ Restartable ECC operations There is currently no support for that in PSA at all, but it will be added at some point, see . -Currently, `MBEDTLS_USE_PSA_CRYPTO` is simply incompatible with -`MBEDTLS_ECP_RESTARTABLE`. +Currently, when `MBEDTLS_USE_PSA_CRYPTO` and `MBEDTLS_ECP_RESTARTABLE` are +both enabled, some operations that should be restartable are not (ECDH in TLS +1.2 clients using ECDHE-ECDSA), as they are using PSA instead, and some +operations that should use PSA do not (signature generation & verification) as +they use the legacy API instead, in order to get restartable behaviour. Things that are in the API but not implemented yet -------------------------------------------------- diff --git a/docs/use-psa-crypto.md b/docs/use-psa-crypto.md index 11442ed66d..194d96fb4e 100644 --- a/docs/use-psa-crypto.md +++ b/docs/use-psa-crypto.md @@ -7,9 +7,6 @@ operations, and enables new APIs for using keys handled by PSA Crypto. General considerations ---------------------- -**Compile-time:** enabling `MBEDTLS_USE_PSA_CRYPTO` requires -`MBEDTLS_ECP_RESTARTABLE` to be disabled. - **Application code:** when this option is enabled, you need to call `psa_crypto_init()` before calling any function from the SSL/TLS, X.509 or PK module. @@ -86,28 +83,34 @@ is enabled, no change required on the application side. Current exceptions: -- finite-field (non-EC) Diffie-Hellman (used in key exchanges: DHE-RSA, - DHE-PSK) +- Finite-field (non-EC) Diffie-Hellman (used in key exchanges: DHE-RSA, + DHE-PSK). +- Restartable operations when `MBEDTLS_ECP_RESTARTABLE` is also enabled (see + the documentation of that option). Other than the above exceptions, all crypto operations are based on PSA when `MBEDTLS_USE_PSA_CRYPTO` is enabled. ### X.509: most crypto operations based on PSA -Current exception: +Current exceptions: -- verification of RSA-PSS signatures with a salt length that is different from +- Verification of RSA-PSS signatures with a salt length that is different from the hash length. +- Restartable operations when `MBEDTLS_ECP_RESTARTABLE` is also enabled (see + the documentation of that option). Other than the above exception, all crypto operations are based on PSA when `MBEDTLS_USE_PSA_CRYPTO` is enabled. ### PK layer: most crypto operations based on PSA -Current exception: +Current exceptions: -- verification of RSA-PSS signatures with a salt length that is different from +- Verification of RSA-PSS signatures with a salt length that is different from the hash length, or with an MGF hash that's different from the message hash. +- Restartable operations when `MBEDTLS_ECP_RESTARTABLE` is also enabled (see + the documentation of that option). Other than the above exception, all crypto operations are based on PSA when `MBEDTLS_USE_PSA_CRYPTO` is enabled. diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 219dd4539e..7a681d1f2d 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -701,22 +701,25 @@ * - Changes the behaviour of TLS 1.2 clients (not servers) when using the * ECDHE-ECDSA key exchange (not other key exchanges) to make all ECC * computations restartable: - * - ECDH operations from the key exchange; + * - ECDH operations from the key exchange - unless MBEDTLS_USE_PSA_CRYPTO + * is also enabled. * - verification of the server's key exchange signature; * - verification of the server's certificate chain; * - generation of our signature if client authentication is used, with an * ECC key/certificate. * - * TODO: document interation with USE_PSA_CRYPTO - * * \note In the cases above, the usual SSL/TLS functions, such as * mbedtls_ssl_handshake(), can now return * MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS. * + * \note When this option and MBEDTLS_USE_PSA_CRYPTO are both enabled, + * restartable operations in PK, X.509 and TLS (see above) are not + * using PSA. On the other hand, ECDH computations in TLS are using + * PSA, and are not restartable. + * * \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, MBEDTLS_ECDSA_XXX_ALT, - * and MBEDTLS_USE_PSA_CRYPTO. + * MBEDTLS_ECP_ALT, MBEDTLS_ECDH_XXX_ALT, MBEDTLS_ECDSA_XXX_ALT. * * Uncomment this macro to enable restartable ECC computations. */ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 154ba348ef..b9b2bc5cc9 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -8395,10 +8395,12 @@ run_test "EC restart: TLS, max_ops=65535" \ -C "mbedtls_ecdh_make_public.*4b00" \ -C "mbedtls_pk_sign.*4b00" +# With USE_PSA disabled we expect full restartable behaviour. requires_config_enabled MBEDTLS_ECP_RESTARTABLE requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 -run_test "EC restart: TLS, max_ops=1000" \ +requires_config_disabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: TLS, max_ops=1000 (no USE_PSA)" \ "$P_SRV curves=secp256r1 auth_mode=required" \ "$P_CLI force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256 \ key_file=data_files/server5.key crt_file=data_files/server5.crt \ @@ -8409,6 +8411,25 @@ run_test "EC restart: TLS, max_ops=1000" \ -c "mbedtls_ecdh_make_public.*4b00" \ -c "mbedtls_pk_sign.*4b00" +# With USE_PSA enabled we expect only partial restartable behaviour: +# everything except ECDH (where TLS calls PSA directly). +requires_config_enabled MBEDTLS_ECP_RESTARTABLE +requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_config_enabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: TLS, max_ops=1000 (USE_PSA)" \ + "$P_SRV curves=secp256r1 auth_mode=required" \ + "$P_CLI force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256 \ + key_file=data_files/server5.key crt_file=data_files/server5.crt \ + debug_level=1 ec_max_ops=1000" \ + 0 \ + -c "x509_verify_cert.*4b00" \ + -c "mbedtls_pk_verify.*4b00" \ + -C "mbedtls_ecdh_make_public.*4b00" \ + -c "mbedtls_pk_sign.*4b00" + +# This works the same with & without USE_PSA as we never get to ECDH: +# we abort as soon as we determined the cert is bad. requires_config_enabled MBEDTLS_ECP_RESTARTABLE requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 @@ -8428,10 +8449,12 @@ run_test "EC restart: TLS, max_ops=1000, badsign" \ -c "! mbedtls_ssl_handshake returned" \ -c "X509 - Certificate verification failed" +# With USE_PSA disabled we expect full restartable behaviour. requires_config_enabled MBEDTLS_ECP_RESTARTABLE requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 -run_test "EC restart: TLS, max_ops=1000, auth_mode=optional badsign" \ +requires_config_disabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: TLS, max_ops=1000, auth_mode=optional badsign (no USE_PSA)" \ "$P_SRV curves=secp256r1 auth_mode=required \ crt_file=data_files/server5-badsign.crt \ key_file=data_files/server5.key" \ @@ -8447,10 +8470,34 @@ run_test "EC restart: TLS, max_ops=1000, auth_mode=optional badsign" \ -C "! mbedtls_ssl_handshake returned" \ -C "X509 - Certificate verification failed" +# With USE_PSA enabled we expect only partial restartable behaviour: +# everything except ECDH (where TLS calls PSA directly). requires_config_enabled MBEDTLS_ECP_RESTARTABLE requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 -run_test "EC restart: TLS, max_ops=1000, auth_mode=none badsign" \ +requires_config_enabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: TLS, max_ops=1000, auth_mode=optional badsign (USE_PSA)" \ + "$P_SRV curves=secp256r1 auth_mode=required \ + crt_file=data_files/server5-badsign.crt \ + key_file=data_files/server5.key" \ + "$P_CLI force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256 \ + key_file=data_files/server5.key crt_file=data_files/server5.crt \ + debug_level=1 ec_max_ops=1000 auth_mode=optional" \ + 0 \ + -c "x509_verify_cert.*4b00" \ + -c "mbedtls_pk_verify.*4b00" \ + -C "mbedtls_ecdh_make_public.*4b00" \ + -c "mbedtls_pk_sign.*4b00" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -C "! mbedtls_ssl_handshake returned" \ + -C "X509 - Certificate verification failed" + +# With USE_PSA disabled we expect full restartable behaviour. +requires_config_enabled MBEDTLS_ECP_RESTARTABLE +requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_config_disabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: TLS, max_ops=1000, auth_mode=none badsign (no USE_PSA)" \ "$P_SRV curves=secp256r1 auth_mode=required \ crt_file=data_files/server5-badsign.crt \ key_file=data_files/server5.key" \ @@ -8466,10 +8513,34 @@ run_test "EC restart: TLS, max_ops=1000, auth_mode=none badsign" \ -C "! mbedtls_ssl_handshake returned" \ -C "X509 - Certificate verification failed" +# With USE_PSA enabled we expect only partial restartable behaviour: +# everything except ECDH (where TLS calls PSA directly). requires_config_enabled MBEDTLS_ECP_RESTARTABLE requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 -run_test "EC restart: DTLS, max_ops=1000" \ +requires_config_enabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: TLS, max_ops=1000, auth_mode=none badsign (USE_PSA)" \ + "$P_SRV curves=secp256r1 auth_mode=required \ + crt_file=data_files/server5-badsign.crt \ + key_file=data_files/server5.key" \ + "$P_CLI force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256 \ + key_file=data_files/server5.key crt_file=data_files/server5.crt \ + debug_level=1 ec_max_ops=1000 auth_mode=none" \ + 0 \ + -C "x509_verify_cert.*4b00" \ + -c "mbedtls_pk_verify.*4b00" \ + -C "mbedtls_ecdh_make_public.*4b00" \ + -c "mbedtls_pk_sign.*4b00" \ + -C "! The certificate is not correctly signed by the trusted CA" \ + -C "! mbedtls_ssl_handshake returned" \ + -C "X509 - Certificate verification failed" + +# With USE_PSA disabled we expect full restartable behaviour. +requires_config_enabled MBEDTLS_ECP_RESTARTABLE +requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_config_disabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: DTLS, max_ops=1000 (no USE_PSA)" \ "$P_SRV curves=secp256r1 auth_mode=required dtls=1" \ "$P_CLI force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256 \ key_file=data_files/server5.key crt_file=data_files/server5.crt \ @@ -8480,10 +8551,29 @@ run_test "EC restart: DTLS, max_ops=1000" \ -c "mbedtls_ecdh_make_public.*4b00" \ -c "mbedtls_pk_sign.*4b00" +# With USE_PSA enabled we expect only partial restartable behaviour: +# everything except ECDH (where TLS calls PSA directly). requires_config_enabled MBEDTLS_ECP_RESTARTABLE requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 -run_test "EC restart: TLS, max_ops=1000 no client auth" \ +requires_config_enabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: DTLS, max_ops=1000 (USE_PSA)" \ + "$P_SRV curves=secp256r1 auth_mode=required dtls=1" \ + "$P_CLI force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256 \ + key_file=data_files/server5.key crt_file=data_files/server5.crt \ + dtls=1 debug_level=1 ec_max_ops=1000" \ + 0 \ + -c "x509_verify_cert.*4b00" \ + -c "mbedtls_pk_verify.*4b00" \ + -C "mbedtls_ecdh_make_public.*4b00" \ + -c "mbedtls_pk_sign.*4b00" + +# With USE_PSA disabled we expect full restartable behaviour. +requires_config_enabled MBEDTLS_ECP_RESTARTABLE +requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_config_disabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: TLS, max_ops=1000 no client auth (no USE_PSA)" \ "$P_SRV curves=secp256r1" \ "$P_CLI force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256 \ debug_level=1 ec_max_ops=1000" \ @@ -8494,6 +8584,22 @@ run_test "EC restart: TLS, max_ops=1000 no client auth" \ -C "mbedtls_pk_sign.*4b00" +# With USE_PSA enabled we expect only partial restartable behaviour: +# everything except ECDH (where TLS calls PSA directly). +requires_config_enabled MBEDTLS_ECP_RESTARTABLE +requires_config_enabled MBEDTLS_ECP_DP_SECP256R1_ENABLED +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_config_enabled MBEDTLS_USE_PSA_CRYPTO +run_test "EC restart: TLS, max_ops=1000 no client auth (USE_PSA)" \ + "$P_SRV curves=secp256r1" \ + "$P_CLI force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256 \ + debug_level=1 ec_max_ops=1000" \ + 0 \ + -c "x509_verify_cert.*4b00" \ + -c "mbedtls_pk_verify.*4b00" \ + -C "mbedtls_ecdh_make_public.*4b00" \ + -C "mbedtls_pk_sign.*4b00" + # Restartable is only for ECDHE-ECDSA, with another ciphersuite we expect no # restartable behaviour at all (not even client auth). # This is the same as "EC restart: TLS, max_ops=1000" except with ECDHE-RSA, From a37398427b05fe7af56ec1fd0314d59aac7b5b39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 6 Dec 2022 09:35:54 +0100 Subject: [PATCH 06/13] Remove check for tests disabled with USE_PSA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit declared that some tests cases in ssl-opt.sh depend on USE_PSA being disabled, which is the right thing to do. We had a check that forbade that - it was mainly meant to prevent accidental re-introduction of such dependencies after we cleaned up a number of cases where it was not warranted, but already at the time that was controversial [1]. Now it's preventing us from doing the right thing, so let's just remove it. [1]: https://github.com/Mbed-TLS/mbedtls/pull/5742#discussion_r855112412 See also https://github.com/Mbed-TLS/mbedtls/pull/5907/ which also removes this for a similar reason. Signed-off-by: Manuel Pégourié-Gonnard --- tests/scripts/all.sh | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index d3eedcf35d..e1717a14b8 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -871,12 +871,6 @@ component_check_test_cases () { fi tests/scripts/check_test_cases.py $opt unset opt - - # Check that no tests are explicitely disabled when USE_PSA_CRYPTO is set - # as a matter of policy to ensure there is no missed testing - msg "Check: explicitely disabled test with USE_PSA_CRYPTO" # < 1s - not grep -n 'depends_on:.*!MBEDTLS_USE_PSA_CRYPTO' tests/suites/*.function tests/suites/*.data - not grep -n '^ *requires_config_disabled.*MBEDTLS_USE_PSA_CRYPTO' tests/ssl-opt.sh tests/opt-testcases/*.sh } component_check_doxygen_warnings () { From a6e3d3ec1032f9a71d1ce33eb841b571248b2917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 6 Dec 2022 12:10:33 +0100 Subject: [PATCH 07/13] Disable restartable in build_module_alt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously we did not need that as restartable was excluded from full. Signed-off-by: Manuel Pégourié-Gonnard --- tests/scripts/all.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index e1717a14b8..a6a688c1ae 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1869,10 +1869,13 @@ component_test_depends_py_pkalgs_psa () { component_build_module_alt () { msg "build: MBEDTLS_XXX_ALT" # ~30s scripts/config.py full - # Disable options that are incompatible with some ALT implementations. + + # Disable options that are incompatible with some ALT implementations: # aesni.c and padlock.c reference mbedtls_aes_context fields directly. scripts/config.py unset MBEDTLS_AESNI_C scripts/config.py unset MBEDTLS_PADLOCK_C + # MBEDTLS_ECP_RESTARTABLE is documented as incompatible. + scripts/config.py unset MBEDTLS_ECP_RESTARTABLE # You can only have one threading implementation: alt or pthread, not both. scripts/config.py unset MBEDTLS_THREADING_PTHREAD # The SpecifiedECDomain parsing code accesses mbedtls_ecp_group fields @@ -1884,10 +1887,12 @@ component_build_module_alt () { # MBEDTLS_SHA512_*ALT can't be used with MBEDTLS_SHA512_USE_A64_CRYPTO_* scripts/config.py unset MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT scripts/config.py unset MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY + # Enable all MBEDTLS_XXX_ALT for whole modules. Do not enable # MBEDTLS_XXX_YYY_ALT which are for single functions. scripts/config.py set-all 'MBEDTLS_([A-Z0-9]*|NIST_KW)_ALT' scripts/config.py unset MBEDTLS_DHM_ALT #incompatible with MBEDTLS_DEBUG_C + # We can only compile, not link, since we don't have any implementations # suitable for testing with the dummy alt headers. make CC=gcc CFLAGS='-Werror -Wall -Wextra -I../tests/include/alt-dummy' lib From 578664601e0b0ac7d74099270f88ea03e7be547e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 6 Dec 2022 12:14:49 +0100 Subject: [PATCH 08/13] Fix missing dependency declaration in test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit muladd() (restartable or not) is only available when at least one short weirstrass curve is enabled. Found by depends.py curves (now that restartable is part of full). Also, document that restartable only work for short weierstrass curves (actually unrelated, but this made me think of that). Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/mbedtls_config.h | 2 ++ tests/suites/test_suite_ecp.function | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 7a681d1f2d..921a74918f 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -721,6 +721,8 @@ * elliptic curve functionality. It is incompatible with * MBEDTLS_ECP_ALT, MBEDTLS_ECDH_XXX_ALT, MBEDTLS_ECDSA_XXX_ALT. * + * \note This option only works for Short Weierstrass curves. + * * Uncomment this macro to enable restartable ECC computations. */ //#define MBEDTLS_ECP_RESTARTABLE diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 7d29e525ed..2971c576ce 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -237,7 +237,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_ECP_RESTARTABLE */ +/* BEGIN_CASE depends_on:MBEDTLS_ECP_RESTARTABLE:MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ void ecp_muladd_restart( int id, char *xR_str, char *yR_str, char *u1_str, char *u2_str, char *xQ_str, char *yQ_str, From ad45c4d3862b1a64f66f96a52ccbc433bdf5720b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 6 Dec 2022 13:20:06 +0100 Subject: [PATCH 09/13] Document that ECP_RESTARTABLE depends on ECP_C MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is not new, it had always been the case, just not documented. Pointed out by depends.py pkalgs (again, now that restartable is part of full). Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/check_config.h | 5 +++++ include/mbedtls/mbedtls_config.h | 2 ++ tests/scripts/depends.py | 1 + 3 files changed, 8 insertions(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index e2f8e62700..17010fe5a1 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -124,6 +124,11 @@ #error "MBEDTLS_ECP_RESTARTABLE defined, but it cannot coexist with an alternative ECP implementation" #endif +#if defined(MBEDTLS_ECP_RESTARTABLE) && \ + !defined(MBEDTLS_ECP_C) +#error "MBEDTLS_ECP_RESTARTABLE defined, but not all prerequisites" +#endif + #if defined(MBEDTLS_ECDSA_DETERMINISTIC) && !defined(MBEDTLS_HMAC_DRBG_C) #error "MBEDTLS_ECDSA_DETERMINISTIC defined, but not all prerequisites" #endif diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 921a74918f..5d4033c170 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -723,6 +723,8 @@ * * \note This option only works for Short Weierstrass curves. * + * Requires: MBEDTLS_ECP_C + * * Uncomment this macro to enable restartable ECC computations. */ //#define MBEDTLS_ECP_RESTARTABLE diff --git a/tests/scripts/depends.py b/tests/scripts/depends.py index 0d6ec94c80..d09b732ca6 100755 --- a/tests/scripts/depends.py +++ b/tests/scripts/depends.py @@ -234,6 +234,7 @@ REVERSE_DEPENDENCIES = { 'MBEDTLS_ECP_C': ['MBEDTLS_ECDSA_C', 'MBEDTLS_ECDH_C', 'MBEDTLS_ECJPAKE_C', + 'MBEDTLS_ECP_RESTARTABLE', 'MBEDTLS_KEY_EXCHANGE_ECDH_ECDSA_ENABLED', 'MBEDTLS_KEY_EXCHANGE_ECDH_RSA_ENABLED', 'MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED', From 182a23b1da41917e84cc3c2476f12eb4466d28ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 7 Dec 2022 10:38:43 +0100 Subject: [PATCH 10/13] Adjust all.sh now that restartable is in full MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/scripts/all.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index a6a688c1ae..08c6a744bd 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1913,7 +1913,6 @@ component_test_no_use_psa_crypto_full_cmake_asan() { # full minus MBEDTLS_USE_PSA_CRYPTO: run the same set of tests as basic-build-test.sh msg "build: cmake, full config minus MBEDTLS_USE_PSA_CRYPTO, ASan" scripts/config.py full - scripts/config.py set MBEDTLS_ECP_RESTARTABLE # not using PSA, so enable restartable ECC scripts/config.py unset MBEDTLS_PSA_CRYPTO_C scripts/config.py unset MBEDTLS_USE_PSA_CRYPTO scripts/config.py unset MBEDTLS_SSL_PROTO_TLS1_3 @@ -1928,6 +1927,9 @@ component_test_no_use_psa_crypto_full_cmake_asan() { msg "test: main suites (full minus MBEDTLS_USE_PSA_CRYPTO)" make test + # Note: ssl-opt.sh has some test cases that depend on + # MBEDTLS_ECP_RESTARTABLE && !MBEDTLS_USE_PSA_CRYPTO + # This is the only component where those tests are not skipped. msg "test: ssl-opt.sh (full minus MBEDTLS_USE_PSA_CRYPTO)" tests/ssl-opt.sh From b2812cc2746311c6dc7d72242f434bf09c25d807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 9 Dec 2022 09:53:55 +0100 Subject: [PATCH 11/13] Clarify documentation of ECP_RESTARTABLE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/mbedtls_config.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 5d4033c170..b4f5eb7d4d 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -693,7 +693,8 @@ * This option: * - Adds xxx_restartable() variants of existing operations in the * following modules, with corresponding restart context types: - * - ECP: scalar multiplication (mult), linear combination (muladd); + * - ECP (for Short Weierstrass curves only): scalar multiplication (mul), + * linear combination (muladd); * - ECDSA: signature generation & verification; * - PK: signature generation & verification; * - X509: certificate chain verification. @@ -701,12 +702,12 @@ * - Changes the behaviour of TLS 1.2 clients (not servers) when using the * ECDHE-ECDSA key exchange (not other key exchanges) to make all ECC * computations restartable: - * - ECDH operations from the key exchange - unless MBEDTLS_USE_PSA_CRYPTO - * is also enabled. + * - ECDH operations from the key exchange, only for Short Weierstass + * curves, only when MBEDTLS_USE_PSA_CRYPTO is not enabled. * - verification of the server's key exchange signature; * - verification of the server's certificate chain; - * - generation of our signature if client authentication is used, with an - * ECC key/certificate. + * - generation of the client's signature if client authentication is used, + * with an ECC key/certificate. * * \note In the cases above, the usual SSL/TLS functions, such as * mbedtls_ssl_handshake(), can now return @@ -715,14 +716,13 @@ * \note When this option and MBEDTLS_USE_PSA_CRYPTO are both enabled, * restartable operations in PK, X.509 and TLS (see above) are not * using PSA. On the other hand, ECDH computations in TLS are using - * PSA, and are not restartable. + * PSA, and are not restartable. These are temporary limitations that + * should be lifted in the future. * * \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, MBEDTLS_ECDSA_XXX_ALT. * - * \note This option only works for Short Weierstrass curves. - * * Requires: MBEDTLS_ECP_C * * Uncomment this macro to enable restartable ECC computations. From df0c73c3085477714d3c5fd3fb14a8973573a24c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 9 Dec 2022 09:57:39 +0100 Subject: [PATCH 12/13] Readability improvement in pk_wrap.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/pk_wrap.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/library/pk_wrap.c b/library/pk_wrap.c index 00abffb2df..ea7d7263ec 100644 --- a/library/pk_wrap.c +++ b/library/pk_wrap.c @@ -1162,12 +1162,11 @@ static int ecdsa_sign_wrap( void *ctx_arg, mbedtls_md_type_t md_alg, size_t key_len; unsigned char buf[MBEDTLS_PK_ECP_PRV_DER_MAX_BYTES]; unsigned char *p; + psa_algorithm_t psa_hash = mbedtls_hash_info_psa_from_md( md_alg ); #if defined(MBEDTLS_ECDSA_DETERMINISTIC) - psa_algorithm_t psa_sig_md = - PSA_ALG_DETERMINISTIC_ECDSA( mbedtls_hash_info_psa_from_md( md_alg ) ); + psa_algorithm_t psa_sig_md = PSA_ALG_DETERMINISTIC_ECDSA( psa_hash ); #else - psa_algorithm_t psa_sig_md = - PSA_ALG_ECDSA( mbedtls_hash_info_psa_from_md( md_alg ) ); + psa_algorithm_t psa_sig_md = PSA_ALG_ECDSA( psa_hash ); #endif size_t curve_bits; psa_ecc_family_t curve = From 67bad73e870477a2c44f843eac50f91c148db6fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 9 Dec 2022 10:01:29 +0100 Subject: [PATCH 13/13] Add a ChangeLog entry for the ECDSA deterministic change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/pk-sign-restartable.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/pk-sign-restartable.txt diff --git a/ChangeLog.d/pk-sign-restartable.txt b/ChangeLog.d/pk-sign-restartable.txt new file mode 100644 index 0000000000..35da2be131 --- /dev/null +++ b/ChangeLog.d/pk-sign-restartable.txt @@ -0,0 +1,5 @@ +Changes + * When MBEDTLS_USE_PSA_CRYPTO and MBEDTLS_ECDSA_DETERMINISTIC are both + defined, mbedtls_pk_sign() now use deterministic ECDSA for ECDSA + signatures. This aligns the behaviour with MBEDTLS_USE_PSA_CRYPTO to + the behaviour without it, where deterministic ECDSA was already used.