Impact
Looking at the implementation of usb_dfu.c in Zephyr’s github repository I believe that the implementation is not fully secure.
Tracing code execution path in usb_device.c usb_handle_control_transfer function when one sends a control transfer read request (bmRequestType direction set to 1, device to host transfer) the wLength size exceeding CONFIG_USB_REQUEST_BUFFER_SIZE will be permitted (usb_device.c: 310) and a installed handler will be used to further process the request (usb_device.c: 332). Next in dfu_class_handle_req (usb_dfu.c) for bRequest set to DFU_DNLOAD for both dfuIDLE and dfuDNLOAD_IDLE states memcopy of wLenght bytes will occur. Since wLength value was provided in the frame and might have a value significantly larger than CONFIG_USB_REQUEST_BUFFER_SIZE – i.e. 0xffff, data will be copied violating buffer boundaries from both dfu_data_worker.buf and *data perspective. Depending on memory layout and possibility to populate memory past end of data buffer this might allow one to modify memory past dfu_data_worker.buf to either corrupt data structures or achieve arbitrary code execution.
Furthermore since the supplies wLength is assigned to dfu_data_worker.worker_len it looks like that the dfu worker will write memory copied past dfu_data_worker to flash, effectively leaking information, since one can readout the flash contents (i.e. using UPLOAD command with valid wLength).
Finally back in usb_device.c. line 344 – wLength bytes of data will be sent to the host. Again this value can be significantly larger then the actual buffer size resulting in readout past buffer boundaries (usb_dev.data_buf_residue was set to MIN(usb_dev.data_buf_len, setup->wLength) which equals to setup->wLength as usb_dev.data_buf_len = setup->wLength in usb_handle_control_transfer, line 341). Looks like a serious memory readout problem.
I might be wrong but a similar issue with readout just as above seems to be lurking also for other commands when issues as reads with a fairly large value of wLength and *data_len is not altered by the command handler i.e. DFU_ABORT, DFU_CLRSTATUS or DFU_DETACH. My understanding after reading the code is that again wLength bytes of data will be sent back to the host passing the CONFIG_USB_REQUEST_BUFFER_SIZE Buffer boundary.
Looking at other device classes like cdc_acm the cdc_acm_class_handle_req again seems to permit behavior similar to described above. For requests such as SET_LINE_CODING or SET_CONTROL_LINE_STATE the response to host (bmRequestType direction again set to 1) will not update the response length value allowing readout past memory buffer boundaries. So the problem might be a bit more wide-spread than only DFU.
Hope the above description makes sense. I added code snippets with comments below to make the description a bit clearer.
Unfortunately I don’t have the hardware to verify the issue.
In case I’m wrong please provide some explanation why you consider the implementation secure as is so I can better understand the case.
usb_dfu.c : 455
case DFU_DNLOAD:
LOG_DBG("DFU_DNLOAD block %d, len %d, state %d",
pSetup->wValue, pSetup->wLength, dfu_data.state);
if (dfu_check_app_state()) {
return -EINVAL;
}
switch (dfu_data.state) {
case dfuIDLE:
LOG_DBG("DFU_DNLOAD start");
dfu_reset_counters();
k_poll_signal_reset(&dfu_signal);
if (dfu_data.flash_area_id !=
UPLOAD_FLASH_AREA_ID) {
dfu_data.status = errWRITE;
dfu_data.state = dfuERROR;
LOG_ERR("This area can not be overwritten");
break;
}
dfu_data.state = dfuDNBUSY;
dfu_data_worker.worker_state = dfuIDLE;
dfu_data_worker.worker_len = pSetup->wLength; // SH: data past buffer bounds will be written to flash
memcpy(dfu_data_worker.buf, *data, pSetup->wLength); // SH: memcopy past buffer bound
k_work_submit_to_queue(&USB_WORK_Q, &dfu_work);
break;
case dfuDNLOAD_IDLE:
dfu_data.state = dfuDNBUSY;
dfu_data_worker.worker_state = dfuDNLOAD_IDLE;
dfu_data_worker.worker_len = pSetup->wLength; // SH: data past buffer bounds will be written to flash
memcpy(dfu_data_worker.buf, *data, pSetup->wLength); // SH: memcopy past buffer bound
k_work_submit_to_queue(&USB_WORK_Q, &dfu_work);
break;
usb_device.c: 282
static void usb_handle_control_transfer(uint8_t ep,
enum usb_dc_ep_cb_status_code ep_status)
{
uint32_t chunk = 0U;
struct usb_setup_packet *setup = &usb_dev.setup;
struct usb_setup_packet_packed setup_raw;
LOG_DBG(“ep 0x%02x, status 0x%02x”, ep, ep_status);
if (ep == USB_CONTROL_OUT_EP0 && ep_status == USB_DC_EP_SETUP) {
/*
* OUT transfer, Setup packet,
* reset request message state machine
*/
if (usb_dc_ep_read(ep, (uint8_t *)&setup_raw,
sizeof(setup_raw), NULL) < 0) {
LOG_DBG(“Read Setup Packet failed”);
usb_dc_ep_set_stall(USB_CONTROL_IN_EP0);
return;
}
/* Take care of endianness */
setup->bmRequestType = setup_raw.bmRequestType;
setup->bRequest = setup_raw.bRequest;
setup->wValue = sys_le16_to_cpu(setup_raw.wValue);
setup->wIndex = sys_le16_to_cpu(setup_raw.wIndex);
setup->wLength = sys_le16_to_cpu(setup_raw.wLength); // SH: wLength is extracted from incoming usb frame
if (setup->wLength > CONFIG_USB_REQUEST_BUFFER_SIZE) {
if (REQTYPE_GET_DIR(setup->bmRequestType)
!= REQTYPE_DIR_TO_HOST) { // SH: bmRequestType dir set to TO_HOST to proceed
LOG_ERR("Request buffer too small");
usb_dc_ep_set_stall(USB_CONTROL_IN_EP0);
usb_dc_ep_set_stall(USB_CONTROL_OUT_EP0);
return;
}
}
usb_dev.data_buf = usb_dev.req_data;
usb_dev.data_buf_residue = setup->wLength;
usb_dev.data_buf_len = setup->wLength;
usb_dev.zlp_flag = false;
if (setup->wLength &&
REQTYPE_GET_DIR(setup->bmRequestType)
== REQTYPE_DIR_TO_DEVICE) {
return;
}
/* Ask installed handler to process request */
if (!usb_handle_request(setup,
&usb_dev.data_buf_len,
&usb_dev.data_buf)) {
LOG_DBG("usb_handle_request failed");
usb_dc_ep_set_stall(USB_CONTROL_IN_EP0);
return;
}
/* Send smallest of requested and offered length */
usb_dev.data_buf_residue = MIN(usb_dev.data_buf_len, // SH: if usb_dev.data_buf_len if not altered in handler (like in DFU_ABORT etc.) both values are the same
setup->wLength);
/* Send first part (possibly a zero-length status message) */
usb_data_to_host(setup->wLength); // SH: wLength > CONFIG_USB_REQUEST_BUFFER_SIZE Will be returned to host leaking memory contents
Patches
This has been fixed in:
- main #36694 (2.7.0)
- v1.14: TBD
For more information
If you have any questions or comments about this advisory:
embargo: 2021-09-21
szymonh Analyst
Impact
Looking at the implementation of usb_dfu.c in Zephyr’s github repository I believe that the implementation is not fully secure.
Tracing code execution path in usb_device.c usb_handle_control_transfer function when one sends a control transfer read request (bmRequestType direction set to 1, device to host transfer) the wLength size exceeding CONFIG_USB_REQUEST_BUFFER_SIZE will be permitted (usb_device.c: 310) and a installed handler will be used to further process the request (usb_device.c: 332). Next in dfu_class_handle_req (usb_dfu.c) for bRequest set to DFU_DNLOAD for both dfuIDLE and dfuDNLOAD_IDLE states memcopy of wLenght bytes will occur. Since wLength value was provided in the frame and might have a value significantly larger than CONFIG_USB_REQUEST_BUFFER_SIZE – i.e. 0xffff, data will be copied violating buffer boundaries from both dfu_data_worker.buf and *data perspective. Depending on memory layout and possibility to populate memory past end of data buffer this might allow one to modify memory past dfu_data_worker.buf to either corrupt data structures or achieve arbitrary code execution.
Furthermore since the supplies wLength is assigned to dfu_data_worker.worker_len it looks like that the dfu worker will write memory copied past dfu_data_worker to flash, effectively leaking information, since one can readout the flash contents (i.e. using UPLOAD command with valid wLength).
Finally back in usb_device.c. line 344 – wLength bytes of data will be sent to the host. Again this value can be significantly larger then the actual buffer size resulting in readout past buffer boundaries (usb_dev.data_buf_residue was set to MIN(usb_dev.data_buf_len, setup->wLength) which equals to setup->wLength as usb_dev.data_buf_len = setup->wLength in usb_handle_control_transfer, line 341). Looks like a serious memory readout problem.
I might be wrong but a similar issue with readout just as above seems to be lurking also for other commands when issues as reads with a fairly large value of wLength and *data_len is not altered by the command handler i.e. DFU_ABORT, DFU_CLRSTATUS or DFU_DETACH. My understanding after reading the code is that again wLength bytes of data will be sent back to the host passing the CONFIG_USB_REQUEST_BUFFER_SIZE Buffer boundary.
Looking at other device classes like cdc_acm the cdc_acm_class_handle_req again seems to permit behavior similar to described above. For requests such as SET_LINE_CODING or SET_CONTROL_LINE_STATE the response to host (bmRequestType direction again set to 1) will not update the response length value allowing readout past memory buffer boundaries. So the problem might be a bit more wide-spread than only DFU.
Hope the above description makes sense. I added code snippets with comments below to make the description a bit clearer.
Unfortunately I don’t have the hardware to verify the issue.
In case I’m wrong please provide some explanation why you consider the implementation secure as is so I can better understand the case.
usb_dfu.c : 455
usb_device.c: 282
static void usb_handle_control_transfer(uint8_t ep,
{
Patches
This has been fixed in:
For more information
If you have any questions or comments about this advisory:
embargo: 2021-09-21