From 590bdcbddf944b573984b0312289c6969c5597d8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sun, 25 Aug 2024 10:41:40 +0200 Subject: [PATCH 1/8] Call psa_crypto_init in the library when required for TLS 1.3: doc For backward compatibility with Mbed TLS <=3.5.x, applications must be able to make a TLS connection with a peer that supports both TLS 1.2 and TLS 1.3, regardless of whether they call psa_crypto_init(). Since Mbed TLS 3.6.0, we enable TLS 1.3 in the default configuration, so we must take care of calling psa_crypto_init() if needed. This is a change from TLS 1.3 in previous versions, where enabling MBEDTLS_SSL_PROTO_TLS1_3 was a user choice and could have additional requirement. This commit removes the compatibility-breaking requirement from the documentation. Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 5 +++-- include/mbedtls/ssl.h | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 7ed1a433ba..4c01cd5c5b 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -1798,8 +1798,9 @@ * Requires: MBEDTLS_PSA_CRYPTO_C * * \note TLS 1.3 uses PSA crypto for cryptographic operations that are - * directly performed by TLS 1.3 code. As a consequence, you must - * call psa_crypto_init() before the first TLS 1.3 handshake. + * directly performed by TLS 1.3 code. As a consequence, when TLS 1.3 + * is enabled, a TLS handshake may call psa_crypto_init(), even + * if it ends up negotiating a different TLS version. * * \note Cryptographic operations performed indirectly via another module * (X.509, PK) or by code shared with TLS 1.2 (record protection, diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 4b59e78532..273933196d 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -4923,10 +4923,11 @@ int mbedtls_ssl_get_session(const mbedtls_ssl_context *ssl, * currently being processed might or might not contain further * DTLS records. * - * \note If the context is configured to allow TLS 1.3, or if - * #MBEDTLS_USE_PSA_CRYPTO is enabled, the PSA crypto + * \note If #MBEDTLS_USE_PSA_CRYPTO is enabled, the PSA crypto * subsystem must have been initialized by calling * psa_crypto_init() before calling this function. + * Otherwise, the handshake may call psa_crypto_init() + * if it ends up negotiating TLS 1.3. */ int mbedtls_ssl_handshake(mbedtls_ssl_context *ssl); From 59503017405b852872fdc6d38f542692a4bc4660 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 24 Aug 2024 11:05:47 +0200 Subject: [PATCH 2/8] Don't call psa_crypto_init in unit tests when not required for TLS 1.3 For backward compatibility with Mbed TLS <=3.5.x, applications must be able to make a TLS connection with a peer that supports both TLS 1.2 and TLS 1.3, regardless of whether they call psa_crypto_init(). Since Mbed TLS 3.6.0, we enable TLS 1.3 in the default configuration, so we must take care of calling psa_crypto_init() if needed. This is a change from TLS 1.3 in previous versions, where enabling MBEDTLS_SSL_PROTO_TLS1_3 was a user choice and could have additional requirement. This commit changes our unit tests to validate that the library does not have the compatibility-breaking requirement. Signed-off-by: Gilles Peskine --- tests/include/test/psa_crypto_helpers.h | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index 71ba0fc021..1039712aef 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -334,9 +334,18 @@ uint64_t mbedtls_test_parse_binary_string(data_t *bin_string); * This is like #PSA_DONE except it does nothing under the same conditions as * #USE_PSA_INIT. */ -#if defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3) +#if defined(MBEDTLS_USE_PSA_CRYPTO) #define USE_PSA_INIT() PSA_INIT() #define USE_PSA_DONE() PSA_DONE() +#elif defined(MBEDTLS_SSL_PROTO_TLS1_3) +/* TLS 1.3 must work without having called psa_crypto_init(), for backward + * compatibility with Mbed TLS <= 3.5 when connecting with a peer that + * supports both TLS 1.2 and TLS 1.3. See mbedtls_ssl_tls13_crypto_init() + * and https://github.com/Mbed-TLS/mbedtls/issues/9072 . */ +#define USE_PSA_INIT() ((void) 0) +/* TLS 1.3 may have initialized the PSA subsystem. Shut it down cleanly, + * otherwise Asan and Valgrind would notice a resource leak. */ +#define USE_PSA_DONE() PSA_DONE() #else /* MBEDTLS_USE_PSA_CRYPTO || MBEDTLS_SSL_PROTO_TLS1_3 */ /* Define empty macros so that we can use them in the preamble and teardown * of every test function that uses PSA conditionally based on @@ -408,13 +417,12 @@ uint64_t mbedtls_test_parse_binary_string(data_t *bin_string); * This is like #PSA_DONE except it does nothing under the same conditions as * #MD_OR_USE_PSA_INIT. */ -#if defined(MBEDTLS_MD_SOME_PSA) || \ - defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3) +#if defined(MBEDTLS_MD_SOME_PSA) #define MD_OR_USE_PSA_INIT() PSA_INIT() #define MD_OR_USE_PSA_DONE() PSA_DONE() #else -#define MD_OR_USE_PSA_INIT() ((void) 0) -#define MD_OR_USE_PSA_DONE() ((void) 0) +#define MD_OR_USE_PSA_INIT() USE_PSA_INIT() +#define MD_OR_USE_PSA_DONE() USE_PSA_DONE() #endif /** \def AES_PSA_INIT From cd4da16eea02f2cfeeb53ba0845305bcb4558980 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 23 Aug 2024 21:51:39 +0200 Subject: [PATCH 3/8] Don't call psa_crypto_init in test programs when not required for TLS 1.3 For backward compatibility with Mbed TLS <=3.5.x, applications must be able to make a TLS connection with a peer that supports both TLS 1.2 and TLS 1.3, regardless of whether they call psa_crypto_init(). Since Mbed TLS 3.6.0, we enable TLS 1.3 in the default configuration, so we must take care of calling psa_crypto_init() if needed. This is a change from TLS 1.3 in previous versions, where enabling MBEDTLS_SSL_PROTO_TLS1_3 was a user choice and could have additional requirement. This commit changes our test programs to validate that the library does not have the compatibility-breaking requirement. Signed-off-by: Gilles Peskine --- programs/ssl/ssl_client2.c | 15 ++++++++++++--- programs/ssl/ssl_server2.c | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index d494de3a60..cd839c1610 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -818,8 +818,6 @@ int main(int argc, char *argv[]) psa_key_attributes_t key_attributes; #endif psa_status_t status; -#elif defined(MBEDTLS_SSL_PROTO_TLS1_3) - psa_status_t status; #endif rng_context_t rng; @@ -894,7 +892,15 @@ int main(int argc, char *argv[]) memset((void *) alpn_list, 0, sizeof(alpn_list)); #endif -#if defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3) + /* For builds with TLS 1.3 enabled but not MBEDTLS_USE_PSA_CRYPTO, + * we deliberately do not call psa_crypto_init() here, to test that + * the library is backward-compatible with versions prior to 3.6.0 + * where calling psa_crypto_init() was not required to open a TLS + * connection in the default configuration. See + * https://github.com/Mbed-TLS/mbedtls/issues/9072 and + * mbedtls_ssl_tls13_crypto_init(). + */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) status = psa_crypto_init(); if (status != PSA_SUCCESS) { mbedtls_fprintf(stderr, "Failed to initialize PSA Crypto implementation: %d\n", @@ -3192,6 +3198,9 @@ exit: /* For builds with MBEDTLS_TEST_USE_PSA_CRYPTO_RNG psa crypto * resources are freed by rng_free(). */ + /* For builds with MBEDTLS_SSL_PROTO_TLS1_3, PSA may have been + * initialized under the hood by the TLS layer. See + * mbedtls_ssl_tls13_crypto_init(). */ #if (defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3)) && \ !defined(MBEDTLS_TEST_USE_PSA_CRYPTO_RNG) mbedtls_psa_crypto_free(); diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 480b262446..79a742e152 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -1594,7 +1594,7 @@ int main(int argc, char *argv[]) int i; char *p, *q; const int *list; -#if defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3) +#if defined(MBEDTLS_USE_PSA_CRYPTO) psa_status_t status; #endif unsigned char eap_tls_keymaterial[16]; @@ -1660,7 +1660,15 @@ int main(int argc, char *argv[]) mbedtls_ssl_cookie_init(&cookie_ctx); #endif -#if defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3) + /* For builds with TLS 1.3 enabled but not MBEDTLS_USE_PSA_CRYPTO, + * we deliberately do not call psa_crypto_init() here, to test that + * the library is backward-compatible with versions prior to 3.6.0 + * where calling psa_crypto_init() was not required to open a TLS + * connection in the default configuration. See + * https://github.com/Mbed-TLS/mbedtls/issues/9072 and + * mbedtls_ssl_tls13_crypto_init(). + */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) status = psa_crypto_init(); if (status != PSA_SUCCESS) { mbedtls_fprintf(stderr, "Failed to initialize PSA Crypto implementation: %d\n", @@ -4309,6 +4317,9 @@ exit: /* For builds with MBEDTLS_TEST_USE_PSA_CRYPTO_RNG psa crypto * resources are freed by rng_free(). */ + /* For builds with MBEDTLS_SSL_PROTO_TLS1_3, PSA may have been + * initialized under the hood by the TLS layer. See + * mbedtls_ssl_tls13_crypto_init(). */ #if (defined(MBEDTLS_USE_PSA_CRYPTO) || defined(MBEDTLS_SSL_PROTO_TLS1_3)) \ && !defined(MBEDTLS_TEST_USE_PSA_CRYPTO_RNG) mbedtls_psa_crypto_free(); From 069bccdf78d4f0e35f4718e4efd6427b74a0bd6a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 23 Aug 2024 21:55:24 +0200 Subject: [PATCH 4/8] Call psa_crypto_init in the library when required for TLS 1.3 For backward compatibility with Mbed TLS <=3.5.x, applications must be able to make a TLS connection with a peer that supports both TLS 1.2 and TLS 1.3, regardless of whether they call psa_crypto_init(). Since Mbed TLS 3.6.0, we enable TLS 1.3 in the default configuration, so we must take care of calling psa_crypto_init() if needed. This is a change from TLS 1.3 in previous versions, where enabling MBEDTLS_SSL_PROTO_TLS1_3 was a user choice and could have additional requirement. This commit makes the library call psa_crypto_init() when it needs PSA crypto in a situation where the application might not have called it, namely, when starting a TLS 1.3 connection. Signed-off-by: Gilles Peskine --- ChangeLog.d/tls13-psa_crypto_init.txt | 4 ++++ library/ssl_misc.h | 20 ++++++++++++++++++++ library/ssl_tls13_client.c | 5 +++++ library/ssl_tls13_generic.c | 10 ++++++++++ library/ssl_tls13_server.c | 5 +++++ 5 files changed, 44 insertions(+) create mode 100644 ChangeLog.d/tls13-psa_crypto_init.txt diff --git a/ChangeLog.d/tls13-psa_crypto_init.txt b/ChangeLog.d/tls13-psa_crypto_init.txt new file mode 100644 index 0000000000..1b188b698b --- /dev/null +++ b/ChangeLog.d/tls13-psa_crypto_init.txt @@ -0,0 +1,4 @@ +Bugfix + * Fix TLS connections failing when the handshake selects TLS 1.3 + in an application that does not call psa_crypto_init(). + Fixes #9072. diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 8402fe86ae..082bc9bd93 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -1891,6 +1891,26 @@ static inline int mbedtls_ssl_conf_is_hybrid_tls12_tls13(const mbedtls_ssl_confi #endif /* MBEDTLS_SSL_PROTO_TLS1_2 && MBEDTLS_SSL_PROTO_TLS1_3 */ #if defined(MBEDTLS_SSL_PROTO_TLS1_3) + +/** \brief Initialize the PSA crypto subsystem if necessary. + * + * Call this function before doing any cryptography in a TLS 1.3 handshake. + * + * This is necessary in Mbed TLS 3.x for backward compatibility. + * Up to Mbed TLS 3.5, in the default configuration, you could perform + * a TLS connection with default parameters without having called + * psa_crypto_init(), since the TLS layer only supported TLS 1.2 and + * did not use PSA crypto. (TLS 1.2 only uses PSA crypto if + * MBEDTLS_USE_PSA_CRYPTO is enabled, which is not the case in the default + * configuration.) Starting with Mbed TLS 3.6.0, TLS 1.3 is enabled + * by default, and the TLS 1.3 layer uses PSA crypto. This means that + * applications that are not otherwise using PSA crypto and that worked + * with Mbed TLS 3.5 started failing in TLS 3.6.0 if they connected to + * a peer that supports TLS 1.3. See + * https://github.com/Mbed-TLS/mbedtls/issues/9072 + */ +int mbedtls_ssl_tls13_crypto_init(mbedtls_ssl_context *ssl); + extern const uint8_t mbedtls_ssl_tls13_hello_retry_request_magic[ MBEDTLS_SERVER_HELLO_RANDOM_LEN]; MBEDTLS_CHECK_RETURN_CRITICAL diff --git a/library/ssl_tls13_client.c b/library/ssl_tls13_client.c index 162e3a3146..b63b5e63c5 100644 --- a/library/ssl_tls13_client.c +++ b/library/ssl_tls13_client.c @@ -1141,6 +1141,11 @@ int mbedtls_ssl_tls13_write_client_hello_exts(mbedtls_ssl_context *ssl, *out_len = 0; + ret = mbedtls_ssl_tls13_crypto_init(ssl); + if (ret != 0) { + return ret; + } + /* Write supported_versions extension * * Supported Versions Extension is mandatory with TLS 1.3. diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 8ac6579e05..631e763faa 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -39,6 +39,16 @@ static int local_err_translation(psa_status_t status) #define PSA_TO_MBEDTLS_ERR(status) local_err_translation(status) #endif +int mbedtls_ssl_tls13_crypto_init(mbedtls_ssl_context *ssl) +{ + psa_status_t status = psa_crypto_init(); + if (status != PSA_SUCCESS) { + (void) ssl; // unused when debugging is disabled + MBEDTLS_SSL_DEBUG_RET(1, "psa_crypto_init", status); + } + return PSA_TO_MBEDTLS_ERR(status); +} + const uint8_t mbedtls_ssl_tls13_hello_retry_request_magic[ MBEDTLS_SERVER_HELLO_RANDOM_LEN] = { 0xCF, 0x21, 0xAD, 0x74, 0xE5, 0x9A, 0x61, 0x11, diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 9c949bd0b1..616d2ee574 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1948,6 +1948,11 @@ static int ssl_tls13_process_client_hello(mbedtls_ssl_context *ssl) MBEDTLS_SSL_DEBUG_MSG(2, ("=> parse client hello")); + ret = mbedtls_ssl_tls13_crypto_init(ssl); + if (ret != 0) { + return ret; + } + MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( ssl, MBEDTLS_SSL_HS_CLIENT_HELLO, &buf, &buflen)); From 50476272a931195fa1f4996689daa8d0d3743bc3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 26 Aug 2024 08:59:22 +0200 Subject: [PATCH 5/8] Error translation and init are needed in PSK-only builds as well Signed-off-by: Gilles Peskine --- library/ssl_tls13_generic.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/library/ssl_tls13_generic.c b/library/ssl_tls13_generic.c index 631e763faa..4251027c3e 100644 --- a/library/ssl_tls13_generic.c +++ b/library/ssl_tls13_generic.c @@ -27,7 +27,6 @@ #include "psa/crypto.h" #include "psa_util_internal.h" -#if defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_EPHEMERAL_ENABLED) /* Define a local translating function to save code size by not using too many * arguments in each translating place. */ static int local_err_translation(psa_status_t status) @@ -37,7 +36,6 @@ static int local_err_translation(psa_status_t status) psa_generic_status_to_mbedtls); } #define PSA_TO_MBEDTLS_ERR(status) local_err_translation(status) -#endif int mbedtls_ssl_tls13_crypto_init(mbedtls_ssl_context *ssl) { From 92e803ea5b70febe3708e6e8914c25dd96331456 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 26 Aug 2024 11:59:48 +0200 Subject: [PATCH 6/8] Clarify "negotiating" Signed-off-by: Gilles Peskine --- include/mbedtls/ssl.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 273933196d..466c734d37 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -4927,7 +4927,9 @@ int mbedtls_ssl_get_session(const mbedtls_ssl_context *ssl, * subsystem must have been initialized by calling * psa_crypto_init() before calling this function. * Otherwise, the handshake may call psa_crypto_init() - * if it ends up negotiating TLS 1.3. + * if a negotiation involving TLS 1.3 takes place (this may + * be the case even if TLS 1.3 is offered but eventually + * not selected). */ int mbedtls_ssl_handshake(mbedtls_ssl_context *ssl); From aa6ef7da50b5abf69274d5a94199b3044d9ea6c5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 26 Aug 2024 12:01:31 +0200 Subject: [PATCH 7/8] Changelog entry for psa_crypto_init potentially being called from TLS Signed-off-by: Gilles Peskine --- ChangeLog.d/tls13-psa_crypto_init.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog.d/tls13-psa_crypto_init.txt b/ChangeLog.d/tls13-psa_crypto_init.txt index 1b188b698b..311db65585 100644 --- a/ChangeLog.d/tls13-psa_crypto_init.txt +++ b/ChangeLog.d/tls13-psa_crypto_init.txt @@ -2,3 +2,8 @@ Bugfix * Fix TLS connections failing when the handshake selects TLS 1.3 in an application that does not call psa_crypto_init(). Fixes #9072. + +Changes + * A TLS handshake may now call psa_crypto_init() if TLS 1.3 is enabled. + This can happen even if TLS 1.3 is offered but eventually not selected + in the protocol version negotiation. From 57dbd69945e60ab2b47338d97c89dc8cf2237d01 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 26 Aug 2024 12:04:39 +0200 Subject: [PATCH 8/8] TLS 1.3 server: move crypto_init after protocol negotiation This reduces the workflows where psa_crypto_init is called when not necessary: it won't be called when a dual-version server receives a 1.2-only ClientHello. Signed-off-by: Gilles Peskine --- library/ssl_tls13_server.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/library/ssl_tls13_server.c b/library/ssl_tls13_server.c index 616d2ee574..693edc7b0b 100644 --- a/library/ssl_tls13_server.c +++ b/library/ssl_tls13_server.c @@ -1412,6 +1412,12 @@ static int ssl_tls13_parse_client_hello(mbedtls_ssl_context *ssl, ssl->session_negotiate->tls_version = MBEDTLS_SSL_VERSION_TLS1_3; ssl->session_negotiate->endpoint = ssl->conf->endpoint; + /* Before doing any crypto, make sure we can. */ + ret = mbedtls_ssl_tls13_crypto_init(ssl); + if (ret != 0) { + return ret; + } + /* * We are negotiating the version 1.3 of the protocol. Do what we have * postponed: copy of the client random bytes, copy of the legacy session @@ -1948,11 +1954,6 @@ static int ssl_tls13_process_client_hello(mbedtls_ssl_context *ssl) MBEDTLS_SSL_DEBUG_MSG(2, ("=> parse client hello")); - ret = mbedtls_ssl_tls13_crypto_init(ssl); - if (ret != 0) { - return ret; - } - MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_tls13_fetch_handshake_msg( ssl, MBEDTLS_SSL_HS_CLIENT_HELLO, &buf, &buflen));