TLS 1.3: SessionTicket: Enable kex change mode check when resumption #6616

lpy4105 · 2022-11-17T07:46:17Z

Description

Resolve: #6551

TODOS

In last flight connection, we should add below action

On client side, copy advertised kex modes to ticket_flags of received tickets.
On server side, check if the advertised modes can support session tickets, if no, do not send session tickets.
On server side, generate tickets with the advertised modes and set tickets in ticket callback.

In resumption connection,

On client side, filter the tickets with advertised modes
On server side, check the advertised modes and ticket flags, if it does not match, fall into 1-RTT

Tests

Test cases for combinations of ticket_flags and advertised modes.

Gatekeeper checklist

changelog required but not added in this PR, will be added in the follow-up PR addressing TLS 1.3: SessionTicket: Check ticket_flags in kex mode determination when resumption #6935
backport not required, TLS 1.3 only and no support for TLS 1.3 in 2.28

yuhaoth

Some comments

library/ssl_tls13_server.c

library/ssl_tls13_generic.c

include/mbedtls/debug.h

include/mbedtls/ssl.h

library/debug.c

yuhaoth

the advertise kex mode is tls13_kex_mode not key_exchange_mode .

And ALLOW_*_RESUMPTION are same with MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE* , no translation code needed.

include/mbedtls/debug.h

library/debug.c

library/ssl_tls13_generic.c

library/ssl_tls13_server.c

library/ssl_tls13_client.c

yuhaoth

Some nitpick comments and one previous comment is not addressed.

Beside that, test cases are expected

library/debug.c

library/ssl_tls13_server.c

tests/opt-testcases/tls13-misc.sh

yuhaoth

Some nitpick comments, others looks good.

library/ssl_misc.h

include/mbedtls/debug.h

include/mbedtls/ssl.h

library/ssl_tls13_server.c

yuhaoth

LGTM

library/ssl_misc.h

include/mbedtls/ssl.h

library/ssl_tls13_server.c

tests/opt-testcases/tls13-misc.sh

library/debug.c

library/ssl_tls13_server.c

ronald-cron-arm · 2023-01-10T12:45:20Z

library/ssl_tls13_server.c

+    ret = MBEDTLS_ERR_SSL_TICKET_INVALID_KEX_MODE;
+    MBEDTLS_SSL_DEBUG_TICKET_FLAGS( 4, session->ticket_flags );
+    if( mbedtls_ssl_tls13_check_kex_modes( ssl,
+            mbedtls_ssl_tls13_session_get_ticket_flags(
+                session, MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_ALL ) ) )
+    {
+        MBEDTLS_SSL_DEBUG_MSG( 3, ( "No suitable key exchange mode" ) );
+        goto exit;
+    }


With this check and if we decide to go ahead with the ticket for session resumption, we do not ensure that the key exchange mode we are eventually going to select is compatible with the ticket. We can have a scenario where the server is configured to support psk_ephemeral and psk, the client claims that it supports both as well in the key exchange modes extension but the ticket (obtained from a previous session) is limited to psk. Then if the client sends the appropriate extensions, in ssl_tls13_determine_key_exchange_mode(), we are going to select psk_ephemeral which is not allowed for the ticket.

Thus I think that:

we should change ssl_tls13_check_psk_ephemeral_key_exchange() to something like:

static int ssl_tls13_check_psk_ephemeral_key_exchange( mbedtls_ssl_context *ssl ) { #if defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL_ENABLED) if( ( ! mbedtls_ssl_conf_tls13_psk_ephemeral_enabled( ssl ) ) || ( ! mbedtls_ssl_tls13_psk_ephemeral_enabled( ssl ) ) || ( ! ssl_tls13_client_hello_has_exts_for_psk_ephemeral_key_exchange( ssl ) ) ) return( 0 ); if( ssl->handshake->resume ) { if( ! mbedtls_ssl_tls13_session_get_ticket_flags( ssl->session_negotiate, MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL ) ) return( 0 ); } return( 1 ); #else ((void) ssl); return( 0 ); #endif }

Change to ssl_tls13_check_psk_key_exchange in a similar way.

And finally here, do something like:

ticket_flags = mbedtls_ssl_session_get_ticket_flags( session, MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_ALL ) ) ); MBEDTLS_SSL_DEBUG_TICKET_FLAGS( 4, ticket_flags ); key_exchanges = 0; if( ( ticket_flags & MBEDTLS_SSL_TLS1_3_TICKET_ALLOW_PSK_EPHEMERAL_RESUMPTION ) && ssl_tls13_check_psk_ephemeral_key_exchange( ssl ) ) key_exchanges |= MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK_EPHEMERAL; } if( ( ticket_flags & MBEDTLS_SSL_TLS1_3_TICKET_ALLOW_PSK_RESUMPTION ) && ssl_tls13_check_psk_key_exchange( ssl ) ) key_exchanges |= MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_PSK; } if( key_exchanges == 0 ) { ret = MBEDTLS_ERR_ERROR_GENERIC_ERROR; MBEDTLS_SSL_DEBUG_MSG( 3, ( "No suitable key exchange mode" ) ); goto exit; }

