HNS-2024-06-threadx.txt

--[ HNS-2024-06 - HN Security Advisory - https://security.humanativaspa.it/

* Title: Multiple vulnerabilities in Eclipse ThreadX
* OS: Eclipse ThreadX < 6.4.0
* Author: Marco Ivaldi <marco.ivaldi@hnsecurity.it>
* Date: 2024-05-28
* CVE IDs and severity:
  * CVE-2024-2214 - High - 7.0 - CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:H
  * CVE-2024-2212 - High - 7.3 - CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:L
  * CVE-2024-2452 - High - 7.0 - CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:L/I:H/A:L
* Advisory URLs: 
  * https://github.com/eclipse-threadx/threadx/security/advisories/GHSA-vmp6-qhp9-r66x
  * https://github.com/eclipse-threadx/threadx/security/advisories/GHSA-v9jj-7qjg-h6g6
  * https://github.com/eclipse-threadx/netxduo/security/advisories/GHSA-h963-7vhw-8rpx
* Vendor URL: https://threadx.io/


--[ 0 - Table of contents

1 - Summary
2 - Background
3 - Vulnerabilities
    3.1 - CVE-2024-2214 - Ineffective array size check and static buffer overflow in Eclipse ThreadX
    3.2 - CVE-2024-2212 - Integer wraparounds, under-allocations, and heap buffer overflows in in Eclipse ThreadX
    3.3 - CVE-2024-2452 - Integer wraparound, under-allocation, and heap buffer overflow in Eclipse ThreadX NetX Duo
    3.4 - Other bugs with potential security implications in Eclipse ThreadX NetX Duo and USBX
4 - Affected products
5 - Remediation
6 - Disclosure timeline
7 - Acknowledgments
8 - References


--[ 1 - Summary

"Why don’t you pick on projects your own size, 
quit tormenting the tiny ones!" 
                                  -- The Grugq

Azure RTOS was Microsoft's real-time operating system for IoT devices. At the
beginning of 2024, Microsoft contributed the Azure RTOS technology to the
Eclipse Foundation [1]. With the Eclipse Foundation as its new home, Azure RTOS
was rebranded as Eclipse ThreadX.

Eclipse ThreadX is an advanced embedded development suite including a small but
powerful operating system that provides reliable, ultra-fast performance for
resource-constrained devices. It offers a vendor-neutral, open source, safety
certified OS for real-time applications, all under a permissive license. 

We reviewed ThreadX's source code hosted on GitHub [2] and identified multiple
security vulnerabilities that may cause memory corruption. Their impacts range
from denial of service to potential arbitrary code execution.


--[ 2 - Background

Continuing our recent vulnerability research work in the IoT space [3] [4] [5],
we keep assisting open-source projects in finding and fixing vulnerabilities by
reviewing their source code. In December 2023, Azure RTOS, which one month
later was rebranded as Eclipse ThreadX, was selected as a target of interest.

During the source code review, we made use of our Semgrep C/C++ ruleset [6] and
weggli pattern collection [7] to identify hotspots in code on which to focus
our attention.


--[ 3 - Vulnerabilities

The vulnerabilities resulting from our source code review are briefly described
in the following sections.


--[ 3.1 - CVE-2024-2214 - Ineffective array size check and static buffer overflow in Eclipse ThreadX

In Eclipse ThreadX before version 6.4.0, the `_Mtxinit()` function in the
Xtensa port was missing an array size check causing a memory overwrite.

The vulnerability was spotted in the following file:
* /ports/xtensa/xcc/src/tx_clib_lock.c

There was no error handling in case `lcnt` >= `XT_NUM_CLIB_LOCKS`. The program
would continue and the `tx_mutex_create()` would eventually corrupt memory by
writing outside the bounds of the `xclib_locks` static array:
```c
#ifdef TX_THREAD_SAFE_CLIB    /* this file is only needed if using C lib */
...
#if XSHAL_CLIB == XTHAL_CLIB_XCLIB
...
static TX_MUTEX xclib_locks[XT_NUM_CLIB_LOCKS];
static uint32_t lcnt;
...
/**************************************************************************/
/*    _Mtxinit - initialize a lock. Called once for each lock.            */
/**************************************************************************/
void
_Mtxinit (_Rmtx * mtx)
{
    TX_MUTEX * lock;

    if (lcnt >= XT_NUM_CLIB_LOCKS) { // VULN: empty if() body
        /* Fatal error */
    }

    lock = &(xclib_locks[lcnt]);
    lcnt++;

    /* See notes for newlib case below. */
#ifdef THREADX_TESTSUITE
    tx_mutex_create (lock, "Clib lock", 0);
#else
    tx_mutex_create (lock, "Clib lock", TX_INHERIT);
#endif

    *mtx = lock;
}
```

Fixes:
https://github.com/eclipse-threadx/threadx/pull/340

See also:
https://github.com/eclipse-threadx/threadx/security/advisories/GHSA-vmp6-qhp9-r66x


--[ 3.2 - CVE-2024-2212 - Integer wraparounds, under-allocations, and heap buffer overflows in in Eclipse ThreadX

In Eclipse ThreadX before version 6.4.0, functions `xQueueCreate()` and
`xQueueCreateSet()` from the FreeRTOS compatibility API were missing parameter
checks. This could lead to integer wraparound, under-allocations, and heap
buffer overflows.

The vulnerabilities were spotted in the following file:
* /utility/rtos_compatibility_layers/FreeRTOS/tx_freertos.c

If an attacker could control `uxQueueLength` or `uxItemSize`, they could cause
an integer wraparound thus causing `txfr_malloc()` to allocate a small amount
of memory, exposing to subsequent heap buffer overflows (AKA BadAlloc-style
memory corruption):
```c
QueueHandle_t xQueueCreate(UBaseType_t uxQueueLength, UBaseType_t uxItemSize)
{
    txfr_queue_t *p_queue;
    void *p_mem;
    size_t mem_size;
    UINT ret;

    configASSERT(uxQueueLength != 0u);
    configASSERT(uxItemSize >= sizeof(UINT));

#if (TX_FREERTOS_AUTO_INIT == 1)
    if(txfr_initialized != 1u) {
        tx_freertos_auto_init();
    }
#endif

    p_queue = txfr_malloc(sizeof(txfr_queue_t));
    if(p_queue == NULL) {
        return NULL;
    }

    mem_size = uxQueueLength*(uxItemSize);

    p_mem = txfr_malloc(mem_size); // VULN: integer wraparound and under-allocation
    if(p_mem == NULL) {
        txfr_free(p_queue);
        return NULL;
    }

    TX_MEMSET(p_mem, 0, mem_size);
    TX_MEMSET(p_queue, 0, sizeof(*p_queue));
    p_queue->allocated = 1u;
    p_queue->p_mem = p_mem;
    p_queue->id = TX_QUEUE_ID;

    p_queue->p_write = (uint8_t *)p_mem;
    p_queue->p_read = (uint8_t *)p_mem;
    p_queue->msg_size = uxItemSize;
    p_queue->queue_length = uxQueueLength;

    ret = tx_semaphore_create(&p_queue->read_sem, "", 0u);
    if(ret != TX_SUCCESS) {
        return NULL;
    }

    ret = tx_semaphore_create(&p_queue->write_sem, "", uxQueueLength);
    if(ret != TX_SUCCESS) {
        return NULL;
    }

    return p_queue;
}
```

If an attacker could control `uxEventQueueLengthi`, they could cause an integer
wraparound thus causing `txfr_malloc()` to allocate a small amount of memory,
exposing to subsequent heap buffer overflows (AKA BadAlloc-style memory
corruption):
```c
QueueSetHandle_t xQueueCreateSet(const UBaseType_t uxEventQueueLength)
{
    txfr_queueset_t *p_set;
    void *p_mem;
    ULONG queue_size;
    UINT ret;

    configASSERT(uxEventQueueLength != 0u);

#if (TX_FREERTOS_AUTO_INIT == 1)
    if(txfr_initialized != 1u) {
        tx_freertos_auto_init();
    }
#endif

    p_set = txfr_malloc(sizeof(txfr_queueset_t));
    if(p_set == NULL) {
        return NULL;
    }

    queue_size = sizeof(void *) * uxEventQueueLength;
    p_mem = txfr_malloc(queue_size); // VULN: integer wraparound and under-allocation
    if(p_mem == NULL) {
        txfr_free(p_set);
        return NULL;
    }

    ret = tx_queue_create(&p_set->queue, "", sizeof(void *) / sizeof(UINT), p_mem, queue_size);
    if(ret != TX_SUCCESS) {
        TX_FREERTOS_ASSERT_FAIL();
        return NULL;
    }

    return p_set;
}
```

These functions are part of an external API to be used by user's applications.
The values of those parameters passed to the vulnerable functions depend on
user's code.

Fixes:
https://github.com/eclipse-threadx/threadx/pull/339

See also:
https://github.com/eclipse-threadx/threadx/security/advisories/GHSA-v9jj-7qjg-h6g6


--[ 3.3 - CVE-2024-2452 - Integer wraparound, under-allocation, and heap buffer overflow in Eclipse ThreadX NetX Duo

In Eclipse ThreadX NetX Duo before version 6.4.0, if an attacker could control
the parameters of `__portable_aligned_alloc()` they could cause an integer
wraparound and an allocation smaller than expected. This could cause subsequent
heap buffer overflows.

The vulnerability was spotted in the following file:
* /addons/azure_iot/azure_iot_security_module/iot-security-module-core/deps/flatcc/include/flatcc/portable/paligned_alloc.h

If an attacker could control the `size` or `alignment` arguments to the
`__portable_aligned_alloc()` function, they could cause an integer wraparound
thus causing `malloc()` to allocate a small amount of memory, exposing to
subsequent heap buffer overflows (AKA BadAlloc-style memory corruption):
```c
static inline void *__portable_aligned_alloc(size_t alignment, size_t size)
{
    char *raw;
    void *buf;
    size_t total_size = (size + alignment - 1 + sizeof(void *)); // VULN: integer wraparound

    if (alignment < sizeof(void *)) {
        alignment = sizeof(void *);
    }
    raw = (char *)(size_t)malloc(total_size); // VULN: under-allocation BadAlloc style
    buf = raw + alignment - 1 + sizeof(void *);
    buf = (void *)(((size_t)buf) & ~(alignment - 1));
    ((void **)buf)[-1] = raw; // malloc ret is not checked; in case NULL is returned the program would crash here
    return buf;
}
```

We spotted the same vulnerability in Azure IoT Preview source code at:
https://github.com/azure-rtos/azure-iot-preview/blob/master/azure_iot/azure_iot_security_module/iot-security-module-core/deps/flatcc/include/flatcc/portable/paligned_alloc.h

The maintainers confirmed the vulnerability, but informed us that the Azure IoT
Preview repository was not part of a product. Therefore, it was removed
entirely.

Fixes:
https://github.com/eclipse-threadx/netxduo/pull/227

See also:
https://github.com/eclipse-threadx/netxduo/security/advisories/GHSA-h963-7vhw-8rpx


--[ 3.4 - Other bugs with potential security implications in Eclipse ThreadX NetX Duo and USBX

In addition to the vulnerabilities covered in the previous sections, we also
reported a few other bugs with potential security implications that were not
considered as vulnerabilities by Eclipse ThreadX maintainers. As such, our
reports were declassed to standard issues for code improvement.

The first one is an unsafe use of the return value of `snprintf()` that we
observed in Eclipse ThreadX NetX Duo, in the following file:
* /addons/azure_iot/nx_azure_iot_adu_agent.c

The `snprintf()` API function returns the total length of the string it tried
to create, which could be larger than the actual length written; if an attacker
were able to craft input so that `update_id_length` became larger than
`NX_AZURE_IOT_ADU_AGENT_UPDATE_MANIFEST_SIZE` and if the return value were used
unsafely (e.g., as an array index) somewhere else in the code, memory
corruption could have occured:
```c
static UINT nx_azure_iot_adu_agent_reported_properties_state_send(NX_AZURE_IOT_ADU_AGENT *adu_agent_ptr)
{

NX_PACKET *packet_ptr;
NX_AZURE_IOT_JSON_WRITER json_writer;
NX_AZURE_IOT_ADU_AGENT_UPDATE_MANIFEST_CONTENT *manifest_content = &(adu_agent_ptr -> nx_azure_iot_adu_agent_update_manifest_content);
UINT status;
UINT result_code;
UINT i;
/* Prepare the buffer for step name: such as: "step_0", the max name is "step_xxx".  */
CHAR step_property_name[8] = "step_";
UINT step_size = sizeof("step_") - 1;
UINT step_property_name_size;
UINT update_id_length;
...
    /* Fill installed update id.  */
    if ((adu_agent_ptr -> nx_azure_iot_adu_agent_state == NX_AZURE_IOT_ADU_AGENT_STATE_IDLE) && 
        (adu_agent_ptr -> nx_azure_iot_adu_agent_update_manifest_content.steps_count))
    {

        /* Use nx_azure_iot_adu_agent_update_manifest as temporary buffer to encode the update id as string.*/
        update_id_length = (UINT)snprintf((CHAR *)adu_agent_ptr -> nx_azure_iot_adu_agent_update_manifest,
                                          NX_AZURE_IOT_ADU_AGENT_UPDATE_MANIFEST_SIZE,
                                          "{\"%.*s\":\"%.*s\",\"%.*s\":\"%.*s\",\"%.*s\":\"%.*s\"}",
                                          sizeof(NX_AZURE_IOT_ADU_AGENT_PROPERTY_NAME_PROVIDER) - 1,
                                          NX_AZURE_IOT_ADU_AGENT_PROPERTY_NAME_PROVIDER,
                                          manifest_content -> update_id.provider_length, manifest_content -> update_id.provider, 
                                          sizeof(NX_AZURE_IOT_ADU_AGENT_PROPERTY_NAME_NAME) - 1,
                                          NX_AZURE_IOT_ADU_AGENT_PROPERTY_NAME_NAME,
                                          manifest_content -> update_id.name_length, manifest_content -> update_id.name,
                                          sizeof(NX_AZURE_IOT_ADU_AGENT_PROPERTY_NAME_VERSION) - 1,
                                          NX_AZURE_IOT_ADU_AGENT_PROPERTY_NAME_VERSION,
                                          manifest_content -> update_id.version_length, manifest_content -> update_id.version); // VULN: unsafe use of snprintf() return value

        if (nx_azure_iot_json_writer_append_property_with_string_value(&json_writer,
                                                                       (const UCHAR *)NX_AZURE_IOT_ADU_AGENT_PROPERTY_NAME_INSTALLED_CONTENT_ID,
                                                                       sizeof(NX_AZURE_IOT_ADU_AGENT_PROPERTY_NAME_INSTALLED_CONTENT_ID) - 1,
                                                                       adu_agent_ptr -> nx_azure_iot_adu_agent_update_manifest,
                                                                       update_id_length)) // VULN: potentially large length is used to populate the "installedUpdateId" JSON property
        {
            nx_packet_release(packet_ptr);
            return (NX_NOT_SUCCESSFUL);
        }
    }
...
```

We also spotted some potentially ineffective size checks due to assertions in
Eclipse ThreadX USBX, in the following files:
* /common/core/src/ux_hcd_sim_host_transaction_schedule.c
* /common/usbx_device_classes/src/ux_device_class_audio20_control_process.c

If assertions were compiled-out in production code and `td ->
ux_sim_host_td_length` was attacker-controlled, the `_ux_utility_memory_copy()`
function would have been able to write past the `slave_transfer_request ->
ux_slave_transfer_request_setup` fixed-size (8 bytes) buffer:
```c
UINT  _ux_hcd_sim_host_transaction_schedule(UX_HCD_SIM_HOST *hcd_sim_host, UX_HCD_SIM_HOST_ED *ed)
{

UX_DCD_SIM_SLAVE        *dcd_sim_slave;
UX_HCD_SIM_HOST_TD      *td;
UX_HCD_SIM_HOST_TD      *head_td;
UX_HCD_SIM_HOST_TD      *tail_td;
UX_HCD_SIM_HOST_TD      *data_td;
UX_ENDPOINT             *endpoint;
UX_SLAVE_ENDPOINT       *slave_endpoint;
UX_DCD_SIM_SLAVE_ED     *slave_ed;
ULONG                   slave_transfer_remaining;
UCHAR                   wake_host;
UCHAR                   wake_slave;
ULONG                   transaction_length;
ULONG                   td_length;
UX_SLAVE_TRANSFER       *slave_transfer_request;
UX_TRANSFER             *transfer_request;
ULONG                   endpoint_index;
UX_SLAVE_DCD            *dcd;

    UX_PARAMETER_NOT_USED(hcd_sim_host);

    /* Get the pointer to the DCD portion of the simulator.  */
    dcd =  &_ux_system_slave -> ux_system_slave_dcd;

    /* Check the state of the controller if OPERATIONAL .  */
    if (dcd -> ux_slave_dcd_status !=  UX_DCD_STATUS_OPERATIONAL)
        return(UX_ERROR);

    /* Get the pointer to the candidate TD on the host.  */
    td =  ed -> ux_sim_host_ed_head_td;

    /* Get the pointer to the endpoint.  */
    endpoint =  ed -> ux_sim_host_ed_endpoint;

    /* Get the pointer to the transfer_request attached with this TD.  */
    transfer_request =  td -> ux_sim_host_td_transfer_request;

    /* Get the index of the endpoint from the host.  */
    endpoint_index = endpoint -> ux_endpoint_descriptor.bEndpointAddress & ~(ULONG)UX_ENDPOINT_DIRECTION;

    /* Get the address of the device controller.  */
    dcd_sim_slave =  (UX_DCD_SIM_SLAVE *) dcd -> ux_slave_dcd_controller_hardware;

    /* Get the endpoint as seen from the device side.  */
#ifdef UX_DEVICE_BIDIRECTIONAL_ENDPOINT_SUPPORT
    slave_ed = ((endpoint -> ux_endpoint_descriptor.bEndpointAddress == 0) ?
            &dcd_sim_slave -> ux_dcd_sim_slave_ed[0] :
            ((endpoint -> ux_endpoint_descriptor.bEndpointAddress & UX_ENDPOINT_DIRECTION) ?
                &dcd_sim_slave -> ux_dcd_sim_slave_ed_in[endpoint_index] :
                &dcd_sim_slave -> ux_dcd_sim_slave_ed[endpoint_index]));
#else
    slave_ed =  &dcd_sim_slave -> ux_dcd_sim_slave_ed[endpoint_index];
#endif

    /* Is this ED used?  */
    if ((slave_ed -> ux_sim_slave_ed_status & UX_DCD_SIM_SLAVE_ED_STATUS_USED) == 0)
        return(UX_ERROR);

    /* Is this ED ready for transaction or stalled ?  */
    if ((slave_ed -> ux_sim_slave_ed_status & (UX_DCD_SIM_SLAVE_ED_STATUS_TRANSFER | UX_DCD_SIM_SLAVE_ED_STATUS_STALLED)) == 0)
        return(UX_ERROR);

    /* Get the logical endpoint from the physical endpoint.  */
    slave_endpoint =  slave_ed -> ux_sim_slave_ed_endpoint;

    /* Get the pointer to the transfer request.  */
    slave_transfer_request =  &slave_endpoint -> ux_slave_endpoint_transfer_request;

    /* Check the phase for this transfer, if this is the SETUP phase, treatment is different.  Explanation of how 
       control transfers are handled in the simulator: if the data phase is OUT, we handle it immediately, meaning we 
       send all the data to the device and remove the STATUS TD in the same scheduler call. If the data phase is IN, we 
       only take out the SETUP TD and handle the data phase like any other non-control transactions (i.e. the scheduler 
       calls us again with the DATA TDs).  */
    if (td -> ux_sim_host_td_status &  UX_HCD_SIM_HOST_TD_SETUP_PHASE)
    {

        /* For control transfer, stall is for protocol error and it's cleared any time when SETUP is received */
        slave_ed -> ux_sim_slave_ed_status &= ~(ULONG)UX_DCD_SIM_SLAVE_ED_STATUS_STALLED;

        /* Validate the length to the setup transaction buffer.  */
        UX_ASSERT(td -> ux_sim_host_td_length == 8); // VULN: if assertions are compiled-out in production code, this check is ineffective

        /* Reset actual data length (not including SETUP received) so far.  */
        slave_transfer_request -> ux_slave_transfer_request_actual_length =  0;

        /* Move the buffer from the host TD to the device TD.  */
        _ux_utility_memory_copy(slave_transfer_request -> ux_slave_transfer_request_setup,
                                td -> ux_sim_host_td_buffer,
                                td -> ux_sim_host_td_length); /* Use case of memcpy is verified. */ // VULN: potential buffer overflow due to ineffective size check
```

If assertions were compiled-out in production code and `data_length` was
attacker-controlled, the `_ux_utility_memory_copy()` function would have been
able to write past the `transfer -> ux_slave_transfer_request_data_pointer`
buffer:
```c
...
UINT _ux_device_class_audio20_control_process(UX_DEVICE_CLASS_AUDIO *audio,
                                              UX_SLAVE_TRANSFER *transfer,
                                              UX_DEVICE_CLASS_AUDIO20_CONTROL_GROUP *group)
{

UX_SLAVE_ENDPOINT                   *endpoint;
UX_DEVICE_CLASS_AUDIO20_CONTROL     *control;
UCHAR                               request;
UCHAR                               request_type;
UCHAR                               unit_id;
UCHAR                               control_selector;
UCHAR                               channel_number;
ULONG                               request_length;
ULONG                               data_length;
ULONG                               i;
ULONG                               n_sub, pos, min, max, res, freq;

    /* Get instances.  */
    endpoint = &audio -> ux_device_class_audio_device -> ux_slave_device_control_endpoint;
    transfer = &endpoint -> ux_slave_endpoint_transfer_request;

    /* Extract all necessary fields of the request.  */
    request          = *(transfer -> ux_slave_transfer_request_setup + UX_DEVICE_CLASS_AUDIO_REQUEST_REQUEST);
    request_type     = *(transfer -> ux_slave_transfer_request_setup + UX_DEVICE_CLASS_AUDIO_REQUEST_REQUEST_TYPE);
    unit_id          = *(transfer -> ux_slave_transfer_request_setup + UX_DEVICE_CLASS_AUDIO_REQUEST_ENEITY_ID);
    control_selector = *(transfer -> ux_slave_transfer_request_setup + UX_DEVICE_CLASS_AUDIO_REQUEST_CONTROL_SELECTOR);
    channel_number   = *(transfer -> ux_slave_transfer_request_setup + UX_DEVICE_CLASS_AUDIO_REQUEST_CHANNEL_NUMBER);
    request_length   = _ux_utility_short_get(transfer -> ux_slave_transfer_request_setup + UX_SETUP_LENGTH);

    for (i = 0; i < group -> ux_device_class_audio20_control_group_controls_nb; i ++)
    {
        control = &group -> ux_device_class_audio20_control_group_controls[i];

        /* Reset change map.  */
        control -> ux_device_class_audio20_control_changed = 0;

        /* Is this request a clock unit request?  */
        if (unit_id == control -> ux_device_class_audio20_control_cs_id)
        {

            /* Clock Source request.
             * We only support Sampling Frequency Control here.
             * The Sampling Frequency Control must support the CUR and RANGE(MIN, MAX, RES) attributes.
             */
...
            /* We just support sampling frequency control, GET request.  */
            if ((request_type & UX_REQUEST_DIRECTION) == UX_REQUEST_IN &&
                (control_selector == UX_DEVICE_CLASS_AUDIO20_CS_SAM_FREQ_CONTROL))
            {

                switch(request)
                {
                case UX_DEVICE_CLASS_AUDIO20_CUR:

                    /* Check request parameter.  */
                    if (request_length < 4)
                        break;

                    /* Send sampling frequency.  */
                    if (control -> ux_device_class_audio20_control_sampling_frequency)
                        _ux_utility_long_put(transfer -> ux_slave_transfer_request_data_pointer, control -> ux_device_class_audio20_control_sampling_frequency);
                    else
                        _ux_utility_long_put(transfer -> ux_slave_transfer_request_data_pointer, control -> ux_device_class_audio20_control_sampling_frequency_cur);
                    _ux_device_stack_transfer_request(transfer, 4, request_length);
                    return(UX_SUCCESS);

                case UX_DEVICE_CLASS_AUDIO20_RANGE:

                    /* Check request parameter.  */
                    if (request_length < 2)
                        break;

                    if (control -> ux_device_class_audio20_control_sampling_frequency == 0)
                    {

                        /* Send range parameters, RANGE is customized.  */
                        UX_ASSERT(control -> ux_device_class_audio20_control_sampling_frequency_range != UX_NULL);

                        /* Get wNumSubRanges.  */
                        n_sub = _ux_utility_short_get(control -> ux_device_class_audio20_control_sampling_frequency_range);
                        UX_ASSERT(n_sub > 0);

                        /* Calculate length, n_sub is 16-bit width, result not overflows ULONG.  */
                        data_length = 2 + n_sub * 12;
                        UX_ASSERT(data_length <= UX_SLAVE_REQUEST_CONTROL_MAX_LENGTH);  // VULN: if assertions are compiled-out in production code, this check is ineffective

                        /* Copy data.  */
                        data_length = UX_MIN(data_length, request_length);
                        _ux_utility_memory_copy(transfer -> ux_slave_transfer_request_data_pointer,
                                control -> ux_device_class_audio20_control_sampling_frequency_range,
                                data_length); /* Use case of memcpy is verified. */ // VULN: potential buffer overflow due to ineffective size check
...
```

Please note that there may be other instances/variants of these last bugs.
Therefore, a thorough assessment of all assertions used to check buffer sizes
in Eclipse ThreadX codebase is recommended.

Finally, in Eclipse ThreadX USBX before pull request #161 was merged, there was
a memory copy with unchecked size in the
`_ux_host_class_pima_storage_info_get()` function in the following file:
* /common/usbx_host_classes/src/ux_host_class_pima_storage_info_get.c

There was no size check for the two memory copy operations via the
`_ux_utility_memory_copy()` function marked below. Therefore, a write past the
end of the fixed size (256 bytes) buffer `storage ->
ux_host_class_pima_storage_description` could have occured:
```c
UINT  _ux_host_class_pima_storage_info_get(UX_HOST_CLASS_PIMA *pima,
                                        UX_HOST_CLASS_PIMA_SESSION *pima_session,
                                        ULONG storage_id, UX_HOST_CLASS_PIMA_STORAGE *storage)
{

UX_HOST_CLASS_PIMA_COMMAND           command;
UINT                                 status;
UCHAR                                *storage_buffer;
UCHAR                                *storage_pointer;
ULONG                                unicode_string_length;

    /* If trace is enabled, insert this event into the trace buffer.  */
    UX_TRACE_IN_LINE_INSERT(UX_TRACE_HOST_CLASS_PIMA_STORAGE_INFO_GET, pima, storage_id, storage, 0, UX_TRACE_HOST_CLASS_EVENTS, 0, 0)

    /* Check if this session is valid or not.  */
    if (pima_session -> ux_host_class_pima_session_magic != UX_HOST_CLASS_PIMA_MAGIC_NUMBER)
        return (UX_HOST_CLASS_PIMA_RC_SESSION_NOT_OPEN);

    /* Check if this session is opened or not.  */
    if (pima_session -> ux_host_class_pima_session_state != UX_HOST_CLASS_PIMA_SESSION_STATE_OPENED)
        return (UX_HOST_CLASS_PIMA_RC_SESSION_NOT_OPEN);

    /* Issue command to get the storage IDs.  1 parameter.  */
    command.ux_host_class_pima_command_nb_parameters =  1;

    /* Parameter 1 is the Storage ID.  */
    command.ux_host_class_pima_command_parameter_1 =  storage_id;

    /* Other parameters unused.  */
    command.ux_host_class_pima_command_parameter_2 =  0;
    command.ux_host_class_pima_command_parameter_3 =  0;
    command.ux_host_class_pima_command_parameter_4 =  0;
    command.ux_host_class_pima_command_parameter_5 =  0;

    /* Then set the command to GET_STORAGE_INFO.  */
    command.ux_host_class_pima_command_operation_code =  UX_HOST_CLASS_PIMA_OC_GET_STORAGE_INFO;

    /* Allocate some DMA safe memory for receiving the storage info block.  */
    storage_buffer =  _ux_utility_memory_allocate(UX_SAFE_ALIGN, UX_CACHE_SAFE_MEMORY, UX_HOST_CLASS_PIMA_STORAGE_MAX_LENGTH);
    if (storage == UX_NULL)
        return(UX_MEMORY_INSUFFICIENT);

    /* Issue the command.  */
    status = _ux_host_class_pima_command(pima, &command, UX_HOST_CLASS_PIMA_DATA_PHASE_IN , storage_buffer,
                                        UX_HOST_CLASS_PIMA_STORAGE_MAX_LENGTH, UX_HOST_CLASS_PIMA_STORAGE_MAX_LENGTH);

    /* Check the result. If the result is OK, the storage info block was read properly. */
    if (status == UX_SUCCESS)
    {
        /* Uncompress the storage descriptor, at least the fixed part.  */
        _ux_utility_descriptor_parse(storage_buffer,
                            _ux_system_class_pima_object_structure,
                            UX_HOST_CLASS_PIMA_OBJECT_ENTRIES,
                            (UCHAR *) storage);

        /* Copy the storage description field.  Point to the beginning of the storage description string.  */
        storage_pointer =  storage_buffer + UX_HOST_CLASS_PIMA_STORAGE_VARIABLE_OFFSET;

        /* Get the unicode string length.  */
        unicode_string_length =  (ULONG) *storage_pointer ;

        /* Copy that string into the storage description field.  */
        _ux_utility_memory_copy(storage -> ux_host_class_pima_storage_description, storage_pointer, unicode_string_length); /* Use case of memcpy is verified. */ // VULN: unchecked copy size for a copy in a fixed size (256 bytes) buffer (UX_HOST_CLASS_PIMA_STORAGE_MAX_LENGTH used to dynamically allocate storage_buffer is 512 bytes)

        /* Point to the volume label.  */
        storage_pointer =  storage_buffer + UX_HOST_CLASS_PIMA_STORAGE_VARIABLE_OFFSET + unicode_string_length;

        /* Get the unicode string length.  */
        unicode_string_length =  (ULONG) *storage_pointer ;

        /* Copy that string into the storage volume label field.  */
        _ux_utility_memory_copy(storage -> ux_host_class_pima_storage_volume_label, storage_pointer, unicode_string_length); /* Use case of memcpy is verified. */ // VULN: unchecked copy size for a copy in a fixed size (256 bytes) buffer (UX_HOST_CLASS_PIMA_STORAGE_MAX_LENGTH used to dynamically allocate storage_buffer is 512 bytes)

    }

    /* Free the original storage info buffer.  */
    _ux_utility_memory_free(storage_buffer);

    /* Return completion status.  */
    return(status);
}
```


--[ 4 - Affected products

Eclipse ThreadX before version 6.4.0 was affected by the vulnerabilities
discussed in this advisory. The other bugs in Eclipse ThreadX NetX Duo and USBX
that we reported will be patched in subsequent releases of Eclipse ThreadX.


--[ 5 - Remediation

Eclipse ThreadX maintainers have fixed all vulnerabilities discussed in this
advisory.

Please check the official Eclipse ThreadX channels for further information
about fixes.


--[ 6 - Disclosure timeline

We reported the vulnerabilities discussed in this advisory to Microsoft in
December 2023 and early January 2024, via their MSRC Researcher Portal [8].
For our efforts, we were awarded 7th place in the Top MSRC 2023 Q4 Azure
Security Researchers Leaderboard [9].

Following the project ownership transfer to the Eclipse Foundation, with
Microsoft's help, we coordinated with the new maintainers to provide
vulnerability information and fixes to the ThreadX users' community.

The (simplified) coordinated disclosure timeline follows:

2023-12-01: Reported two vulnerabilities to MSRC.
2023-12-13: MSRC confirmed our first vulnerability.
2023-12-14: MSRC confirmed our second vulnerability.
2023-12-21: Reported other two vulnerabilities to MSRC.
2023-12-31: Reported another vulnerability to MSRC.
2024-01-02: Reported other three vulnerabilities to MSRC.
2024-01-05: MSRC confirmed a vulnerability was fixed in version 6.4.
2024-01-06: MSRC informed us we made the 2023 Q4 leaderboard!
2024-01-10: MSRC confirmed another vulnerability was fixed in version 6.4.
2024-02-10: Asked MSRC for updates on all open reports.
2024-02-16: Asked the Eclipse Foundation for advice on how to proceed.
2024-02-20: Eclipse replied they were coordinating with MSRC.
2024-02-21: MSRC informed us of the ownership transfer to Eclipse.
2024-02-27: MSRC confirmed a third vulnerability was fixed in version 6.4.
2024-02-28: Upon MSRC's request, we submitted our reports to Eclipse.
2024-03-05: Eclipse started creating GitHub advisories for all reports.
2024-03-13: Provided Eclipse with clarifications on some of the reports.
2024-03-15: MSRC provided some additional feedback on the transition.
2024-03-18: Eclipse finished creating GitHub advisories for all reports.
2024-03-22: MSRC closed the remaining cases after the transition.
2024-03-25: Eclipse published CVE-2024-2212, CVE-2024-2214, CVE-2024-2452.
2024-03-29: Provided Eclipse with clarifications on remaining reports.
2024-04-24: Asked for a status update on the remaining reports.
2024-04-26: Agreed to declass the remaining reports to standard issues.
2024-05-02: Sent draft advisory and writeup to MSRC and Eclipse.
2024-05-28: Published advisory and writeup.


--[ 7 - Acknowledgments

We would like to thank MSRC and the Eclipse Foundation (with a special mention
to Marta Rybczynska who took care of coordinated disclosure after the project
ownership change) for triaging and fixing the reported vulnerabilities. It was
a pleasure to work with you!


--[ 8 - References

[1] https://eclipse-foundation.blog/2023/11/21/introducing-eclipse-threadx/
[2] https://github.com/eclipse-threadx/
[3] https://security.humanativaspa.it/ost2-zephyr-rtos-and-a-bunch-of-cves/
[4] https://security.humanativaspa.it/multiple-vulnerabilities-in-rt-thread-rtos/
[5] https://security.humanativaspa.it/multiple-vulnerabilities-in-riot-os/
[6] https://security.humanativaspa.it/big-update-to-my-semgrep-c-cpp-ruleset/
[7] https://security.humanativaspa.it/a-collection-of-weggli-patterns-for-c-cpp-vulnerability-research/
[8] https://msrc.microsoft.com/report/vulnerability
[9] https://msrc.microsoft.com/blog/2024/01/congratulations-to-the-top-msrc-2023-q4-security-researchers/


Copyright (c) 2024 Marco Ivaldi and Humanativa Group. All rights reserved.