From dc918c5266fb62e065cdfe3736c7c13400a8afd4 Mon Sep 17 00:00:00 2001 From: Tiejun Zhou Date: Wed, 3 Aug 2022 05:37:32 +0000 Subject: - Add check for overflow of the 64-bit sequence number. - Clear entire key material when TLS session resets - Check expiration for all the certificates in the certificate chain. - Correct alert message for downgrade protection --- nx_secure/inc/nx_secure_tls.h | 7 +++++- nx_secure/inc/nx_secure_x509.h | 8 +++++-- nx_secure/src/nx_secure_dtls_send_record.c | 13 ++++++++++- nx_secure/src/nx_secure_tls_1_3_client_handshake.c | 8 +++++-- nx_secure/src/nx_secure_tls_key_material_init.c | 27 +++++----------------- nx_secure/src/nx_secure_tls_map_error_to_alert.c | 7 +++++- nx_secure/src/nx_secure_tls_process_record.c | 13 ++++++++++- .../src/nx_secure_tls_remote_certificate_verify.c | 16 +++++-------- nx_secure/src/nx_secure_tls_send_record.c | 13 ++++++++++- nx_secure/src/nx_secure_tls_verify_mac.c | 23 +++++++++++++++++- .../src/nx_secure_x509_certificate_chain_verify.c | 16 +++++++++++-- .../src/nx_secure_x509_crl_revocation_check.c | 8 +++++-- 12 files changed, 114 insertions(+), 45 deletions(-) diff --git a/nx_secure/inc/nx_secure_tls.h b/nx_secure/inc/nx_secure_tls.h index 858267de..a9576dc9 100644 --- a/nx_secure/inc/nx_secure_tls.h +++ b/nx_secure/inc/nx_secure_tls.h @@ -26,7 +26,7 @@ /* COMPONENT DEFINITION RELEASE */ /* */ /* nx_secure_tls.h PORTABLE C */ -/* 6.1.11 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -85,6 +85,10 @@ /* 04-25-2022 Yuxin Zhou Modified comment(s), and */ /* enabled AEAD for TLS 1.3, */ /* resulting in version 6.1.11 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* updated alert message for */ +/* downgrade protection, */ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ @@ -289,6 +293,7 @@ extern "C" { #define NX_SECURE_TLS_RECORD_OVERFLOW 0x151 /* Received a TLSCiphertext record that had a length too long. */ #define NX_SECURE_TLS_HANDSHAKE_FRAGMENT_RECEIVED 0x152 /* Received a fragmented handshake message - take appropriate action at a higher level of the state machine. */ #define NX_SECURE_TLS_TRANSMIT_LOCKED 0x153 /* Another thread is transmitting. */ +#define NX_SECURE_TLS_DOWNGRADE_DETECTED 0x154 /* Detected an inappropriate TLS version downgrade by TLS 1.3 client. */ /* NX_CONTINUE is a symbol defined in NetX Duo 5.10. For backward compatibility, this symbol is defined here */ #if ((__NETXDUO_MAJOR_VERSION__ == 5) && (__NETXDUO_MINOR_VERSION__ == 9)) diff --git a/nx_secure/inc/nx_secure_x509.h b/nx_secure/inc/nx_secure_x509.h index 1825dd60..7863b0ff 100644 --- a/nx_secure/inc/nx_secure_x509.h +++ b/nx_secure/inc/nx_secure_x509.h @@ -26,7 +26,7 @@ /* COMPONENT DEFINITION RELEASE */ /* */ /* nx_secure_x509.h PORTABLE C */ -/* 6.1.10 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -55,6 +55,10 @@ /* ignored public key in EC */ /* private key, */ /* resulting in version 6.1.10 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), */ +/* checked expiration for all */ +/* the certs in the chain, */ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ @@ -936,7 +940,7 @@ UINT _nx_secure_x509_certificate_verify(NX_SECURE_X509_CERTIFICATE_STORE *store, /* Verify a given certificate chain to see if the end-entity certificate can be traced through the chain to a trust anchor. */ UINT _nx_secure_x509_certificate_chain_verify(NX_SECURE_X509_CERTIFICATE_STORE *store, - NX_SECURE_X509_CERT *certificate); + NX_SECURE_X509_CERT *certificate, ULONG current_time); /* Parse an OID string, returning an internally-used constant (defined above) for use in other parsing. */ VOID _nx_secure_x509_oid_parse(const UCHAR *oid, ULONG length, UINT *oid_value); diff --git a/nx_secure/src/nx_secure_dtls_send_record.c b/nx_secure/src/nx_secure_dtls_send_record.c index 24fd00ae..49936af0 100644 --- a/nx_secure/src/nx_secure_dtls_send_record.c +++ b/nx_secure/src/nx_secure_dtls_send_record.c @@ -33,7 +33,7 @@ /* FUNCTION RELEASE */ /* */ /* _nx_secure_dtls_send_record PORTABLE C */ -/* 6.1 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -86,6 +86,9 @@ /* verified memcpy use cases, */ /* released packet securely, */ /* resulting in version 6.1 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* checked seq number overflow,*/ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ UINT _nx_secure_dtls_send_record(NX_SECURE_DTLS_SESSION *dtls_session, NX_PACKET *send_packet, @@ -169,6 +172,14 @@ UCHAR epoch_seq_num[8]; { /* Check for overflow of the 32-bit number. */ tls_session -> nx_secure_tls_local_sequence_number[1]++; + + if (tls_session -> nx_secure_tls_local_sequence_number[1] == 0) + { + + /* Check for overflow of the 64-bit unsigned number. As it should not reach here + in practical, we return a general error to prevent overflow theoretically. */ + return(NX_NOT_SUCCESSFUL); + } } tls_session -> nx_secure_tls_local_sequence_number[0]++; diff --git a/nx_secure/src/nx_secure_tls_1_3_client_handshake.c b/nx_secure/src/nx_secure_tls_1_3_client_handshake.c index 0c2e7dcd..480e04e0 100644 --- a/nx_secure/src/nx_secure_tls_1_3_client_handshake.c +++ b/nx_secure/src/nx_secure_tls_1_3_client_handshake.c @@ -30,7 +30,7 @@ /* FUNCTION RELEASE */ /* */ /* _nx_secure_tls_1_3_client_handshake PORTABLE C */ -/* 6.1.4 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -116,6 +116,10 @@ /* support for fragmented TLS */ /* Handshake messages, */ /* resulting in version 6.1.4 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* updated alert message for */ +/* downgrade protection, */ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ @@ -286,7 +290,7 @@ const UCHAR *server_random; if (NX_SECURE_MEMCMP(&(tls_session -> nx_secure_tls_key_material.nx_secure_tls_server_random[24]), server_random, 8) == 0) { - status = NX_SECURE_TLS_UNKNOWN_TLS_VERSION; + status = NX_SECURE_TLS_DOWNGRADE_DETECTED; } else { diff --git a/nx_secure/src/nx_secure_tls_key_material_init.c b/nx_secure/src/nx_secure_tls_key_material_init.c index 7ea0483a..51513e2c 100644 --- a/nx_secure/src/nx_secure_tls_key_material_init.c +++ b/nx_secure/src/nx_secure_tls_key_material_init.c @@ -29,7 +29,7 @@ /* FUNCTION RELEASE */ /* */ /* _nx_secure_tls_key_material_init PORTABLE C */ -/* 6.1 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -61,31 +61,16 @@ /* 05-19-2020 Timothy Stapko Initial Version 6.0 */ /* 09-30-2020 Timothy Stapko Modified comment(s), */ /* resulting in version 6.1 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), */ +/* cleared all key material, */ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ UINT _nx_secure_tls_key_material_init(NX_SECURE_TLS_KEY_MATERIAL *key_material) { - /* Clear out key blocks individually - we may need to keep some key data - * around in the future for things like session re-negotiation/resumption. */ - - NX_SECURE_MEMSET(key_material -> nx_secure_tls_client_random, 0, NX_SECURE_TLS_RANDOM_SIZE); - - NX_SECURE_MEMSET(key_material -> nx_secure_tls_server_random, 0, NX_SECURE_TLS_RANDOM_SIZE); - - NX_SECURE_MEMSET(key_material -> nx_secure_tls_pre_master_secret, 0, NX_SECURE_TLS_PREMASTER_SIZE); - key_material -> nx_secure_tls_pre_master_secret_size = 0; - - NX_SECURE_MEMSET(key_material -> nx_secure_tls_master_secret, 0, NX_SECURE_TLS_MASTER_SIZE); - - NX_SECURE_MEMSET(key_material -> nx_secure_tls_key_material_data, 0, NX_SECURE_TLS_KEY_MATERIAL_SIZE); - NX_SECURE_MEMSET(key_material -> nx_secure_tls_new_key_material_data, 0, NX_SECURE_TLS_KEY_MATERIAL_SIZE); - -#if(NX_SECURE_TLS_TLS_1_3_ENABLED) - NX_SECURE_MEMSET(&key_material->nx_secure_tls_key_secrets, 0, sizeof(NX_SECURE_TLS_KEY_SECRETS)); - NX_SECURE_MEMSET(key_material->nx_secure_tls_ecc_key_data, 0, sizeof(key_material->nx_secure_tls_ecc_key_data)); - key_material->nx_secure_tls_handshake_cache_length = 0; -#endif + /* Clear out the entire key material. */ + NX_SECURE_MEMSET(key_material, 0, sizeof(NX_SECURE_TLS_KEY_MATERIAL)); return(NX_SECURE_TLS_SUCCESS); } diff --git a/nx_secure/src/nx_secure_tls_map_error_to_alert.c b/nx_secure/src/nx_secure_tls_map_error_to_alert.c index c0a10559..23538e72 100644 --- a/nx_secure/src/nx_secure_tls_map_error_to_alert.c +++ b/nx_secure/src/nx_secure_tls_map_error_to_alert.c @@ -29,7 +29,7 @@ /* FUNCTION RELEASE */ /* */ /* _nx_secure_tls_map_error_to_alert PORTABLE C */ -/* 6.1.6 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -74,6 +74,10 @@ /* 04-02-2021 Timothy Stapko Modified comment(s), */ /* updated X.509 return value, */ /* resulting in version 6.1.6 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* updated alert message for */ +/* downgrade protection, */ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ VOID _nx_secure_tls_map_error_to_alert(UINT error_number, UINT *alert_number, UINT *alert_level) @@ -176,6 +180,7 @@ VOID _nx_secure_tls_map_error_to_alert(UINT error_number, UINT *alert_number, UI case NX_SECURE_TLS_BAD_COMPRESSION_METHOD: /* Deliberate fall-through. */ case NX_SECURE_TLS_1_3_UNKNOWN_CIPHERSUITE: case NX_SECURE_TLS_BAD_SERVERHELLO_KEYSHARE: + case NX_SECURE_TLS_DOWNGRADE_DETECTED: *alert_number = NX_SECURE_TLS_ALERT_ILLEGAL_PARAMETER; *alert_level = NX_SECURE_TLS_ALERT_LEVEL_FATAL; break; diff --git a/nx_secure/src/nx_secure_tls_process_record.c b/nx_secure/src/nx_secure_tls_process_record.c index 8d9a204a..2abe5011 100644 --- a/nx_secure/src/nx_secure_tls_process_record.c +++ b/nx_secure/src/nx_secure_tls_process_record.c @@ -31,7 +31,7 @@ static VOID _nx_secure_tls_packet_trim(NX_PACKET *packet_ptr); /* FUNCTION RELEASE */ /* */ /* _nx_secure_tls_process_record PORTABLE C */ -/* 6.1.11 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -97,6 +97,9 @@ static VOID _nx_secure_tls_packet_trim(NX_PACKET *packet_ptr); /* 04-25-2022 Yuxin Zhou Modified comment(s), */ /* removed unnecessary code, */ /* resulting in version 6.1.11 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* checked seq number overflow,*/ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ UINT _nx_secure_tls_process_record(NX_SECURE_TLS_SESSION *tls_session, NX_PACKET *packet_ptr, @@ -323,6 +326,14 @@ NX_PACKET *decrypted_packet; { /* Check for overflow of the 32-bit unsigned number. */ tls_session -> nx_secure_tls_remote_sequence_number[1]++; + + if (tls_session -> nx_secure_tls_remote_sequence_number[1] == 0) + { + + /* Check for overflow of the 64-bit unsigned number. As it should not reach here + in practical, we return a general error to prevent overflow theoretically. */ + return(NX_NOT_SUCCESSFUL); + } } tls_session -> nx_secure_tls_remote_sequence_number[0]++; } diff --git a/nx_secure/src/nx_secure_tls_remote_certificate_verify.c b/nx_secure/src/nx_secure_tls_remote_certificate_verify.c index c8be7f1b..27df70ed 100644 --- a/nx_secure/src/nx_secure_tls_remote_certificate_verify.c +++ b/nx_secure/src/nx_secure_tls_remote_certificate_verify.c @@ -30,7 +30,7 @@ /* FUNCTION RELEASE */ /* */ /* _nx_secure_tls_remote_certificate_verify PORTABLE C */ -/* 6.1.10 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -81,6 +81,10 @@ /* improved code coverage */ /* results, */ /* resulting in version 6.1.10 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* checked expiration for all */ +/* the certs in the chain, */ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ UINT _nx_secure_tls_remote_certificate_verify(NX_SECURE_TLS_SESSION *tls_session) @@ -123,19 +127,11 @@ ULONG current_time; { /* Get the current time from our callback. */ current_time = tls_session -> nx_secure_tls_session_time_function(); - - /* Check the remote certificate against the current time. */ - status = _nx_secure_x509_expiration_check(remote_certificate, current_time); - - if (status != NX_SUCCESS) - { - return(status); - } } /* Now verify our remote certificate chain. If the certificate can be linked to an issuer in the trusted store through an issuer chain, this function will return NX_SUCCESS. */ - status = _nx_secure_x509_certificate_chain_verify(store, remote_certificate); + status = _nx_secure_x509_certificate_chain_verify(store, remote_certificate, current_time); if (status != NX_SUCCESS) { diff --git a/nx_secure/src/nx_secure_tls_send_record.c b/nx_secure/src/nx_secure_tls_send_record.c index 3a47e59f..2852a8cc 100644 --- a/nx_secure/src/nx_secure_tls_send_record.c +++ b/nx_secure/src/nx_secure_tls_send_record.c @@ -29,7 +29,7 @@ /* FUNCTION RELEASE */ /* */ /* _nx_secure_tls_send_record PORTABLE C */ -/* 6.1.11 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -93,6 +93,9 @@ /* 04-25-2022 Yuxin Zhou Modified comment(s), */ /* improved internal logic, */ /* resulting in version 6.1.11 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* checked seq number overflow,*/ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ UINT _nx_secure_tls_send_record(NX_SECURE_TLS_SESSION *tls_session, NX_PACKET *send_packet, @@ -318,6 +321,14 @@ NX_PACKET *current_packet; { /* Check for overflow of the 32-bit number. */ tls_session -> nx_secure_tls_local_sequence_number[1]++; + + if (tls_session -> nx_secure_tls_local_sequence_number[1] == 0) + { + + /* Check for overflow of the 64-bit unsigned number. As it should not reach here + in practical, we return a general error to prevent overflow theoretically. */ + return(NX_NOT_SUCCESSFUL); + } } tls_session -> nx_secure_tls_local_sequence_number[0]++; } diff --git a/nx_secure/src/nx_secure_tls_verify_mac.c b/nx_secure/src/nx_secure_tls_verify_mac.c index 5d3a7f77..16bb21c6 100644 --- a/nx_secure/src/nx_secure_tls_verify_mac.c +++ b/nx_secure/src/nx_secure_tls_verify_mac.c @@ -32,7 +32,7 @@ static UCHAR _received_hash[NX_SECURE_TLS_MAX_HASH_SIZE]; /* FUNCTION RELEASE */ /* */ /* _nx_secure_tls_verify_mac PORTABLE C */ -/* 6.1.11 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -79,6 +79,9 @@ static UCHAR _received_hash[NX_SECURE_TLS_MAX_HASH_SIZE]; /* 04-25-2022 Yuxin Zhou Modified comment(s), and */ /* reorganized internal logic, */ /* resulting in version 6.1.11 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* checked seq number overflow,*/ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ UINT _nx_secure_tls_verify_mac(NX_SECURE_TLS_SESSION *tls_session, UCHAR *header_data, @@ -132,6 +135,15 @@ ULONG bytes_copied; { /* Check for overflow of the 32-bit unsigned number. */ tls_session -> nx_secure_tls_remote_sequence_number[1]++; + + if (tls_session -> nx_secure_tls_remote_sequence_number[1] == 0) + { + + /* Check for overflow of the 64-bit unsigned number. As it should not reach here + in practical, we return a general error to prevent overflow theoretically. */ + return(NX_NOT_SUCCESSFUL); + } + } tls_session -> nx_secure_tls_remote_sequence_number[0]++; @@ -165,6 +177,15 @@ ULONG bytes_copied; { /* Check for overflow of the 32-bit unsigned number. */ tls_session -> nx_secure_tls_remote_sequence_number[1]++; + + if (tls_session -> nx_secure_tls_remote_sequence_number[1] == 0) + { + + /* Check for overflow of the 64-bit unsigned number. As it should not reach here + in practical, we return a general error to prevent overflow theoretically. */ + return(NX_NOT_SUCCESSFUL); + } + } tls_session -> nx_secure_tls_remote_sequence_number[0]++; diff --git a/nx_secure/src/nx_secure_x509_certificate_chain_verify.c b/nx_secure/src/nx_secure_x509_certificate_chain_verify.c index 4979d84e..8e6ebc2d 100644 --- a/nx_secure/src/nx_secure_x509_certificate_chain_verify.c +++ b/nx_secure/src/nx_secure_x509_certificate_chain_verify.c @@ -29,7 +29,7 @@ /* FUNCTION RELEASE */ /* */ /* _nx_secure_x509_certificate_chain_verify PORTABLE C */ -/* 6.1.11 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -76,10 +76,14 @@ /* 04-25-2022 Yuxin Zhou Modified comment(s), and */ /* reorganized internal logic, */ /* resulting in version 6.1.11 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* checked expiration for all */ +/* the certs in the chain, */ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ UINT _nx_secure_x509_certificate_chain_verify(NX_SECURE_X509_CERTIFICATE_STORE *store, - NX_SECURE_X509_CERT *certificate) + NX_SECURE_X509_CERT *certificate, ULONG current_time) { UINT status; NX_SECURE_X509_CERT *current_certificate; @@ -101,7 +105,15 @@ INT compare_result; { /* Check the certificate expiration against the current time. */ + if (current_time != 0) + { + status = _nx_secure_x509_expiration_check(current_certificate, current_time); + if (status != NX_SECURE_X509_SUCCESS) + { + return(status); + } + } /* See if the certificate is self-signed or not. */ compare_result = _nx_secure_x509_distinguished_name_compare(¤t_certificate -> nx_secure_x509_distinguished_name, diff --git a/nx_secure/src/nx_secure_x509_crl_revocation_check.c b/nx_secure/src/nx_secure_x509_crl_revocation_check.c index a2fe4e8c..b60c17df 100644 --- a/nx_secure/src/nx_secure_x509_crl_revocation_check.c +++ b/nx_secure/src/nx_secure_x509_crl_revocation_check.c @@ -35,7 +35,7 @@ static UINT _nx_secure_x509_crl_parse_entry(const UCHAR *buffer, ULONG length, U /* FUNCTION RELEASE */ /* */ /* _nx_secure_x509_crl_revocation_check PORTABLE C */ -/* 6.1.11 */ +/* 6.1.11a */ /* AUTHOR */ /* */ /* Timothy Stapko, Microsoft Corporation */ @@ -97,6 +97,10 @@ static UINT _nx_secure_x509_crl_parse_entry(const UCHAR *buffer, ULONG length, U /* modified to improve code */ /* coverage result, */ /* resulting in version 6.1.11 */ +/* 07-19-2022 Yuxin Zhou Modified comment(s), and */ +/* checked expiration for all */ +/* the certs in the chain, */ +/* resulting in version 6.1.11a*/ /* */ /**************************************************************************/ UINT _nx_secure_x509_crl_revocation_check(const UCHAR *crl_data, UINT crl_length, @@ -146,7 +150,7 @@ UINT serial_number_length; } /* Now, check that the issuer is valid. */ - status = _nx_secure_x509_certificate_chain_verify(store, issuer_certificate); + status = _nx_secure_x509_certificate_chain_verify(store, issuer_certificate, 0); if (status != NX_SECURE_X509_SUCCESS) { -- cgit v1.3.1