Note that at this point ssl->handshake->resume is equal to 0 and thus when calling ssl_tls13_check_psk(_ephemeral)_key_exchange here the check on session_negotiate are not done as it should be as session_negotiate is not ready for that.

Good point! I have posted the same concern about this in #6551 (comment). I prefer to resolve it in another PR, how do you think?

Yes that's fine by me to do that in a follow-up PR. I guess with this in the "TLS 1.3 m->m: Resumption with ticket flags, psk_all/psk" tests the selected key exchange mode should be eventually psk and not psa_ephemeral as currently.

Exactly. In the test, the current selected key is psa_ephemeral, but should be psk eventually. I also found that psk/psk and psk/psk_all tests would select ephemeral in the second flight. I think this should also be resolved in the follow-up PR.

Signed-off-by: Pengyu Lv <[email protected]>

Handshake parameter field, tls13_kex_mode is only valid when MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_PSK_ENABLED is set. So, any functions / calls should be guarded by this macros. Signed-off-by: Pengyu Lv <[email protected]>

Signed-off-by: Pengyu Lv <[email protected]>

This commit add test cases to test if the check of kex change mode in SessionTicket works well. Signed-off-by: Pengyu Lv <[email protected]>

Signed-off-by: Pengyu Lv <[email protected]>

Ticket flags is quite generic and may make sense in the future versions of TLS or even in TLS 1.2 with new extensions. This change remane the ticket_flags helper functions with more generic `mbedtls_ssl_session` prefix instead of `mbedtls_ssl_tls13_session`. Signed-off-by: Pengyu Lv <[email protected]>

Return MBEDTLS_ERR_ERROR_GENERIC_ERROR when ticket_flags are not compatible with advertised key exchange mode. Signed-off-by: Pengyu Lv <[email protected]>

The debug helpers printing ticket_flags status are moved to ssl_tls.c and ssl_debug_helpers.h. Signed-off-by: Pengyu Lv <[email protected]>

Now the config dependencies used for ticket_flags test cases are TLS 1.2 specified. Correct them to MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_* Signed-off-by: Pengyu Lv <[email protected]>

Signed-off-by: Pengyu Lv <[email protected]>

lpy4105 · 2023-01-13T04:50:29Z

Force push to due to coding style switch.

ronald-cron-arm

This looks quite good to me. I am just proposing some minor adjustments.

library/ssl_misc.h

library/ssl_debug_helpers.h

tests/opt-testcases/tls13-misc.sh

library/ssl_tls.c

library/ssl_tls13_client.c

ronald-cron-arm · 2023-01-13T15:08:54Z

library/ssl_tls13_server.c

+    ret = MBEDTLS_ERR_SSL_TICKET_INVALID_KEX_MODE;
+    MBEDTLS_SSL_DEBUG_TICKET_FLAGS( 4, session->ticket_flags );
+    if( mbedtls_ssl_tls13_check_kex_modes( ssl,
+            mbedtls_ssl_tls13_session_get_ticket_flags(
+                session, MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_ALL ) ) )
+    {
+        MBEDTLS_SSL_DEBUG_MSG( 3, ( "No suitable key exchange mode" ) );
+        goto exit;
+    }


Yes that's fine by me to do that in a follow-up PR. I guess with this in the "TLS 1.3 m->m: Resumption with ticket flags, psk_all/psk" tests the selected key exchange mode should be eventually psk and not psa_ephemeral as currently.

When ticket_flags used as parameter, use unsigned int, instead of uint8_t or mbedtls_ssl_tls13_ticket_flags.Also remove the definition of mbedtls_ssl_tls13_ticket_flags. Signed-off-by: Pengyu Lv <[email protected]>

Signed-off-by: Pengyu Lv <[email protected]>

This content of the function is moved to ssl_tls13_has_configured_ticket. Signed-off-by: Pengyu Lv <[email protected]>

