From 0734bef862ed1490e94897b06ab59e838d99728e Mon Sep 17 00:00:00 2001 From: Tomas Rezucha Date: Fri, 27 Mar 2026 13:49:16 +0100 Subject: [PATCH] fix(usb): USB security fixes from esp-usb Combination of the following commits: - https://github.com/espressif/esp-usb/commit/c546816048e449f11c57ca9a1e0541e592d04c07 - https://github.com/espressif/esp-usb/commit/749af3b938aeb95e1c5b4926f2e28eb2fe65f381 - https://github.com/espressif/esp-usb/commit/234ed4e9d404590011ef61826220f403fc52bb54 - https://github.com/espressif/esp-usb/commit/8f7fe7308d39ec69d3d8822691d74f43e71da331 --- components/usb/enum.c | 15 ++++- components/usb/include/usb/usb_helpers.h | 4 +- components/usb/include/usb/usb_types_ch9.h | 7 ++- components/usb/usb_helpers.c | 68 +++++++++++++++++----- 4 files changed, 75 insertions(+), 19 deletions(-) diff --git a/components/usb/enum.c b/components/usb/enum.c index fc93a38f72..d42dc5500c 100644 --- a/components/usb/enum.c +++ b/components/usb/enum.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -168,7 +168,7 @@ const char *ENUM_TAG = "ENUM"; // ---------------------------- Helpers ---------------------------------------- // ----------------------------------------------------------------------------- #define ENUM_CHECK(cond, ret_val) ({ \ - if (!(cond)) { \ + if (unlikely(!(cond))) { \ return (ret_val); \ } \ }) @@ -541,6 +541,10 @@ static esp_err_t parse_short_config_desc(void) usb_transfer_t *ctrl_xfer = &p_enum_driver->constant.urb->transfer; const usb_config_desc_t *config_desc = (usb_config_desc_t *)(ctrl_xfer->data_buffer + sizeof(usb_setup_packet_t)); + // Validate actual received data size to prevent OOB reads + const int actual_data_len = ctrl_xfer->actual_num_bytes - sizeof(usb_setup_packet_t); + ENUM_CHECK(actual_data_len >= ENUM_SHORT_DESC_REQ_LEN, ESP_ERR_INVALID_RESPONSE); + // Check if the returned descriptor is corrupted if (config_desc->bDescriptorType != USB_B_DESCRIPTOR_TYPE_CONFIGURATION) { ESP_LOGE(ENUM_TAG, "Short config desc has wrong bDescriptorType"); @@ -577,6 +581,10 @@ static esp_err_t parse_full_config_desc(void) usb_transfer_t *ctrl_xfer = &p_enum_driver->constant.urb->transfer; const usb_config_desc_t *config_desc = (usb_config_desc_t *)(ctrl_xfer->data_buffer + sizeof(usb_setup_packet_t)); + // Validate actual received data size to prevent OOB reads + const int actual_data_len = ctrl_xfer->actual_num_bytes - sizeof(usb_setup_packet_t); + ENUM_CHECK(actual_data_len >= ENUM_SHORT_DESC_REQ_LEN, ESP_ERR_INVALID_RESPONSE); + // Check if the returned descriptor is corrupted if (config_desc->bDescriptorType != USB_B_DESCRIPTOR_TYPE_CONFIGURATION) { ESP_LOGE(ENUM_TAG, "Full config desc has wrong bDescriptorType"); @@ -640,6 +648,9 @@ static esp_err_t parse_langid_table(void) usb_transfer_t *transfer = &p_enum_driver->constant.urb->transfer; const usb_str_desc_t *str_desc = (usb_str_desc_t *)(transfer->data_buffer + sizeof(usb_setup_packet_t)); + // Validate minimum LANGID descriptor length: header (2 bytes) + at least 1 LANGID entry (2 bytes) + ENUM_CHECK(str_desc->bLength >= (sizeof(usb_str_desc_t) + 2), ESP_ERR_INVALID_RESPONSE); + //Scan the LANGID table for our target LANGID ret = ESP_ERR_NOT_FOUND; int langid_table_num_entries = (str_desc->bLength - sizeof(usb_str_desc_t)) / 2; // Each LANGID is 2 bytes diff --git a/components/usb/include/usb/usb_helpers.h b/components/usb/include/usb/usb_helpers.h index 63810b830d..48f465a39d 100644 --- a/components/usb/include/usb/usb_helpers.h +++ b/components/usb/include/usb/usb_helpers.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -79,7 +79,7 @@ int usb_parse_interface_number_of_alternate(const usb_config_desc_t *config_desc * @param[in] bInterfaceNumber Interface number * @param[in] bAlternateSetting Alternate setting number * @param[out] offset Byte offset of the interface descriptor relative to the start of the configuration descriptor. Can be NULL. - * @return const usb_intf_desc_t* Pointer to interface descriptor, NULL if not found. + * @return const usb_intf_desc_t* Pointer to interface descriptor, NULL if not found or invalid (e.g., too many endpoints) descriptor */ const usb_intf_desc_t *usb_parse_interface_descriptor(const usb_config_desc_t *config_desc, uint8_t bInterfaceNumber, uint8_t bAlternateSetting, int *offset); diff --git a/components/usb/include/usb/usb_types_ch9.h b/components/usb/include/usb/usb_types_ch9.h index 500a5dc51c..f54f94842a 100644 --- a/components/usb/include/usb/usb_types_ch9.h +++ b/components/usb/include/usb/usb_types_ch9.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -411,6 +411,11 @@ ESP_STATIC_ASSERT(sizeof(usb_iad_desc_t) == USB_IAD_DESC_SIZE, "Size of usb_iad_ */ #define USB_INTF_DESC_SIZE 9 +/** + * @brief Maximum number of endpoints in a USB interface descriptor + */ +#define USB_MAX_ENDPOINTS_PER_INTERFACE 32 // 16 IN and 16 OUT + /** * @brief Structure representing a USB interface descriptor * diff --git a/components/usb/usb_helpers.c b/components/usb/usb_helpers.c index 08c50e3b87..2d53860c6d 100644 --- a/components/usb/usb_helpers.c +++ b/components/usb/usb_helpers.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -20,15 +20,24 @@ const usb_standard_desc_t *usb_parse_next_descriptor(const usb_standard_desc_t *cur_desc, uint16_t wTotalLength, int *offset) { assert(cur_desc != NULL && offset != NULL); + if (cur_desc->bLength < sizeof(usb_standard_desc_t)) { + return NULL; // Invalid descriptor length, would cause infinite loop + } if (*offset >= wTotalLength) { return NULL; // We have traversed the entire configuration descriptor } - if (*offset + cur_desc->bLength >= wTotalLength) { - return NULL; // Next descriptor is out of bounds + /* + * The returned pointer must address at least sizeof(usb_standard_desc_t) bytes, + * because callers read bLength and bDescriptorType immediately (e.g. usb_print_config_descriptor()). + * Reject advancing when fewer than that many bytes remain in [next_offset, wTotalLength). + */ + const int next_offset = *offset + cur_desc->bLength; + if (next_offset + (int)sizeof(usb_standard_desc_t) > (int)wTotalLength) { + return NULL; // Next descriptor start out of bounds or trailing fragment < sizeof(usb_standard_desc_t) } // Return the next descriptor, update offset - const usb_standard_desc_t *ret_desc = (const usb_standard_desc_t *)(((uint32_t)cur_desc) + cur_desc->bLength); - *offset += cur_desc->bLength; + const usb_standard_desc_t *ret_desc = (const usb_standard_desc_t *)(((uintptr_t)cur_desc) + cur_desc->bLength); + *offset = next_offset; return ret_desc; } @@ -37,16 +46,36 @@ const usb_standard_desc_t *usb_parse_next_descriptor_of_type(const usb_standard_ assert(cur_desc != NULL && offset != NULL); int offset_temp = *offset; // We only want to update offset if we've actually found a descriptor // Keep stepping over descriptors until we find one of bDescriptorType or until we go out of bounds - const usb_standard_desc_t *ret_desc = usb_parse_next_descriptor(cur_desc, wTotalLength, &offset_temp); - while (ret_desc != NULL) { + const usb_standard_desc_t *ret_desc = cur_desc; + while ((ret_desc = usb_parse_next_descriptor(ret_desc, wTotalLength, &offset_temp)) != NULL) { if (ret_desc->bDescriptorType == bDescriptorType) { + switch (bDescriptorType) { + case USB_B_DESCRIPTOR_TYPE_CONFIGURATION: + if (ret_desc->bLength < sizeof(usb_config_desc_t)) { + return NULL; + } + break; + case USB_B_DESCRIPTOR_TYPE_INTERFACE: + if (ret_desc->bLength < sizeof(usb_intf_desc_t)) { + return NULL; + } + break; + case USB_B_DESCRIPTOR_TYPE_ENDPOINT: + if (ret_desc->bLength < sizeof(usb_ep_desc_t)) { + return NULL; + } + break; + case USB_B_DESCRIPTOR_TYPE_INTERFACE_ASSOCIATION: + if (ret_desc->bLength < sizeof(usb_iad_desc_t)) { + return NULL; + } + break; + default: + break; + } + *offset = offset_temp; // Found: advance caller's offset past descriptors we stepped over break; } - ret_desc = usb_parse_next_descriptor(ret_desc, wTotalLength, &offset_temp); - } - if (ret_desc != NULL) { - // We've found a descriptor. Update the offset - *offset = offset_temp; } return ret_desc; } @@ -104,8 +133,16 @@ const usb_intf_desc_t *usb_parse_interface_descriptor(const usb_config_desc_t *c // Get the next interface descriptor next_intf_desc = (const usb_intf_desc_t *)usb_parse_next_descriptor_of_type((const usb_standard_desc_t *)next_intf_desc, config_desc->wTotalLength, USB_B_DESCRIPTOR_TYPE_INTERFACE, &offset_temp); } - if (next_intf_desc != NULL && offset != NULL) { - *offset = offset_temp; + + if (next_intf_desc != NULL) { + // In case offset was provided, set it + if (offset != NULL) { + *offset = offset_temp; + } + // Check if the interface descriptor has too many endpoints + if (next_intf_desc->bNumEndpoints > USB_MAX_ENDPOINTS_PER_INTERFACE) { + return NULL; // Too many endpoints, invalid descriptor + } } return next_intf_desc; } @@ -199,6 +236,9 @@ static void print_ep_desc(const usb_ep_desc_t *ep_desc) USB_EP_DESC_GET_EP_DIR(ep_desc) ? "IN" : "OUT"); printf("\t\tbmAttributes 0x%x\t%s\n", ep_desc->bmAttributes, ep_type_str); printf("\t\twMaxPacketSize %d\n", USB_EP_DESC_GET_MPS(ep_desc)); + if (USB_EP_DESC_GET_MULT(ep_desc) > 0) { + printf("\t\twMaxPacketSize (with additional transactions in microframe) %d\n", USB_EP_DESC_GET_MPS(ep_desc) * (USB_EP_DESC_GET_MULT(ep_desc) + 1)); + } printf("\t\tbInterval %d\n", ep_desc->bInterval); }