eclipse-threadx / netxduo

Eclipse ThreadX - NetXDuo is an advanced, industrial-grade TCP/IP network stack designed specifically for deeply embedded real-time and IoT applications
https://github.com/eclipse-threadx/rtos-docs/blob/main/rtos-docs/netx-duo/index.md
MIT License
230 stars 131 forks source link

BusFault happens when TLS 1.3 client authentication is used #153

Closed jkuwaha closed 1 year ago

jkuwaha commented 1 year ago

Hi,

We are using NetX Duo 6.1.10, and experiencing BusFault when trying to connect with 'TLS 1.3 with client authentication'.

We use openssl s_server as a server, and it works fine with 'TLS 1.2 with/without client authentication' and 'TLS 1.3 without client authentication'.

BusFault happens in _nx_packet_release(), but its line numbers and call paths vary.

Could you please let us know what we need to check? Is this related to other issues already reported?

Settings:

    _nx_packet_release() at nx_packet_release.c:140
    _nx_secure_tls_handshake_process() at nx_secure_tls_handshake_process.c:103
    _nx_secure_tls_session_start() at nx_secure_tls_session_start.c:185
    _nxe_secure_tls_session_start() at nxe_secure_tls_session_start.c:99
    ...
    _nx_packet_release() at nx_packet_release.c:133
    _nx_secure_tls_handshake_process() at nx_secure_tls_handshake_process.c:103
    _nx_secure_tls_session_start() at nx_secure_tls_session_start.c:185
    _nxe_secure_tls_session_start() at nxe_secure_tls_session_start.c:99
    ...
    _nx_packet_release() at nx_packet_release.c:133
    _nx_tcp_socket_send_internal() at nx_tcp_socket_send_internal.c:961
    _nx_tcp_socket_send() at nx_tcp_socket_send.c:87
    _nx_secure_tls_send_record() at nx_secure_tls_send_record.c:345
    _nx_secure_tls_send_handshake_record() at nx_secure_tls_send_handshake_record.c:161
    _nx_secure_tls_1_3_client_handshake() at nx_secure_tls_1_3_client_handshake.c:571
    _nx_secure_tls_process_record() at nx_secure_tls_process_record.c:498
    _nx_secure_tls_session_receive_records() at nx_secure_tls_session_receive_records.c:121
    _nx_secure_tls_handshake_process() at nx_secure_tls_handshake_process.c:90
    _nx_secure_tls_session_start() at nx_secure_tls_session_start.c:185
    ...
jkuwaha commented 1 year ago

Hi,

We think we found the root cause of the issue.

Settings

Observation

On calling nx_secure_tls_session_start(), BusFault happens. One of the certificates NetX sent has extra zeros in the middle of it.

Root cause

In _nx_secure_tls_send_certificate(), an out-of-array access happens, which may destroy a value of nx_packet_pool_owner member of NX_PACKET.

This may cause BusFault when the pointer is used at _nx_packet_release().

https://github.com/azure-rtos/netxduo/blob/068a9493ecd5af0024005ce695b992c4b172d6aa/nx_secure/src/nx_secure_tls_send_certificate.c#L223-L238

When the certificates do not fit in one NX_PACKET, new NX_PACKET is allocated and linked to the original.

In that case, send_packet -> nx_packet_append_ptr does not point to the end of valid data, but the next address of the end of the data area of the first NX_PACKET. Writing through this pointer causes out of bounds access.

Tentative fix

I tried following and confirmed working.

#if (NX_SECURE_TLS_TLS_1_3_ENABLED)
        /* Check for TLS 1.3 extensions following each certificate. */
        if(tls_session->nx_secure_tls_1_3)
        {
            extensions_length = 0;
            UCHAR extensions_length_be[2];

            extensions_length_be[0] = (UCHAR)((extensions_length & 0xFF00) >> 8);
            extensions_length_be[1] = (UCHAR)(extensions_length & 0x00FF);

            /* Add extension length to packet. */
            status = nx_packet_data_append(send_packet,
                                           extensions_length_be,
                                           sizeof(extensions_length_be),
                                           tls_session -> nx_secure_tls_packet_pool,
                                           wait_option);
            if (status != NX_SUCCESS)
            {
                return(status);
            }
            total_length += 2;
        }
#endif

Does this fix look okay?

yanwucai commented 1 year ago

Thanks for reporting this bug. Your fix looks good.

jkuwaha commented 1 year ago

Thank you for checking my code! I'll use this solution until the bug is fixed in the future release.

yanwucai commented 1 year ago

The bug fix is available in this commit.