From b03f88f06ce03875464a244388e7d8ad980b8be3 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 24 Nov 2020 06:41:37 +0000 Subject: [PATCH 1/4] Introduce helper for handling of post-handshake handshake messages Handling the receipt of a handshake record after the initial handshake requires non-trivial logic depending on the protocol version and the endpoint. This logic is currently embedded in mbedtls_ssl_read(). With the introduction of support for [D]TLS 1.3, the logic will become even more complex, since [D]TLS 1.3 drops support for renegotiation -- which in [D]TLS 1.2 is the main purpose of post-handshake handshake messages -- but instead introduces numerous other post-handshake handshake messages. In order to pave the way for those changes, this commit improves readability and maintainability of mbedtls_ssl_read() by moving the TLS <=1.2 logic for handling post-handshake handshake messages into a separate helper function ssl_handle_hs_message_post_handshake(). The logic of the code is entirely unchanged. Signed-off-by: Hanno Becker --- library/ssl_msg.c | 235 +++++++++++++++++++++++----------------------- 1 file changed, 120 insertions(+), 115 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 51a0ac2051..247dd1a96c 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5112,6 +5112,120 @@ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_RENEGOTIATION */ +/* This function is called from mbedtls_ssl_read() when a handshake message is + * received after the initial handshake. In this context, handshake messages + * may only be sent for the purpose of initiating renegotiations. + * + * This function is introduced as a separate helper since the handling + * of post-handshake handshake messages changes significantly in TLS 1.3, + * and having a helper function allows to distinguish between TLS <= 1.2 and + * TLS 1.3 in the future without bloating the logic of mbedtls_ssl_read(). + */ +int ssl_handle_hs_message_post_handshake( mbedtls_ssl_context *ssl ) +{ + int ret; + + /* + * - For client-side, expect SERVER_HELLO_REQUEST. + * - For server-side, expect CLIENT_HELLO. + * - Fail (TLS) or silently drop record (DTLS) in other cases. + */ + +#if defined(MBEDTLS_SSL_CLI_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && + ( ssl->in_msg[0] != MBEDTLS_SSL_HS_HELLO_REQUEST || + ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) ) ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "handshake received (not HelloRequest)" ) ); + + /* With DTLS, drop the packet (probably from last handshake) */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + return( 0 ); + } +#endif + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + } +#endif /* MBEDTLS_SSL_CLI_C */ + +#if defined(MBEDTLS_SSL_SRV_C) + if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && + ssl->in_msg[0] != MBEDTLS_SSL_HS_CLIENT_HELLO ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "handshake received (not ClientHello)" ) ); + + /* With DTLS, drop the packet (probably from last handshake) */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + { + return( 0 ); + } +#endif + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + } +#endif /* MBEDTLS_SSL_SRV_C */ + +#if defined(MBEDTLS_SSL_RENEGOTIATION) + /* Determine whether renegotiation attempt should be accepted */ + if( ! ( ssl->conf->disable_renegotiation == MBEDTLS_SSL_RENEGOTIATION_DISABLED || + ( ssl->secure_renegotiation == MBEDTLS_SSL_LEGACY_RENEGOTIATION && + ssl->conf->allow_legacy_renegotiation == + MBEDTLS_SSL_LEGACY_NO_RENEGOTIATION ) ) ) + { + /* + * Accept renegotiation request + */ + + /* DTLS clients need to know renego is server-initiated */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && + ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) + { + ssl->renego_status = MBEDTLS_SSL_RENEGOTIATION_PENDING; + } +#endif + ret = mbedtls_ssl_start_renegotiation( ssl ); + if( ret != MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && + ret != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_start_renegotiation", + ret ); + return( ret ); + } + } + else +#endif /* MBEDTLS_SSL_RENEGOTIATION */ + { + /* + * Refuse renegotiation + */ + + MBEDTLS_SSL_DEBUG_MSG( 3, ( "refusing renegotiation, sending alert" ) ); + +#if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) + if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_1 ) + { + if( ( ret = mbedtls_ssl_send_alert_message( ssl, + MBEDTLS_SSL_ALERT_LEVEL_WARNING, + MBEDTLS_SSL_ALERT_MSG_NO_RENEGOTIATION ) ) != 0 ) + { + return( ret ); + } + } + else +#endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || + MBEDTLS_SSL_PROTO_TLS1_2 */ + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + } + + return( 0 ); +} + /* * Receive application data decrypted from the SSL layer */ @@ -5210,124 +5324,15 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "received handshake message" ) ); - - /* - * - For client-side, expect SERVER_HELLO_REQUEST. - * - For server-side, expect CLIENT_HELLO. - * - Fail (TLS) or silently drop record (DTLS) in other cases. - */ - -#if defined(MBEDTLS_SSL_CLI_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && - ( ssl->in_msg[0] != MBEDTLS_SSL_HS_HELLO_REQUEST || - ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) ) ) + ret = ssl_handle_hs_message_post_handshake( ssl ); + if( ret != 0) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "handshake received (not HelloRequest)" ) ); - - /* With DTLS, drop the packet (probably from last handshake) */ -#if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) - { - continue; - } -#endif - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - } -#endif /* MBEDTLS_SSL_CLI_C */ - -#if defined(MBEDTLS_SSL_SRV_C) - if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && - ssl->in_msg[0] != MBEDTLS_SSL_HS_CLIENT_HELLO ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "handshake received (not ClientHello)" ) ); - - /* With DTLS, drop the packet (probably from last handshake) */ -#if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) - { - continue; - } -#endif - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - } -#endif /* MBEDTLS_SSL_SRV_C */ - -#if defined(MBEDTLS_SSL_RENEGOTIATION) - /* Determine whether renegotiation attempt should be accepted */ - if( ! ( ssl->conf->disable_renegotiation == MBEDTLS_SSL_RENEGOTIATION_DISABLED || - ( ssl->secure_renegotiation == MBEDTLS_SSL_LEGACY_RENEGOTIATION && - ssl->conf->allow_legacy_renegotiation == - MBEDTLS_SSL_LEGACY_NO_RENEGOTIATION ) ) ) - { - /* - * Accept renegotiation request - */ - - /* DTLS clients need to know renego is server-initiated */ -#if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && - ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT ) - { - ssl->renego_status = MBEDTLS_SSL_RENEGOTIATION_PENDING; - } -#endif - ret = mbedtls_ssl_start_renegotiation( ssl ); - if( ret != MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && - ret != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_start_renegotiation", - ret ); - return( ret ); - } - } - else -#endif /* MBEDTLS_SSL_RENEGOTIATION */ - { - /* - * Refuse renegotiation - */ - - MBEDTLS_SSL_DEBUG_MSG( 3, ( "refusing renegotiation, sending alert" ) ); - -#if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ - defined(MBEDTLS_SSL_PROTO_TLS1_2) - if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_1 ) - { - if( ( ret = mbedtls_ssl_send_alert_message( ssl, - MBEDTLS_SSL_ALERT_LEVEL_WARNING, - MBEDTLS_SSL_ALERT_MSG_NO_RENEGOTIATION ) ) != 0 ) - { - return( ret ); - } - } - else -#endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || - MBEDTLS_SSL_PROTO_TLS1_2 */ - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); - return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); - } + MBEDTLS_SSL_DEBUG_RET( 1, "ssl_handle_hs_message_post_handshake", + ret ); + return( ret ); } - /* At this point, we don't know whether the renegotiation has been - * completed or not. The cases to consider are the following: - * 1) The renegotiation is complete. In this case, no new record - * has been read yet. - * 2) The renegotiation is incomplete because the client received - * an application data record while awaiting the ServerHello. - * 3) The renegotiation is incomplete because the client received - * a non-handshake, non-application data message while awaiting - * the ServerHello. - * In each of these case, looping will be the proper action: - * - For 1), the next iteration will read a new record and check - * if it's application data. - * - For 2), the loop condition isn't satisfied as application data - * is present, hence continue is the same as break - * - For 3), the loop condition is satisfied and read_record - * will re-deliver the message that was held back by the client - * when expecting the ServerHello. - */ + /* Post-handshake handshake messages are not passed to the user. */ continue; } #if defined(MBEDTLS_SSL_RENEGOTIATION) From cad3dbaf458b58702a6724204a02782df15877b7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 24 Nov 2020 06:57:13 +0000 Subject: [PATCH 2/4] Add missing static qualification for post-HS HS message handler Signed-off-by: Hanno Becker --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 247dd1a96c..33751c72af 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5121,7 +5121,7 @@ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) * and having a helper function allows to distinguish between TLS <= 1.2 and * TLS 1.3 in the future without bloating the logic of mbedtls_ssl_read(). */ -int ssl_handle_hs_message_post_handshake( mbedtls_ssl_context *ssl ) +static int ssl_handle_hs_message_post_handshake( mbedtls_ssl_context *ssl ) { int ret; From fae12cf1ef9b18e9ec10e711bc2d48c74c09dce1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 21 Apr 2021 07:20:20 +0100 Subject: [PATCH 3/4] Use error corruption detection as initial return value Signed-off-by: Hanno Becker --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 33751c72af..826c53239e 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5123,7 +5123,7 @@ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) */ static int ssl_handle_hs_message_post_handshake( mbedtls_ssl_context *ssl ) { - int ret; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; /* * - For client-side, expect SERVER_HELLO_REQUEST. From f26cc72e7b164c50fbcbe0e4dbb6cf056930b1df Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 21 Apr 2021 07:30:13 +0100 Subject: [PATCH 4/4] Reintroduce comment on state of renegotiation after post HS message Signed-off-by: Hanno Becker --- library/ssl_msg.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 826c53239e..a2d19f64d9 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5113,7 +5113,7 @@ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_SSL_RENEGOTIATION */ /* This function is called from mbedtls_ssl_read() when a handshake message is - * received after the initial handshake. In this context, handshake messages + * received after the initial handshake. In this context, handshake messages * may only be sent for the purpose of initiating renegotiations. * * This function is introduced as a separate helper since the handling @@ -5332,7 +5332,27 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) return( ret ); } - /* Post-handshake handshake messages are not passed to the user. */ + /* At this point, we don't know whether the renegotiation triggered + * by the post-handshake message has been completed or not. The cases + * to consider are the following: + * 1) The renegotiation is complete. In this case, no new record + * has been read yet. + * 2) The renegotiation is incomplete because the client received + * an application data record while awaiting the ServerHello. + * 3) The renegotiation is incomplete because the client received + * a non-handshake, non-application data message while awaiting + * the ServerHello. + * + * In each of these cases, looping will be the proper action: + * - For 1), the next iteration will read a new record and check + * if it's application data. + * - For 2), the loop condition isn't satisfied as application data + * is present, hence continue is the same as break + * - For 3), the loop condition is satisfied and read_record + * will re-deliver the message that was held back by the client + * when expecting the ServerHello. + */ + continue; } #if defined(MBEDTLS_SSL_RENEGOTIATION)