Open hac425xxx opened 3 years ago
have you tried it, and do you have any examples to prove it ?
no, I found these by reading the code
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.
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.
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 !
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
.
Add 2 new bug
Do you plan to submit a PR for these?
sorry, I have no time for this
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.
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.
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.
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
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 2
dfu_req_dnload_setup
btd_control_xfer_cb