Multiple out of bound read Vulnerabilities In tinyusb

[hac425xxx]

These bugs are caused by the failure to check request->wLength.

hidd_control_xfer_cb

case 1

if request->wLength > the size of p_hid->epin_buf it could lead buffer out of bound read.

      case HID_REQ_CONTROL_GET_REPORT:
        if ( stage == CONTROL_STAGE_SETUP )
        {
          uint8_t const report_type = tu_u16_high(request->wValue);
          uint8_t const report_id   = tu_u16_low(request->wValue);

          uint16_t xferlen = tud_hid_get_report_cb(hid_itf, report_id, (hid_report_type_t) report_type, p_hid->epin_buf, request->wLength);
          TU_ASSERT( xferlen > 0 );

          tud_control_xfer(rhport, request, p_hid->epin_buf, xferlen);
        }

case 2

        else if ( stage == CONTROL_STAGE_ACK )
        {
          uint8_t const report_type = tu_u16_high(request->wValue);
          uint8_t const report_id   = tu_u16_low(request->wValue);
          
          //  bug !!!
          tud_hid_set_report_cb(hid_itf, report_id, (hid_report_type_t) report_type, p_hid->epout_buf, request->wLength);
        }

dfu_req_dnload_setup

static void dfu_req_dnload_setup(uint8_t rhport, tusb_control_request_t const * request)
{
 
  // setup for data phase
  tud_control_xfer(rhport, request, _dfu_state_ctx.transfer_buf, request->wLength);  // bug !!!
}

btd_control_xfer_cb

  if ( stage == CONTROL_STAGE_SETUP )
  {
  
    .....................
    .....................

    // bug!!!
    return tud_control_xfer(rhport, request, &_btd_itf.hci_cmd, request->wLength);
  }

[hathach]

have you tried it, and do you have any examples to prove it ?

[hac425xxx]

no, I found these by reading the code

[hathach]

Ah thanks, I thought you would have a running examples so that we could quickly test it out and fix it. Regardless, these are valid issue. Ultimately we shouldn't just host for its length.

• The 1) can be fixed by limited the requestlength to the endpoint size.
• For 2 DFU, it is newly added, I have an plan to enhance it later on, will keep this in mind.
• For 3) same as 1.

Thanks for your issue, if you have time please submit PR if possible. Otherwise, It is great enough, I will try to fix this whenever I could.

[hac425xxx]

out of bound in proc_read10_cmd

p_cbw and p_csw is receive from other usb device

