Tls13 parse early data indication ee #6490

xkqian · 2022-10-27T10:26:01Z

Parse early data indication in encrypted extentions.
Empty to indicate that early data supported by server side.

Part of adding early data support in TLS 1.3. No need for a change log.

yuhaoth

Some changes to meet latest code

yuhaoth · 2022-11-18T10:29:27Z

library/ssl_tls13_client.c

+#if defined(MBEDTLS_SSL_EARLY_DATA)
+/*
+ * ssl_tls13_parse_ee_early_data_ext()
+ *      Parse early data indication extension in EncryptedExtensions.
+ *
+ * struct {} Empty;
+ *
+ * struct {
+ *   select (Handshake.msg_type) {
+ *     ...
+ *     case client_hello:         Empty;
+ *     case encrypted_extensions: Empty;
+ *   };
+ * } EarlyDataIndication;
+ *
+ */
+
+MBEDTLS_CHECK_RETURN_CRITICAL
+static int ssl_tls13_parse_ee_early_data_ext( mbedtls_ssl_context *ssl,
+                                              const unsigned char *buf,
+                                              size_t len )
+{
+    if( ssl->early_data_status < MBEDTLS_SSL_EARLY_DATA_STATUS_INDICATION_SENT )
+    {
+        /* The server must not send the EarlyDataIndication if the
+         * client hasn't indicated the use of early data. */
+        MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER,
+                                      MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );
+        return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );
+    }
+
+    if( len != 0 )
+    {
+        /* The message must be empty. */
+        MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR,
+                                      MBEDTLS_ERR_SSL_DECODE_ERROR );
+        return( MBEDTLS_ERR_SSL_DECODE_ERROR );
+    }
+
+    /* Nothing to parse */
+    ((void) buf);
+
+    ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;
+    return( 0 );
+}
+#endif /* MBEDTLS_SSL_EARLY_DATA */
+


#6170 has been merged, it is not needed.

Suggested change

#if defined(MBEDTLS_SSL_EARLY_DATA)

/*

* ssl_tls13_parse_ee_early_data_ext()

* Parse early data indication extension in EncryptedExtensions.

*

* struct {} Empty;

*

* struct {

* select (Handshake.msg_type) {

* ...

* case client_hello: Empty;

* case encrypted_extensions: Empty;

* };

* } EarlyDataIndication;

*

*/

MBEDTLS_CHECK_RETURN_CRITICAL

static int ssl_tls13_parse_ee_early_data_ext( mbedtls_ssl_context *ssl,

const unsigned char *buf,

size_t len )

{

if( ssl->early_data_status < MBEDTLS_SSL_EARLY_DATA_STATUS_INDICATION_SENT )

{

/* The server must not send the EarlyDataIndication if the

* client hasn't indicated the use of early data. */

MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER,

MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );

return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );

}

if( len != 0 )

{

/* The message must be empty. */

MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR,

MBEDTLS_ERR_SSL_DECODE_ERROR );

return( MBEDTLS_ERR_SSL_DECODE_ERROR );

}

/* Nothing to parse */

((void) buf);

ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;

return( 0 );

}

#endif /* MBEDTLS_SSL_EARLY_DATA */

Not addressed. Although the code is changed. This point is not addressed

Not addressed. Although the code is changed. This point is not addressed

Which point is not addressed? What I can see is remove all of the code realted with parsing. Do I miss something?

library/ssl_tls13_client.c

yuhaoth · 2022-11-18T10:37:17Z

library/ssl_tls13_client.c

+    /* Nothing to parse */
+    ((void) buf);
+
+    ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;


Put below code in postprocess_encrypted_extesion

Suggested change

ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;

if( ssl->handshake->received_extension & MBEDTLS_SSL_EXT_MASK( EARLY_DATA ) )

ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;

I didn't find the function postprocess_encrypted_extesion(), so I put ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED; in the ssl_tls13_parse_encrypted_extensions().

line 2132.
Not Addressed

I am not sure why we should put the status change in line 2132.
Seems it's more readable and directly if we put the status change "ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;" just follow up the early data extension here.
And, we have no need of the if( ssl->handshake->received_extension & MBEDTLS_SSL_EXT_MASK( EARLY_DATA ) ), which can save the code size.