ronald-cron-arm

This looks good to me now. Thanks.

yuhaoth

LGTM

xkqian comments are all resolved.

lpy4105 added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests component-tls13 priority-medium Medium priority - this can be reviewed as time permits labels Nov 17, 2022

lpy4105 requested review from yuhaoth, ronald-cron-arm and xkqian November 17, 2022 07:46

lpy4105 force-pushed the 6551-tls13-SessionTicket-kex-change-check branch from ced68de to b67a13f Compare November 17, 2022 07:59

yuhaoth suggested changes Nov 17, 2022

View reviewed changes

library/ssl_tls13_server.c Outdated Show resolved Hide resolved

library/ssl_tls13_server.c Outdated Show resolved Hide resolved

library/ssl_tls13_generic.c Outdated Show resolved Hide resolved

xkqian previously requested changes Nov 21, 2022

View reviewed changes

lpy4105 force-pushed the 6551-tls13-SessionTicket-kex-change-check branch from b67a13f to 3c327a8 Compare November 22, 2022 07:31

lpy4105 requested a review from xkqian November 22, 2022 07:32

lpy4105 force-pushed the 6551-tls13-SessionTicket-kex-change-check branch from 3c327a8 to b262295 Compare November 24, 2022 01:59

yuhaoth mentioned this pull request Dec 1, 2022

TLS 1.3: SessionTicket: Enable kex change mode check when resumption #6551

Closed

5 tasks

yuhaoth suggested changes Dec 1, 2022

View reviewed changes

lpy4105 force-pushed the 6551-tls13-SessionTicket-kex-change-check branch from b262295 to 164e11c Compare December 2, 2022 06:59

lpy4105 requested a review from yuhaoth December 2, 2022 07:04

lpy4105 changed the title ~~WIP: TLS 1.3: SessionTicket: Enable kex change mode check when resumption~~ TLS 1.3: SessionTicket: Enable kex change mode check when resumption Dec 2, 2022

lpy4105 force-pushed the 6551-tls13-SessionTicket-kex-change-check branch from 164e11c to 092b4cb Compare December 5, 2022 07:09

yuhaoth suggested changes Dec 7, 2022

View reviewed changes

library/debug.c Outdated Show resolved Hide resolved

library/ssl_tls13_server.c Outdated Show resolved Hide resolved

library/ssl_tls13_server.c Outdated Show resolved Hide resolved

yuhaoth self-requested a review December 9, 2022 07:46

yuhaoth reviewed Dec 9, 2022

View reviewed changes

tests/opt-testcases/tls13-misc.sh Outdated Show resolved Hide resolved

yuhaoth suggested changes Dec 13, 2022

View reviewed changes

library/ssl_misc.h Outdated Show resolved Hide resolved

include/mbedtls/debug.h Outdated Show resolved Hide resolved

include/mbedtls/ssl.h Outdated Show resolved Hide resolved

library/ssl_tls13_server.c Outdated Show resolved Hide resolved

yuhaoth previously approved these changes Dec 14, 2022

View reviewed changes

ronald-cron-arm requested changes Jan 11, 2023

View reviewed changes

ronald-cron-arm removed the request for review from xkqian January 11, 2023 13:26

lpy4105 dismissed yuhaoth’s stale review via 49fc85a January 12, 2023 08:51

lpy4105 requested review from ronald-cron-arm and yuhaoth January 12, 2023 09:05

Add debug helper to print ticket_flags status

5b8dcd2

Signed-off-by: Pengyu Lv <[email protected]>

lpy4105 added 12 commits January 13, 2023 11:05

guard tls13_kex_modes related function calls with macro

e6487fe

Handshake parameter field, tls13_kex_mode is only valid when MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_PSK_ENABLED is set. So, any functions / calls should be guarded by this macros. Signed-off-by: Pengyu Lv <[email protected]>

filter the tickets with tls13_kex_mode on client side.

9356678

Signed-off-by: Pengyu Lv <[email protected]>

add cases to test session resumption with different ticket_flags

302feb3

This commit add test cases to test if the check of kex change mode in SessionTicket works well. Signed-off-by: Pengyu Lv <[email protected]>

improve code format and readability

9eacb44

Signed-off-by: Pengyu Lv <[email protected]>

fix review comments

1735ba3

Signed-off-by: Pengyu Lv <[email protected]>

unroll test cases to improve coverage of check_test_cases in all.sh

06cf66d

Signed-off-by: Pengyu Lv <[email protected]>

fix review comments

a1aa31b

