dmitrystu / sboot_stm32

Secure USB DFU1.1 bootloader for STM32
Apache License 2.0
303 stars 63 forks source link

Multiple Memory Overflow Vulnerabilities In dfu_control #35

Closed hac425xxx closed 3 years ago

hac425xxx commented 3 years ago

The definition of the dfu_control function is as follows:


static usbd_respond dfu_control (usbd_device *dev, usbd_ctlreq *req, usbd_rqc_callback *callback) {

req is receive from the external usb device.

it's struct is as follows:

/**\brief Represents generic USB control request.*/
typedef struct {
    uint8_t     bmRequestType;  /**<\brief This bitmapped field identifies the characteristics of
                                 * the specific request.*/
    uint8_t     bRequest;       /**<\brief This field specifies the particular request.*/
    uint16_t    wValue;         /**<\brief It is used to pass a parameter to the device, specific to
                                 * the request.*/
    uint16_t    wIndex;         /**<\brief It is used to pass a parameter to the device, specific to
                                 * the request.*/
    uint16_t    wLength;        /**<\brief This field specifies the length of the data transferred
                                 * during the second phase of the control transfer.*/
    uint8_t     data[];         /**<\brief Data payload.*/
} usbd_ctlreq;

BUG:

        case USB_DFU_DNLOAD:
            return dfu_dnload(req->data, req->wLength);
        case USB_DFU_UPLOAD:
            return dfu_upload(dev, req->wLength);

for USB_DFU_DNLOAD case, if req->wLength > the sizeof req->data , it could trigger buffer overflow in dfu_dnload.

for USB_DFU_UPLOAD case, if req->wLength > the sizeof dev->status.data_ptr , it could trigger buffer overflow in dfu_upload.

static usbd_respond dfu_upload(usbd_device *dev, size_t blksize) {
    switch (dfu_data.bState) {
    case USB_DFU_STATE_DFU_IDLE:
    case USB_DFU_STATE_DFU_UPLOADIDLE:
        if (dfu_data.remained == 0) {
            dev->status.data_count = 0;
            return dfu_set_idle();
        } else if (dfu_data.remained < DFU_BLOCKSZ) {
            blksize = dfu_data.remained;
        }
        aes_encrypt(dev->status.data_ptr, dfu_data.dptr, blksize);  // overflow !!!
dmitrystu commented 3 years ago

Thank you for pointing to this vulnerability. I think this can't be used as an exploit to extract key or protected data, but it can cause unpredictable behavior. Could you check PR?

hac425xxx commented 3 years ago

No problems with this patch.

dmitrystu commented 3 years ago

Closed with MR#36