Closed szymonh closed 2 years ago
ALABSTM
commented
2 years ago Hi @szymonh,
Thank you very much for this exhaustive description and the proposal you made. It will be forwarded to our development teams. We will get back to you as soon as we have their feedback.
With regards,
ALABSTM
commented
2 years ago Fix proposal reworded:
The code bloc below is executed in case USBH_MAX_EP_PACKET_SIZE equals USBH_MAX_DATA_BUFFER. In this case, it must be ensured that ep_descriptor->wMaxPacketSize is not allowed to be greater than USBH_MAX_EP_PACKET_SIZE.
ALABSTM
commented
2 years ago ST Internal Reference: 118017
szymonh
commented
2 years ago Hello @ALABSTM ,
Since this issue may result in a buffer overflow affecting application security are you planning to publish a corresponding CVE?
Best regards, Szymon
Summary
USB host implementation is vulnerable to buffer overflows due to implementation of USBH_ParseEPDesc when _USBH_MAX_EP_PACKETSIZE is equal to _USBH_MAX_DATABUFFER.
Description
The function responsible for endpoint descriptor parsing _USBHParseEPDesc as defined in _usbhctlreq.c does not limit the value of _epdescriptor->wMaxPacketSize when _USBH_MAX_EP_PACKETSIZE is equal to _USBH_MAX_DATABUFFER. By default _USBH_MAX_EP_PACKETSIZE is set to 0x400 (as in usbh_def.h) and _USBH_MAX_DATABUFFER also equals to 0x400 (again as defined in usbh_def.h). In contrast usbh_conf_template.h defines USBH_MAX_DATA_BUFFER to be 0x200U.
With _USBH_MAX_EP_PACKETSIZE == _USBH_MAX_DATABUFFER ep_descriptor->wMaxPacketSize is directly derived from the endpoint descriptor.
` static USBH_StatusTypeDef USBH_ParseEPDesc(USBH_HandleTypeDef phost, USBH_EpDescTypeDef ep_descriptor, uint8_t buf) { USBH_StatusTypeDef status = USBH_OK; ep_descriptor->bLength = (uint8_t )(buf + 0); ep_descriptor->bDescriptorType = (uint8_t )(buf + 1); ep_descriptor->bEndpointAddress = (uint8_t )(buf + 2); ep_descriptor->bmAttributes = (uint8_t )(buf + 3); ep_descriptor->wMaxPacketSize = LE16(buf + 4); ep_descriptor->bInterval = (uint8_t *)(buf + 6);
/ Make sure that wMaxPacketSize is different from 0 / if (ep_descriptor->wMaxPacketSize == 0x00U) { status = USBH_NOT_SUPPORTED; } else if (USBH_MAX_EP_PACKET_SIZE < (uint16_t)USBH_MAX_DATA_BUFFER) { / Make sure that maximum packet size (bits 0..10) does not exceed the max endpoint packet size / ep_descriptor->wMaxPacketSize &= ~0x7FFU; ep_descriptor->wMaxPacketSize |= MIN((uint16_t)(LE16(buf + 4) & 0x7FFU), (uint16_t)USBH_MAX_EP_PACKET_SIZE);
} else if ((uint16_t)USBH_MAX_DATA_BUFFER < USBH_MAX_EP_PACKET_SIZE) { / Make sure that maximum packet size (bits 0..10) does not exceed the total buffer length / ep_descriptor->wMaxPacketSize &= ~0x7FFU; ep_descriptor->wMaxPacketSize |= MIN((uint16_t)(LE16(buf + 4) & 0x7FFU), (uint16_t)USBH_MAX_DATA_BUFFER); } else { / ... / } `
Consequently _epdescriptor->wMaxPacketSize value will be set as follows:
In case of a malicious device the wMaxPacketSize value provided in the endpoint descriptor may have a unexpected large value like 0xffff. This case will result in buffer overflows in multiple locations in the USB Host stack like:
Impact
Devices implementing usb host functionality based on stm32_mw_usb_host middleware are vulnerable to multiple buffer overflows due to endpoint descriptor parsing implemented in _USBHParseEPDesc in case _USBH_MAX_EP_PACKETSIZE is equal to _USBH_MAX_DATABUFFER. Depending on particular code location affected by unexpectedly large value of _epdescriptor->wMaxPacketSize this may result in write past buffer boundaries, bypass of security features or in the worst case scenario execution of arbitrary code.
Expected resolution
Assure that _epdescriptor->wMaxPacketSize is not permitted to be greater than _USBH_MAX_EP_PACKETSIZE when _USBH_MAX_EP_PACKETSIZE equals to _USBH_MAX_DATABUFFER.