static void proc_read10_cmd(uint8_t rhport, mscd_interface_t* p_msc)
{
  msc_cbw_t const * p_cbw = &p_msc->cbw;
  msc_csw_t       * p_csw = &p_msc->csw;

  uint16_t const block_cnt = rdwr10_get_blockcount(p_cbw->command);
  uint16_t const block_sz = p_cbw->total_bytes / block_cnt;

  // Adjust lba with transferred bytes
  uint32_t const lba = rdwr10_get_lba(p_cbw->command) + (p_msc->xferred_len / block_sz);

  // remaining bytes capped at class buffer
  int32_t nbytes = (int32_t) tu_min32(sizeof(_mscd_buf), p_cbw->total_bytes-p_msc->xferred_len);

  // Application can consume smaller bytes
  nbytes = tud_msc_read10_cb(p_cbw->lun, lba, p_msc->xferred_len % block_sz, _mscd_buf, (uint32_t) nbytes);

p_cbw->lun and lba don't be checked.

tud_msc_read10_cb also don't check lun and lba.

// Callback invoked when received READ10 command.
// Copy disk's data to buffer (up to bufsize) and return number of copied bytes.
int32_t tud_msc_read10_cb(uint8_t lun, uint32_t lba, uint32_t offset, void* buffer, uint32_t bufsize)
{

  // vuln !!!
  uint8_t const* addr = (lun ? msc_disk1[lba] : msc_disk0[lba]) + offset;
  memcpy(buffer, addr, bufsize);

  return bufsize;
}

if lba is large , it could lead out of bound !

Int overflow in handle_incoming_packet

static void handle_incoming_packet(uint32_t len)
{
  
    rndis_data_packet_t *r = (rndis_data_packet_t *) ((void*) pnt);
    if (len >= sizeof(rndis_data_packet_t))
      if ( (r->MessageType == REMOTE_NDIS_PACKET_MSG) && (r->MessageLength <= len))
      
      	// int overflow !!!
        if ( (r->DataOffset + offsetof(rndis_data_packet_t, DataOffset) + r->DataLength) <= len)
        {
          pnt = &received[r->DataOffset + offsetof(rndis_data_packet_t, DataOffset)];
          size = r->DataLength;
        }
  }

  if (!tud_network_recv_cb(pnt, size))
  {
    /* if a buffer was never handled by user code, we must renew on the user's behalf */
    tud_network_recv_renew();
  }

the type of r->DataLength and r->DataOffset are uint32_t .

typedef uint32_t rndis_DataOffset_t;
typedef uint32_t rndis_DataLength_t;
typedef uint32_t rndis_OOBDataOffset_t;
typedef uint32_t rndis_OOBDataLength_t;
typedef uint32_t rndis_NumOOBDataElements_t;
typedef uint32_t rndis_PerPacketInfoOffset_t;
typedef uint32_t rndis_PerPacketInfoLength_t;

typedef struct{
	rndis_MessageType_t			MessageType;
	rndis_MessageLength_t		MessageLength;
	rndis_DataOffset_t			DataOffset;
	rndis_DataLength_t			DataLength;
	rndis_OOBDataOffset_t		OOBDataOffset;
	rndis_OOBDataLength_t		OOBDataLength;
	rndis_NumOOBDataElements_t	NumOOBDataElements;
	rndis_PerPacketInfoOffset_t	PerPacketInfoOffset;
	rndis_PerPacketInfoLength_t PerPacketInfoLength;
	rndis_DeviceVcHandle_t		DeviceVcHandle;
	rndis_Reserved_t			Reserved;
	}rndis_data_packet_t;

when r->DataLength=0xFFFFFFFF and r->DataOffset is a small value, it could int overflow , then bypass the check.

if ( (r->DataOffset + offsetof(rndis_data_packet_t, DataOffset) + r->DataLength) <= len)

Then size could be 0xFFFFFFFF, it could lead out of bound read in tud_network_recv_cb.

[hac425xxx]

Add 2 new bug

[cr1901]

Do you plan to submit a PR for these?

[hac425xxx]

sorry, I have no time for this

[cr1901]

Then I would appreciate it if you knock it off with the unhelpful/smug // bug!!! and // vuln!!! annotations in the source snippets as you find them.

[xmos-jmccarthy]

I reviewed the dfu_req_dnload_setup and do not see this as a bug, provided that the CFG_TUD_DFU_TRANSFER_BUFFER_SIZE is in the descriptor. For this case to happen, the host would have to non-compliant. I will add a check here so that if the host were to send a request too long the issue would be more easily traceable.

On review, I did discover that I do have a bug elsewhere:

static uint16_t dfu_req_upload(uint8_t rhport, tusb_control_request_t const * request, uint16_t block_num, uint16_t wLength)
{
  TU_VERIFY( wLength <= CFG_TUD_DFU_TRANSFER_BUFFER_SIZE);
  uint16_t retval = tud_dfu_req_upload_data_cb(block_num, (uint8_t *)_dfu_state_ctx.transfer_buf, wLength);
  tud_control_xfer(rhport, request, _dfu_state_ctx.transfer_buf, retval);
  return retval;
}

I verify that the host requested length is valid, but not that the user callback length is valid, leaving an opening for an out of bounds access. I will submit a PR to fix this issue when I get the chance.

[hac425xxx]

The attack model is that there is a malicious USB device that will send malformed USB messages to our protocol stack， so we need to check the data recv from another USB device.

[hathach]

Yeah I am well aware of this issue. This will basically allow host purposely write/read memory in other section by overflowing current one. This can effective change the behavior by changing RAM contents. An famous example is Nintendo Switch https://youtu.be/67swnr1NCP4?t=195