From 7cf872557a393326bc7246cbd3d37f2eb6ef3174 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Tue, 14 Jun 2022 07:12:33 -0400 Subject: [PATCH 1/5] Rearrange the session resumption code Previously, the transforms were populated before extension parsing, which resulted in the client rejecting a server hello that contained a connection ID. Signed-off-by: Andrzej Kurek --- library/ssl_tls12_client.c | 23 +++++++++++++---------- tests/ssl-opt.sh | 23 +++++++++++++++++++++++ 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c index f516efab1..3388f10d1 100644 --- a/library/ssl_tls12_client.c +++ b/library/ssl_tls12_client.c @@ -1409,16 +1409,6 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) else { ssl->state = MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC; - - if( ( ret = mbedtls_ssl_derive_keys( ssl ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_derive_keys", ret ); - mbedtls_ssl_send_alert_message( - ssl, - MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); - return( ret ); - } } MBEDTLS_SSL_DEBUG_MSG( 3, ( "%s session has been resumed", @@ -1654,6 +1644,19 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) } } + if( ssl->state == MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) + { + if( ( ret = mbedtls_ssl_derive_keys( ssl ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_derive_keys", ret ); + mbedtls_ssl_send_alert_message( + ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_INTERNAL_ERROR ); + return( ret ); + } + } + /* * Renegotiation security checks */ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index f81370d35..aff5c7eed 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -3650,6 +3650,29 @@ run_test "Session resume using cache: openssl server" \ -C "parse new session ticket" \ -c "a session has been resumed" +# Tests for Session resume and extensions + +requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_config_enabled MBEDTLS_SSL_DTLS_CONNECTION_ID +run_test "Session resume and connection ID" \ + "$P_SRV debug_level=3 cid=1 cid_val=dead dtls=1 tickets=0" \ + "$P_CLI debug_level=3 cid=1 cid_val=beef dtls=1 tickets=0 reconnect=1" \ + 0 \ + -c "Enable use of CID extension." \ + -s "Enable use of CID extension." \ + -c "client hello, adding CID extension" \ + -s "found CID extension" \ + -s "Use of CID extension negotiated" \ + -s "server hello, adding CID extension" \ + -c "found CID extension" \ + -c "Use of CID extension negotiated" \ + -s "Copy CIDs into SSL transform" \ + -c "Copy CIDs into SSL transform" \ + -c "Peer CID (length 2 Bytes): de ad" \ + -s "Peer CID (length 2 Bytes): be ef" \ + -s "Use of Connection ID has been negotiated" \ + -c "Use of Connection ID has been negotiated" + # Tests for Session Resume based on session-ID and cache, DTLS requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 From 5708b45154dcf908619fc12c30d6972d246b016e Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Thu, 23 Jun 2022 08:00:14 -0400 Subject: [PATCH 2/5] Add a changelog entry for the session resumption + CID bug Signed-off-by: Andrzej Kurek --- ChangeLog.d/resumption_cid.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/resumption_cid.txt diff --git a/ChangeLog.d/resumption_cid.txt b/ChangeLog.d/resumption_cid.txt new file mode 100644 index 000000000..d6d74b877 --- /dev/null +++ b/ChangeLog.d/resumption_cid.txt @@ -0,0 +1,4 @@ +Bugfix + * Fix a bug where the connection ID extension on client side would not work + after a session resumption took place. This caused the client to not + recognize records containing a CID and drop them. From 3a29e9cf579948cc39fd84f34bb5a8f1a8701782 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Tue, 5 Jul 2022 10:49:10 -0400 Subject: [PATCH 3/5] Improve changelog wording Co-authored-by: Ronald Cron Signed-off-by: Andrzej Kurek --- ChangeLog.d/resumption_cid.txt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ChangeLog.d/resumption_cid.txt b/ChangeLog.d/resumption_cid.txt index d6d74b877..0cdd4fc6a 100644 --- a/ChangeLog.d/resumption_cid.txt +++ b/ChangeLog.d/resumption_cid.txt @@ -1,4 +1,5 @@ Bugfix - * Fix a bug where the connection ID extension on client side would not work - after a session resumption took place. This caused the client to not - recognize records containing a CID and drop them. + * Fix server connection identifier setting for outgoing encrypted records + on DTLS 1.2 session resumption. After DTLS 1.2 session resumption with + connection identifier, the Mbed TLS client now sends properly the server + connection identifier in encrypted record headers. Fix #5872. From 21b50808cdc379cee4533c4b4bddd75a56a246e5 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 6 Jul 2022 03:26:55 -0400 Subject: [PATCH 4/5] Clarify the need for calling mbedtls_ssl_derive_keys after extension parsing Use a more straightforward condition to note that session resumption is happening. Co-authored-by: Ronald Cron Signed-off-by: Andrzej Kurek --- library/ssl_tls12_client.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c index 3388f10d1..3f2ef3148 100644 --- a/library/ssl_tls12_client.c +++ b/library/ssl_tls12_client.c @@ -1644,7 +1644,12 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) } } - if( ssl->state == MBEDTLS_SSL_SERVER_CHANGE_CIPHER_SPEC ) + /* + * mbedtls_ssl_derive_keys() has to be called after the parsing of the + * extensions. It sets the transform data for the resumed session which in + * case of DTLS includes the server CID extracted from the CID extension. + */ + if( ssl->handshake->resume ) { if( ( ret = mbedtls_ssl_derive_keys( ssl ) ) != 0 ) { From 1ce9ca0630c2f119df67f14a0620ba229e8bcbea Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 6 Jul 2022 06:48:48 -0400 Subject: [PATCH 5/5] Changelog rewording Signed-off-by: Andrzej Kurek --- ChangeLog.d/resumption_cid.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/resumption_cid.txt b/ChangeLog.d/resumption_cid.txt index 0cdd4fc6a..5c237aa88 100644 --- a/ChangeLog.d/resumption_cid.txt +++ b/ChangeLog.d/resumption_cid.txt @@ -1,5 +1,5 @@ Bugfix * Fix server connection identifier setting for outgoing encrypted records on DTLS 1.2 session resumption. After DTLS 1.2 session resumption with - connection identifier, the Mbed TLS client now sends properly the server + connection identifier, the Mbed TLS client now properly sends the server connection identifier in encrypted record headers. Fix #5872.