Comment 6 for bug 494549

Revision history for this message
Hector Martin (marcan42) wrote :

The first memcpy() is already guarded by a check right before it.
The second memcpy is potentially exploitable, I fixed that.
The third one isn't, because the length is always < USB_MRU which is < the buffer size (and the buffer is empty in this case), though I added a paranoia check anyways in case the USB implementation changes in the future.
The fourth memcpy is also guarded by a check right before it, not to mention that it's for data going _to_ the client, not coming from it, and we always generate that data ourselves.
The data from the client is not memcpy()ed, it's just recv()ed straight into the buffer, and the amount is also checked against the buffer size.

So it looks like I only had one exploitable bug, not four.

It's also worth noting that integer overflow bugs are not possible, neither on the device input nor on the client input, because packet lengths are bounded by the USB MRU on the device side (and we check header lengths against it) and we check header length (no math, and this is unsigned) against the client buffer size. I changed the buffer/packet sizes to unsigned for good hygiene anyway.

I also added underflow checks to device_data_input, to make sure packets are large enough for the required headers. This isn't exploitable either (buffers are always large enough, all it could do is result in garbage headers), but again, it's good hygiene.

The new version is in Git, let me know if it's acceptable and I'll make a release.