Signed-off-by: Pengyu Lv <[email protected]>

remove MBEDTLS_ERR_SSL_TICKET_INVALID_KEX_MODE error

1894653

Return MBEDTLS_ERR_ERROR_GENERIC_ERROR when ticket_flags are not compatible with advertised key exchange mode. Signed-off-by: Pengyu Lv <[email protected]>

move ticket_flags debug helpers

ee455c0

The debug helpers printing ticket_flags status are moved to ssl_tls.c and ssl_debug_helpers.h. Signed-off-by: Pengyu Lv <[email protected]>

correct test case dependencies

c1334d9

Now the config dependencies used for ticket_flags test cases are TLS 1.2 specified. Correct them to MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_* Signed-off-by: Pengyu Lv <[email protected]>

refine the state setting in tls13_handshake_wrapup

3643fdb

Signed-off-by: Pengyu Lv <[email protected]>

lpy4105 force-pushed the 6551-tls13-SessionTicket-kex-change-check branch from 49fc85a to 3643fdb Compare January 13, 2023 04:49

ronald-cron-arm requested changes Jan 13, 2023

View reviewed changes

lpy4105 added 5 commits January 16, 2023 11:23

make ticket_flags param types consistent

acecf9c

When ticket_flags used as parameter, use unsigned int, instead of uint8_t or mbedtls_ssl_tls13_ticket_flags.Also remove the definition of mbedtls_ssl_tls13_ticket_flags. Signed-off-by: Pengyu Lv <[email protected]>

refine ticket_flags printing helper

4938a56

Signed-off-by: Pengyu Lv <[email protected]>

update docs of ssl_client2 and improve code format

e2f1dbf

Signed-off-by: Pengyu Lv <[email protected]>

simplify test case dependencies and test commands

2bfd716

Signed-off-by: Pengyu Lv <[email protected]>

remove ssl_tls13_has_compat_ticket_flags

9b84ea7

This content of the function is moved to ssl_tls13_has_configured_ticket. Signed-off-by: Pengyu Lv <[email protected]>

lpy4105 requested a review from ronald-cron-arm January 16, 2023 09:49

lpy4105 mentioned this pull request Jan 16, 2023

TLS 1.3: SessionTicket: Check ticket_flags in kex mode determination when resumption #6935

Closed

ronald-cron-arm approved these changes Jan 16, 2023

View reviewed changes

yuhaoth approved these changes Jan 17, 2023

View reviewed changes

lpy4105 added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jan 17, 2023

ronald-cron-arm merged commit 340d4c8 into Mbed-TLS:development Jan 17, 2023

lpy4105 mentioned this pull request Jan 18, 2023

TLS 1.3: SRV: Check ticket_flags on kex mode determination when resumption #6945

Merged

7 tasks

lpy4105 mentioned this pull request Feb 9, 2023

TLS 1.3: SRV: PSK resumption is not selected when ephemeral mode is available #7069

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS 1.3: SessionTicket: Enable kex change mode check when resumption #6616

TLS 1.3: SessionTicket: Enable kex change mode check when resumption #6616

lpy4105 commented Nov 17, 2022 •

edited by ronald-cron-arm

Loading

yuhaoth left a comment

yuhaoth left a comment

yuhaoth left a comment

yuhaoth left a comment

yuhaoth left a comment

ronald-cron-arm Jan 10, 2023

lpy4105 Jan 12, 2023 •

edited

Loading

ronald-cron-arm Jan 13, 2023

lpy4105 Jan 16, 2023

lpy4105 commented Jan 13, 2023

ronald-cron-arm left a comment

ronald-cron-arm Jan 13, 2023

ronald-cron-arm left a comment

yuhaoth left a comment

TLS 1.3: SessionTicket: Enable kex change mode check when resumption #6616

TLS 1.3: SessionTicket: Enable kex change mode check when resumption #6616

Conversation

lpy4105 commented Nov 17, 2022 • edited by ronald-cron-arm Loading

Description

TODOS

Gatekeeper checklist

yuhaoth left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

ronald-cron-arm Jan 10, 2023

Choose a reason for hiding this comment

lpy4105 Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm Jan 13, 2023

Choose a reason for hiding this comment

lpy4105 Jan 16, 2023

Choose a reason for hiding this comment

lpy4105 commented Jan 13, 2023

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm Jan 13, 2023

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

lpy4105 commented Nov 17, 2022 •

edited by ronald-cron-arm

Loading

lpy4105 Jan 12, 2023 •

edited

Loading