Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLS 1.3: Refine and test client early data status #8755

Merged
merged 12 commits into from
Feb 13, 2024
Merged

TLS 1.3: Refine and test client early data status #8755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
fb53647
tests: ssl: Move group list to options
ronald-cron-arm Jan 26, 2024
b4ad3e7
tests: ssl: First reset to all zeroes options in init
ronald-cron-arm Jan 26, 2024
ced99be
tests: ssl: Add early data handshake option
ronald-cron-arm Jan 26, 2024
1f6e4e4
tests: ssl: Add helper function to get a TLS 1.3 ticket
ronald-cron-arm Jan 26, 2024
bfcdc06
tests: ssl: Use get TLS 1.3 ticket helper for early data test
ronald-cron-arm Jan 26, 2024
fe59ff7
tls13: Send dummy CCS only once
ronald-cron-arm Jan 24, 2024
90e2233
tls13: cli: Refine early data status
ronald-cron-arm Jan 22, 2024
a7f94e4
tests: ssl: Add early data status unit test
ronald-cron-arm Jan 24, 2024
5c208d7
tests: ssl: Add scenario param to early data status testing function
ronald-cron-arm Jan 24, 2024
265273e
tests: early data status: Add "not sent" scenario
ronald-cron-arm Jan 24, 2024
d6dba67
tests: early data status: Add "server rejects" scenario
ronald-cron-arm Jan 24, 2024
2261ab2
tests: early data status: Add HRR scenario
ronald-cron-arm Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/mbedtls/ssl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5106,9 +5106,9 @@ int mbedtls_ssl_close_notify(mbedtls_ssl_context *ssl);

#if defined(MBEDTLS_SSL_EARLY_DATA)

#define MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT 0
#define MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED 1
#define MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED 2
#define MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-exisiting: it would be good to have comment here like we have for the other values defined in ssl_misc.h. In particular, out of context it might not be clear if STATUS_NOT_SENT means the extension has not been sent, or early data itself has not been sent.

Btw, maybe renaming to STATUS_NOT_OFFERED or STATUS_EXT_NOT_SENT would make that clearer? (In addition to comments I mean.)

Edit, after reading more of the code: IIUC this actually means "we decided not to send it" as opposed to "we haven't sent it yet but might send it in the future". This only strengthens my point about adding a comment to make that clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(pre-existing, minor) feels like this should be an enum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #8755 (comment)

#define MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED 2
#define MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED 3

#if defined(MBEDTLS_SSL_SRV_C)
/**
Expand Down
47 changes: 44 additions & 3 deletions library/ssl_misc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,14 +731,23 @@ struct mbedtls_ssl_handshake_params {
uint8_t key_exchange_mode; /*!< Selected key exchange mode */

/** Number of HelloRetryRequest messages received/sent from/to the server. */
int hello_retry_request_count;
uint8_t hello_retry_request_count;

#if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE)
/**
* Number of dummy change_cipher_spec (CCS) record sent. Used to send only
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(minor, fix in follow-up) should be plural records sent. Also, there seem to be two spaces after the * on each line :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment changed in #8760

* one CCS per handshake without having to complicate the handshake state
* transitions.
*/
uint8_t ccs_count;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the name slightly misleading, since we expect to send no more than once. I'd prefer ccs_sent. (Said otherwise, if an integer variable's only legal values are 0 and 1, I prefer to think of it as a boolean rather than a counter.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Manuel, but minor, can be done in a follow-up PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree as well, done in #8760 commit "tls13: Use a flag not a counter for CCS and HRR handling".

#endif

#if defined(MBEDTLS_SSL_SRV_C)
/** selected_group of key_share extension in HelloRetryRequest message. */
uint16_t hrr_selected_group;
#if defined(MBEDTLS_SSL_TLS1_3_KEY_EXCHANGE_MODE_SOME_PSK_ENABLED)
uint8_t tls13_kex_modes; /*!< Key exchange modes supported by the client */
#endif
/** selected_group of key_share extension in HelloRetryRequest message. */
uint16_t hrr_selected_group;
#if defined(MBEDTLS_SSL_SESSION_TICKETS)
uint16_t new_session_tickets_count; /*!< number of session tickets */
#endif
Expand Down Expand Up @@ -2136,6 +2145,38 @@ int mbedtls_ssl_tls13_write_early_data_ext(mbedtls_ssl_context *ssl,
unsigned char *buf,
const unsigned char *end,
size_t *out_len);

#if defined(MBEDTLS_SSL_CLI_C)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the definitions in ssl.h are not guarded by this, should they be?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(minor) I'd prefer to see them defined in one place, even if values are defined that aren't used under some circumstances - they are well-enough namespaced

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #8755 (comment)