ronald-cron-arm · 2022-11-22T08:40:08Z

library/ssl_tls13_client.c

@@ -1335,6 +1335,53 @@ static int ssl_tls13_is_downgrade_negotiation( mbedtls_ssl_context *ssl,
    return( 0 );
 }

+#if defined(MBEDTLS_SSL_EARLY_DATA)


The early data indication extension is very similar in NewSessionTicket, ClientHello and EncryptedExtensions thus in that case it would be good to have only one parsing function for the three cases. The function should just do the parsing though thus all the code related to early_data_status currently in this function should be moved to ssl_tls13_parse_encrypted_extensions().

Signed-off-by: Xiaokang Qian <[email protected]>

yuhaoth

Some feedback is not addressed. Do you forget push code?

yuhaoth · 2022-11-23T01:43:26Z

library/ssl_tls13_client.c

+                if( ssl->early_data_status != MBEDTLS_SSL_EARLY_DATA_STATUS_INDICATION_SENT )
+                {
+                    /* The server must not send the EarlyDataIndication if the
+                    * client hasn't indicated the use of early data. */
+                    MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER,
+                                                  MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );
+                    return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );
+                }


The case has been processed by mbedtls_ssl_tls13_check_received_extension

Suggested change

if( ssl->early_data_status != MBEDTLS_SSL_EARLY_DATA_STATUS_INDICATION_SENT )

{

/* The server must not send the EarlyDataIndication if the

* client hasn't indicated the use of early data. */

MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER,

MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );

return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );

}

Seems the code if( ( extension_mask & hs_msg_allowed_extensions_mask ) == 0 ) has processed it.

yuhaoth · 2022-11-23T01:44:09Z

library/ssl_tls13_client.c

+                    return( MBEDTLS_ERR_SSL_DECODE_ERROR );
+                }
+
+	            ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;


Move to line 2132

Suggested change

ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;

if( ssl->handshake->received_extension & MBEDTLS_SSL_EXT_MASK( EARLY_DATA ) )

ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;

yuhaoth · 2022-11-23T01:45:17Z

library/ssl_tls13_client.c

+#if defined(MBEDTLS_SSL_EARLY_DATA)
+/*
+ * ssl_tls13_parse_ee_early_data_ext()
+ *      Parse early data indication extension in EncryptedExtensions.
+ *
+ * struct {} Empty;
+ *
+ * struct {
+ *   select (Handshake.msg_type) {
+ *     ...
+ *     case client_hello:         Empty;
+ *     case encrypted_extensions: Empty;
+ *   };
+ * } EarlyDataIndication;
+ *
+ */
+
+MBEDTLS_CHECK_RETURN_CRITICAL
+static int ssl_tls13_parse_ee_early_data_ext( mbedtls_ssl_context *ssl,
+                                              const unsigned char *buf,
+                                              size_t len )
+{
+    if( ssl->early_data_status < MBEDTLS_SSL_EARLY_DATA_STATUS_INDICATION_SENT )
+    {
+        /* The server must not send the EarlyDataIndication if the
+         * client hasn't indicated the use of early data. */
+        MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER,
+                                      MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );
+        return( MBEDTLS_ERR_SSL_ILLEGAL_PARAMETER );
+    }
+
+    if( len != 0 )
+    {
+        /* The message must be empty. */
+        MBEDTLS_SSL_PEND_FATAL_ALERT( MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR,
+                                      MBEDTLS_ERR_SSL_DECODE_ERROR );
+        return( MBEDTLS_ERR_SSL_DECODE_ERROR );
+    }
+
+    /* Nothing to parse */
+    ((void) buf);
+
+    ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;
+    return( 0 );
+}
+#endif /* MBEDTLS_SSL_EARLY_DATA */
+


Not addressed. Although the code is changed. This point is not addressed

yuhaoth · 2022-11-23T01:46:01Z

library/ssl_tls13_client.c

+    /* Nothing to parse */
+    ((void) buf);
+
+    ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;


line 2132.
Not Addressed

