From 37d8682c1bc0e87313d8502f9b9b3ca1b400eafe Mon Sep 17 00:00:00 2001 From: Huan Nguyen Date: Wed, 14 May 2025 12:40:17 -0600 Subject: Move empty packet check in nx_secure_tls_session_send It was causing some tests to hang forever because they were expecting different errors to be returned. We do want to keep the check for empty packets as the netx_web_invalid_release_test expects that sending an empty packet fails. Note that the test also tests HTTPS, which will technically send a non-empty packet because the empty packet will be modified to include the TLS data. However, the empty packet check in nx_secure_tls_session_send will fulfill the same role as the check in nx_tcp_socket_send.c, checking for an empty packet prior to the modification of the packet to include TLS data. This gets the tests passing again. --- nx_secure/src/nxe_secure_tls_session_send.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/nx_secure/src/nxe_secure_tls_session_send.c b/nx_secure/src/nxe_secure_tls_session_send.c index 09e9281c..85ad1726 100644 --- a/nx_secure/src/nxe_secure_tls_session_send.c +++ b/nx_secure/src/nxe_secure_tls_session_send.c @@ -91,16 +91,6 @@ UINT status; return(NX_PTR_ERROR); } - if (packet_ptr -> nx_packet_length == 0) - { - /* Must check for empty packets here, as TLS data will make a packet's contents - non-empty. _nx_tcp_socket_send_internal has a check for an empty packet - that correctly works in an HTTP session but will result in a false negative if - the session is HTTPS. Thus, this check is performed before the TLS session - operations that modify the packet. */ - return(NX_INVALID_PACKET); - } - if (tls_session -> nx_secure_tls_tcp_socket == NX_NULL) { return(NX_SECURE_TLS_SESSION_UNINITIALIZED); @@ -121,6 +111,16 @@ UINT status; /* Check for appropriate caller. */ NX_THREADS_ONLY_CALLER_CHECKING + if (packet_ptr -> nx_packet_length == 0) + { + /* Must check for empty packets here, as TLS data will make a packet's contents + non-empty. _nx_tcp_socket_send_internal has a check for an empty packet + that correctly works in an HTTP session but will result in a false negative if + the session is HTTPS. Thus, this check is performed before the TLS session + operations that modify the packet. */ + return(NX_INVALID_PACKET); + } + status = _nx_secure_tls_session_send(tls_session, packet_ptr, wait_option); /* Return completion status. */ -- cgit v1.3.1 From 30accc9c2c2c0db6e853c49eee1dc17cdfab3e8b Mon Sep 17 00:00:00 2001 From: Huan Nguyen Date: Wed, 14 May 2025 12:43:20 -0600 Subject: Fix broken bounds check and add regression test The check is in the function _nx_secure_tls_process_clienthello_psk_extension and was reported as a vulnerability. --- nx_secure/src/nx_secure_tls_process_clienthello_extensions.c | 3 ++- .../nx_secure_tls_1_3_clienthello_length_checking_test.c | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/nx_secure/src/nx_secure_tls_process_clienthello_extensions.c b/nx_secure/src/nx_secure_tls_process_clienthello_extensions.c index 07b913bc..25169ae3 100644 --- a/nx_secure/src/nx_secure_tls_process_clienthello_extensions.c +++ b/nx_secure/src/nx_secure_tls_process_clienthello_extensions.c @@ -1444,7 +1444,8 @@ NX_SECURE_TLS_PSK_STORE *psk_store; offset += 2; /* Make sure the length is reasonable. */ - if(list_length > extension_length) + /* Account for extension_length including the 2-byte list_length field */ + if(list_length > (extension_length - 2U)) { return(NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH); } diff --git a/test/regression/nx_secure_test/nx_secure_tls_1_3_clienthello_length_checking_test.c b/test/regression/nx_secure_test/nx_secure_tls_1_3_clienthello_length_checking_test.c index dec2c502..3a724a2b 100644 --- a/test/regression/nx_secure_test/nx_secure_tls_1_3_clienthello_length_checking_test.c +++ b/test/regression/nx_secure_test/nx_secure_tls_1_3_clienthello_length_checking_test.c @@ -97,7 +97,7 @@ static UCHAR client_hello_empty_key_share[] = { 0x00, 0x02, 0x00, 0x01, 0x01, /* compression method */ 0x00, -0x00, 0x41, /* extensions */ +0x00, 0x45, /* extensions */ 0x00, 0x0a, /* ec groups */ 0x00, 0x08, 0x00, 0x06, 0x00, 0x17, 0x00, 0x18, 0x00, 0x19, @@ -116,7 +116,10 @@ static UCHAR client_hello_empty_key_share[] = { 0x04, 0x03, 0x05, 0x03, 0x06, 0x03, 0x04, 0x01, 0x05, 0x01, 0x06, 0x01, 0x03, 0x03, 0x02, 0x03, 0x02, 0x01, 0x01, 0x01, /* empty extension */ -0x00, 0x00, 0x00, 0x00, +0x00, 0x00, 0x00, 0x00, // ID, length, 2 bytes each +// List ID length, need at least 2 to make extension_NX_SECURE_TLS_EXTENSION_PRE_SHARED_KEY_LIST_LEN work +// (needs to store 2-byte List ID Length) +0x00, 0x00, 0x00, 0x00 }; static UCHAR client_hello_size[] = {0x00, 0x9e}; @@ -124,6 +127,7 @@ static UCHAR client_hello_size[] = {0x00, 0x9e}; /* various extension types. */ static UCHAR extension_NX_SECURE_TLS_EXTENSION_PRE_SHARED_KEY_ZERO[] = {0x00, 0x29, 0x00, 0x00}; static UCHAR extension_NX_SECURE_TLS_EXTENSION_PRE_SHARED_KEY_MAX_INT[] = {0x00, 0x29, 0xff, 0xff}; +static UCHAR extension_NX_SECURE_TLS_EXTENSION_PRE_SHARED_KEY_LIST_LEN[] = {0x00, 0x29, 0x00, 0x04, 0x00, 0x03}; static UCHAR extension_NX_SECURE_TLS_EXTENSION_SECURE_RENEGOTIATION_ZERO[] = {0xff, 0x01, 0x00, 0x00}; static UCHAR extension_NX_SECURE_TLS_EXTENSION_SECURE_RENEGOTIATION_MAX_INT[] = {0xff, 0x01, 0xff, 0xff}; static UCHAR extension_NX_SECURE_TLS_EXTENSION_SERVER_NAME_INDICATION_MAX_INT[] = {0x00, 0x00, 0xff, 0xff}; @@ -178,6 +182,7 @@ static TEST_POINT test_array[] = /* other extension length fields. */ #ifdef NX_SECURE_ENABLE_PSK_CIPHERSUITES {NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH, 154, extension_NX_SECURE_TLS_EXTENSION_PRE_SHARED_KEY_ZERO, 4}, + {NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH, 154, extension_NX_SECURE_TLS_EXTENSION_PRE_SHARED_KEY_LIST_LEN, 6}, #endif {NX_SECURE_TLS_INCORRECT_MESSAGE_LENGTH, 154, extension_NX_SECURE_TLS_EXTENSION_PRE_SHARED_KEY_MAX_INT, 4}, #ifndef NX_SECURE_TLS_DISABLE_SECURE_RENEGOTIATION -- cgit v1.3.1