/*
* The client has not sent the first ClientHello yet, it is unknown if the
* client will send an early data indication extension or not.
*/
#define MBEDTLS_SSL_EARLY_DATA_STATUS_UNKNOWN 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the documentation for the early_data_status field still says Reset to #MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT - does it need updating?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #8760, commit "tls13: Fix/Improve comments"


/*
* The client has sent an early data indication extension in its first
* ClientHello, it has not received the response (ServerHello or
* HelloRetryRequest) from the server yet. The transform to protect early data
* is not set and early data cannot be sent yet.
*/
#define MBEDTLS_SSL_EARLY_DATA_STATUS_SENT 4

/*
* The client has sent an early data indication extension in its first
* ClientHello, it has not received the response (ServerHello or
* HelloRetryRequest) from the server yet. The transform to protect early data
* has been set and early data can be written now.
*/
#define MBEDTLS_SSL_EARLY_DATA_STATUS_CAN_WRITE 5

/*
* The client has sent an early data indication extension in its first
* ClientHello, the server has accepted them and the client has received the
* server Finished message. It cannot send early data to the server anymore.
*/
#define MBEDTLS_SSL_EARLY_DATA_STATUS_SERVER_FINISHED_RECEIVED 6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(for follow-up) If these are all possible values of early_data_status i.e. return values from mbedtls_ssl_get_early_data_status() could they be defined together, ideally in an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mbedtls_ssl_get_early_data_status() returns only NOT_SENT or ACCEPTED or REJECTED. The other values are for internal use only. See also #8755 (comment).

#endif /* MBEDTLS_SSL_CLI_C */

#endif /* MBEDTLS_SSL_EARLY_DATA */

#endif /* MBEDTLS_SSL_PROTO_TLS1_3 */
Expand Down
2 changes: 1 addition & 1 deletion library/ssl_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,7 +1100,7 @@ static int ssl_handshake_init(mbedtls_ssl_context *ssl)

#if defined(MBEDTLS_SSL_EARLY_DATA)
#if defined(MBEDTLS_SSL_CLI_C)
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT;
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_UNKNOWN;
#endif
#if defined(MBEDTLS_SSL_SRV_C)
ssl->discard_early_data_record = MBEDTLS_SSL_EARLY_DATA_NO_DISCARD;
Expand Down
68 changes: 40 additions & 28 deletions library/ssl_tls13_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1180,26 +1180,21 @@ int mbedtls_ssl_tls13_write_client_hello_exts(mbedtls_ssl_context *ssl,
#endif

#if defined(MBEDTLS_SSL_EARLY_DATA)
if (mbedtls_ssl_conf_tls13_is_some_psk_enabled(ssl) &&
ssl_tls13_early_data_has_valid_ticket(ssl) &&
ssl->conf->early_data_enabled == MBEDTLS_SSL_EARLY_DATA_ENABLED &&
ssl->handshake->hello_retry_request_count == 0) {
if (ssl->handshake->hello_retry_request_count == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but: can there really be more that on HRR per handshake? If not, I'd also suggest making that a boolean instead as I think that count may mislead readers into thinking there may be more than one. (I'll agree if you say that's out of scope.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, done in #8760 commit "tls13: Use a flag not a counter for CCS and HRR handling".

if (mbedtls_ssl_conf_tls13_is_some_psk_enabled(ssl) &&
Copy link
Contributor

@mpg mpg Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[edit: skip to last paragraph, only requesting adding a comment after all]

I'm not sure I understand or agree with the logic here:

if no_hrr:
   if other_conditions:
      send it and mark as sent
   else:
      mark as not sent

So, it there was an HRR in this handshake, we will not send the extension and not update the status to "not sent" either? Is that intentional? I'd have expected:

if no_hrr and other_conditions:
   send it and mark as sent
else:
   mark as not sent

What am I missing?

Edit, after reading more of the code: I think what I was missing is that if there was an HRR earlier, status has already been updated to "rejected" and we want to leave it at that. So, now I think I understand and agree with the logic here, but a comment would be welcome.

Copy link
Contributor Author

@ronald-cron-arm ronald-cron-arm Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #8760 commit "tls13: client: Add comment about early data in 2nd ClientHello".

ssl_tls13_early_data_has_valid_ticket(ssl) &&
ssl->conf->early_data_enabled == MBEDTLS_SSL_EARLY_DATA_ENABLED) {
ret = mbedtls_ssl_tls13_write_early_data_ext(
ssl, 0, p, end, &ext_len);
if (ret != 0) {
return ret;
}
p += ext_len;

ret = mbedtls_ssl_tls13_write_early_data_ext(
ssl, 0, p, end, &ext_len);
if (ret != 0) {
return ret;
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_SENT;
} else {
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT;
}
p += ext_len;

/* Initializes the status to `rejected`. It will be updated to
* `accepted` if the EncryptedExtension message contain an early data
* indication extension.
*/
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED;
} else {
MBEDTLS_SSL_DEBUG_MSG(2, ("<= skip write early_data extension"));
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT;
}
#endif /* MBEDTLS_SSL_EARLY_DATA */

Expand Down Expand Up @@ -1236,7 +1231,7 @@ int mbedtls_ssl_tls13_finalize_client_hello(mbedtls_ssl_context *ssl)
size_t psk_len;
const mbedtls_ssl_ciphersuite_t *ciphersuite_info;

if (ssl->early_data_status == MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED) {
if (ssl->early_data_status == MBEDTLS_SSL_EARLY_DATA_STATUS_SENT) {
MBEDTLS_SSL_DEBUG_MSG(
1, ("Set hs psk for early data when writing the first psk"));

Expand Down Expand Up @@ -1299,6 +1294,7 @@ int mbedtls_ssl_tls13_finalize_client_hello(mbedtls_ssl_context *ssl)
1, ("Switch to early data keys for outbound traffic"));
mbedtls_ssl_set_outbound_transform(
ssl, ssl->handshake->transform_earlydata);
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_CAN_WRITE;
#endif
}
#endif /* MBEDTLS_SSL_EARLY_DATA */
Expand Down Expand Up @@ -1971,6 +1967,13 @@ static int ssl_tls13_postprocess_hrr(mbedtls_ssl_context *ssl)
}

ssl->session_negotiate->ciphersuite = ssl->handshake->ciphersuite_info->id;

#if defined(MBEDTLS_SSL_EARLY_DATA)
if (ssl->early_data_status != MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT) {
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED;
}
#endif

return 0;
}

Expand Down Expand Up @@ -2230,6 +2233,8 @@ static int ssl_tls13_process_encrypted_extensions(mbedtls_ssl_context *ssl)
}

ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED;
} else if (ssl->early_data_status != MBEDTLS_SSL_EARLY_DATA_STATUS_NOT_SENT) {
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED;
}
#endif