Signed-off-by: Xiaokang Qian <[email protected]>

yuhaoth

LGTM

ronald-cron-arm

LGTM

xkqian added needs-work needs-ci Needs to pass CI tests component-tls13 labels Oct 27, 2022

xkqian linked an issue Oct 27, 2022 that may be closed by this pull request

TLS 1.3 client: Parsing of the early data indication extension #6328

Closed

1 task

xkqian force-pushed the tls13_parse_early_data_indication_ee branch from 68e5af4 to b4e4ec7 Compare October 27, 2022 10:45

daverodgman added priority-medium Medium priority - this can be reviewed as time permits priority-high High priority - will be reviewed soon and removed priority-medium Medium priority - this can be reviewed as time permits labels Oct 27, 2022

xkqian force-pushed the tls13_parse_early_data_indication_ee branch 2 times, most recently from 9557318 to f35dbb6 Compare November 3, 2022 02:51

xkqian force-pushed the tls13_parse_early_data_indication_ee branch 2 times, most recently from 9f313c0 to f0e5687 Compare November 18, 2022 03:32

xkqian mentioned this pull request Nov 18, 2022

Tls13 write end of early data #6499

Merged

xkqian requested review from ronald-cron-arm and yuhaoth November 18, 2022 03:34

xkqian added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Nov 18, 2022

yuhaoth suggested changes Nov 18, 2022

View reviewed changes

ronald-cron-arm requested changes Nov 22, 2022

View reviewed changes

Add parse function for early data in encrypted extentions

8bee899

Signed-off-by: Xiaokang Qian <[email protected]>

xkqian force-pushed the tls13_parse_early_data_indication_ee branch from f0e5687 to 30dd204 Compare November 22, 2022 10:06

yuhaoth suggested changes Nov 23, 2022

View reviewed changes

Remove useless function and parse early data in ee

ca09afc

Signed-off-by: Xiaokang Qian <[email protected]>

xkqian force-pushed the tls13_parse_early_data_indication_ee branch from 30dd204 to ca09afc Compare November 23, 2022 02:17

xkqian requested review from yuhaoth and ronald-cron-arm November 23, 2022 02:17

xkqian added 2 commits November 23, 2022 03:21

Remove the duplicate early_data_status check

e861ba0

Signed-off-by: Xiaokang Qian <[email protected]>

Move the early data status set afeter all of the extensions parse

b157e91

Signed-off-by: Xiaokang Qian <[email protected]>

yuhaoth approved these changes Nov 23, 2022

View reviewed changes

ronald-cron-arm approved these changes Nov 23, 2022

View reviewed changes

ronald-cron-arm merged commit 1d1d536 into Mbed-TLS:development Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tls13 parse early data indication ee #6490

Tls13 parse early data indication ee #6490

xkqian commented Oct 27, 2022 •

edited by ronald-cron-arm

Loading

yuhaoth left a comment

yuhaoth Nov 18, 2022

yuhaoth Nov 23, 2022

xkqian Nov 23, 2022

yuhaoth Nov 18, 2022 •

edited

Loading

xkqian Nov 22, 2022

yuhaoth Nov 23, 2022

xkqian Nov 23, 2022

ronald-cron-arm Nov 22, 2022

yuhaoth left a comment

yuhaoth Nov 23, 2022

xkqian Nov 23, 2022 •

edited

Loading

yuhaoth Nov 23, 2022

yuhaoth Nov 23, 2022

yuhaoth Nov 23, 2022

yuhaoth left a comment

ronald-cron-arm left a comment

	ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;
	if( ssl->handshake->received_extension & MBEDTLS_SSL_EXT_MASK( EARLY_DATA ) )
	ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;

Tls13 parse early data indication ee #6490

Tls13 parse early data indication ee #6490

Conversation

xkqian commented Oct 27, 2022 • edited by ronald-cron-arm Loading

yuhaoth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhaoth Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xkqian Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhaoth left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

xkqian commented Oct 27, 2022 •

edited by ronald-cron-arm

Loading

yuhaoth Nov 18, 2022 •

edited

Loading

xkqian Nov 23, 2022 •

edited

Loading