buffer overflow in USBDevice

[hac425xxx]

bug 1 - buffer overflow in USBD_CtrlReceiveData

path

Device/usbd_ctrl.c

code

 
USBD_ReturnType USBD_CtrlReceiveData(USBD_HandleType *dev, void *data)
{
    USBD_ReturnType retval = USBD_E_ERROR;

    /* Sanity check */
    if (dev->EP.OUT[0].State == USB_EP_STATE_SETUP)
    {
        uint16_t len = dev->Setup.Length;  // recv from other USB device.

        dev->EP.OUT[0].State = USB_EP_STATE_DATA;
        USBD_PD_EpReceive(dev, 0x00, (uint8_t*)data, len);

        retval = USBD_E_OK;
    }
    return retval;
}

dev->Setup.Length is recv from other USB device, if dev->Setup.Length > the size of data, it will overflow.

[hac425xxx]

bug 2 - out of bound read and buffer overflow in dfu_upload

path

Class/DFU/usbd_dfu.c

code

static USBD_ReturnType dfu_upload(USBD_DFU_IfHandleType *itf)
{

    USBD_HandleType *dev = itf->Base.Device;


    if ((dev->Setup.Length > 0) && (DFU_APP(itf)->Read != NULL))
    {

        /* Read memory */
        else if (itf->BlockNum > 1)
        {
            itf->DevStatus.State = DFU_STATE_UPLOAD_IDLE;

            DFU_APP(itf)->Read(
                    DFUSE_GETADDRESS(itf, &dfu_desc),
                    data,
                    dev->Setup.Length);   // bug !!!

            retval = USBD_CtrlSendData(dev, data, dev->Setup.Length);
        }

if dev->Setup.Length is too large, it could lead out of bound read, or overflow

case 2

        /* Check for correct sequence */
        if (dev->Setup.Value == ((itf->BlockNum + 1) & 0xFFFF))
        {
            uint16_t len;
            uint32_t progress = (uint32_t)itf->Address - DFU_APP(itf)->Firmware.Address;

            /* Shorten the block size if it's the end of the firmware memory,
             * return to IDLE */
            if ((progress + dev->Setup.Length) > DFU_APP(itf)->Firmware.TotalSize)
            {
                len = DFU_APP(itf)->Firmware.TotalSize - progress;
                itf->DevStatus.State = DFU_STATE_IDLE;
            }
            else
            {
                len = dev->Setup.Length;
                itf->DevStatus.State = DFU_STATE_UPLOAD_IDLE;
            }

            DFU_APP(itf)->Read(itf->Address, data, len);

it could check dev->Setup.Length with Firmware.TotalSize.

(progress + dev->Setup.Length) > DFU_APP(itf)->Firmware.TotalSize

the length of data is 256 bytes, but Firmware.TotalSize is more large than 256, such as 24568

make TARGET_HEADER="\<stm32f042x6.h\>" SERIES=STM32F0 APP_ADDRESS=0x08002000 APP_SIZE=24568 TOTAL_ERASE_MS=480 VID=FFFF PID=FFFF

it could lead buffer overflow when read data to data.

DFU_APP(itf)->Read(itf->Address, data, len);

[hac425xxx]

bug 3- buffer overflow in dfu_download

path

Class/DFU/usbd_dfu.c

code

            /* Checks for valid sequence and overall length */
            if (   ( dev->Setup.Value == ((itf->BlockNum + 1) & 0xFFFF))
                && (((uint32_t)itf->Address + dev->Setup.Length) <
                    (DFU_APP(itf)->Firmware.Address + DFU_APP(itf)->Firmware.TotalSize)))

            {
                
                /* Prepare the reception of the buffer over EP0 */
                retval = USBD_CtrlReceiveData(dev, dev->CtrlData);
            }

it could check dev->Setup.Length with Firmware.TotalSize, but Firmware.TotalSize is more large than the length of data

[benedekkupper]

Thanks for this analysis, I have provided fixes that should remedy these exploitable bugs. Please keep me updated with newer results. Just out of curiosity, what tool are you using to analyze the code?

[hac425xxx]

I analyze the code by vscode.

these patch seems ok.

I also have a question

                /* HID report OUT */
                case HID_REQ_SET_REPORT:
                {
                    uint16_t max_len;
                    if (reportType == HID_REPORT_OUTPUT)
                    {
                        max_len = HID_APP(itf)->Report->Output.MaxSize;
                    }
                    else
                    {
                        max_len = HID_APP(itf)->Report->Feature.MaxSize;
                    }
                    retval = USBD_CtrlReceiveData(dev, dev->CtrlData, max_len);
                    break;
                }

Where is the .MaxSize field defined?

[benedekkupper]

Those are defined by the author of the application, as part of a const struct.

[hac425xxx]

ok, and do we need to verify that MaxSize smaller than the size of CtrlData?