Expand Down Expand Up @@ -2567,9 +2572,8 @@ static int ssl_tls13_process_server_finished(mbedtls_ssl_context *ssl)

#if defined(MBEDTLS_SSL_EARLY_DATA)
if (ssl->early_data_status == MBEDTLS_SSL_EARLY_DATA_STATUS_ACCEPTED) {
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_SERVER_FINISHED_RECEIVED;
mbedtls_ssl_handshake_set_state(ssl, MBEDTLS_SSL_END_OF_EARLY_DATA);
} else if (ssl->early_data_status == MBEDTLS_SSL_EARLY_DATA_STATUS_REJECTED) {
mbedtls_ssl_handshake_set_state(ssl, MBEDTLS_SSL_CLIENT_CERTIFICATE);
} else
#endif /* MBEDTLS_SSL_EARLY_DATA */
{
Expand Down Expand Up @@ -3059,18 +3063,25 @@ int mbedtls_ssl_tls13_handshake_client_step(mbedtls_ssl_context *ssl)
*/
#if defined(MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE)
case MBEDTLS_SSL_CLIENT_CCS_BEFORE_2ND_CLIENT_HELLO:
ret = mbedtls_ssl_tls13_write_change_cipher_spec(ssl);
if (ret == 0) {
mbedtls_ssl_handshake_set_state(ssl, MBEDTLS_SSL_CLIENT_HELLO);
ret = 0;
if (ssl->handshake->ccs_count == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I think we could put that check inside mbedtls_ssl_tls13_write_change_cipher_spec() (which would do nothing and return 0 if CCS has already been sent). This would avoid having to repeat this at each call site, saving a few bytes of code size but more importantly making sure we don't forget the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #8760 commit "tls13: client: Improve CCS handling"

ret = mbedtls_ssl_tls13_write_change_cipher_spec(ssl);
if (ret != 0) {
break;
}
}
mbedtls_ssl_handshake_set_state(ssl, MBEDTLS_SSL_CLIENT_HELLO);
break;

case MBEDTLS_SSL_CLIENT_CCS_AFTER_SERVER_FINISHED:
ret = mbedtls_ssl_tls13_write_change_cipher_spec(ssl);
if (ret == 0) {
mbedtls_ssl_handshake_set_state(
ssl, MBEDTLS_SSL_CLIENT_CERTIFICATE);
ret = 0;
if (ssl->handshake->ccs_count == 0) {
ret = mbedtls_ssl_tls13_write_change_cipher_spec(ssl);
if (ret != 0) {
break;
}
}
mbedtls_ssl_handshake_set_state(ssl, MBEDTLS_SSL_CLIENT_CERTIFICATE);
break;

#if defined(MBEDTLS_SSL_EARLY_DATA)
Expand All @@ -3083,6 +3094,7 @@ int mbedtls_ssl_tls13_handshake_client_step(mbedtls_ssl_context *ssl)
1, ("Switch to early data keys for outbound traffic"));
mbedtls_ssl_set_outbound_transform(
ssl, ssl->handshake->transform_earlydata);
ssl->early_data_status = MBEDTLS_SSL_EARLY_DATA_STATUS_CAN_WRITE;
}
break;
#endif /* MBEDTLS_SSL_EARLY_DATA */
Expand Down
2 changes: 2 additions & 0 deletions library/ssl_tls13_generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1390,6 +1390,8 @@ int mbedtls_ssl_tls13_write_change_cipher_spec(mbedtls_ssl_context *ssl)
/* Dispatch message */
MBEDTLS_SSL_PROC_CHK(mbedtls_ssl_write_record(ssl, 0));

ssl->handshake->ccs_count++;

cleanup:

MBEDTLS_SSL_DEBUG_MSG(2, ("<= write change cipher spec"));
Expand Down
10 changes: 7 additions & 3 deletions library/ssl_tls13_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3482,10 +3482,14 @@ int mbedtls_ssl_tls13_handshake_server_step(mbedtls_ssl_context *ssl)
break;

case MBEDTLS_SSL_SERVER_CCS_AFTER_SERVER_HELLO:
ret = mbedtls_ssl_tls13_write_change_cipher_spec(ssl);
if (ret == 0) {
mbedtls_ssl_handshake_set_state(ssl, MBEDTLS_SSL_ENCRYPTED_EXTENSIONS);
ret = 0;
if (ssl->handshake->ccs_count == 0) {
ret = mbedtls_ssl_tls13_write_change_cipher_spec(ssl);
if (ret != 0) {
break;
}
}
mbedtls_ssl_handshake_set_state(ssl, MBEDTLS_SSL_ENCRYPTED_EXTENSIONS);
break;
#endif /* MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE */

Expand Down
16 changes: 14 additions & 2 deletions tests/include/test/ssl_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ typedef struct mbedtls_test_ssl_log_pattern {

typedef struct mbedtls_test_handshake_test_options {
const char *cipher;
uint16_t *group_list;
mbedtls_ssl_protocol_version client_min_version;
mbedtls_ssl_protocol_version client_max_version;
mbedtls_ssl_protocol_version server_min_version;
Expand Down Expand Up @@ -112,6 +113,7 @@ typedef struct mbedtls_test_handshake_test_options {
void (*srv_log_fun)(void *, int, const char *, int, const char *);
void (*cli_log_fun)(void *, int, const char *, int, const char *);
int resize_buffers;
int early_data;
#if defined(MBEDTLS_SSL_CACHE_C)
mbedtls_ssl_cache_context *cache;
#endif
Expand Down Expand Up @@ -440,8 +442,7 @@ int mbedtls_test_ssl_endpoint_init(
mbedtls_test_handshake_test_options *options,
mbedtls_test_message_socket_context *dtls_context,
mbedtls_test_ssl_message_queue *input_queue,
mbedtls_test_ssl_message_queue *output_queue,
uint16_t *group_list);
mbedtls_test_ssl_message_queue *output_queue);

/*
* Deinitializes endpoint represented by \p ep.
Expand Down Expand Up @@ -599,6 +600,17 @@ int mbedtls_test_ticket_parse(void *p_ticket, mbedtls_ssl_session *session,
unsigned char *buf, size_t len);
#endif /* MBEDTLS_SSL_SESSION_TICKETS */

#if defined(MBEDTLS_SSL_CLI_C) && defined(MBEDTLS_SSL_SRV_C) && \
defined(MBEDTLS_SSL_PROTO_TLS1_3) && defined(MBEDTLS_SSL_SESSION_TICKETS) && \
defined(MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED)
int mbedtls_test_get_tls13_ticket(
mbedtls_test_handshake_test_options *client_options,
mbedtls_test_handshake_test_options *server_options,
mbedtls_ssl_session *session);
#endif /* MBEDTLS_SSL_CLI_C && MBEDTLS_SSL_SRV_C &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(minor) we don't really need the list of conditions in the #endif since it's so close to the #if - it clutters the text, and if the condition needs changing in the future it's an unnecessary thing to have to keep in sync

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #8760, commit "tls13: Fix/Improve comments"

MBEDTLS_SSL_PROTO_TLS1_3 && MBEDTLS_SSL_SESSION_TICKETS &&
MBEDTLS_SSL_HANDSHAKE_WITH_CERT_ENABLED */

#define ECJPAKE_TEST_PWD "bla"

#if defined(MBEDTLS_USE_PSA_CRYPTO)
Expand Down
Loading