bound cdc-data endpoints against descriptor length in acm_open#3750
bound cdc-data endpoints against descriptor length in acm_open#3750dxbjavid wants to merge 5 commits into
Conversation
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| raspberrypi_cm4/cdc_msc | 57,180 → 57,372 (+192) | 680 → 4,584 (+3,904) | — | — | 57,860 → 61,956 (+4,096) | +7.1% |
| lpcxpresso1347/midi2_device | 16,780 → 17,212 (+432) | — | — | — | 17,276 → 17,708 (+432) | +2.5% |
| metro_m4_express/midi2_device | 17,216 → 17,644 (+428) | — | — | — | 17,344 → 17,772 (+428) | +2.5% |
| samg55_xplained/midi2_device | 17,408 → 17,836 (+428) | — | — | — | 17,532 → 17,960 (+428) | +2.4% |
| lpcxpresso1769/midi2_device | 17,400 → 17,832 (+432) | — | — | — | 17,896 → 18,328 (+432) | +2.4% |
| lpcxpresso1549/midi2_device | 17,384 → 17,816 (+432) | — | — | — | 17,960 → 18,392 (+432) | +2.4% |
| mm32f327x_mb39/midi2_device | 16,320 → 16,724 (+404) | 1,600 → 1,628 (+28) | — | — | 18,448 → 18,880 (+432) | +2.3% |
| ea4088_quickstart/midi2_device | 17,208 → 17,636 (+428) | — | — | — | 18,664 → 19,092 (+428) | +2.3% |
| msp_exp430f5529lp/midi2_device | 21,200 → 21,704 (+504) | 1,700 → 1,730 (+30) | — | — | 23,316 → 23,850 (+534) | +2.3% |
| metro_m0_express/midi2_device | 16,876 → 17,260 (+384) | — | — | — | 17,004 → 17,388 (+384) | +2.3% |
There was a problem hiding this comment.
Pull request overview
This PR hardens the CDC-ACM host enumeration path (acm_open) against malicious or malformed configuration descriptors, aiming to prevent out-of-bounds reads when walking device-supplied descriptors.
Changes:
- Adds a bounds check before reading/opening the CDC notification endpoint descriptor.
- Ensures the CDC-DATA interface descriptor is within bounds before inspecting it.
- Adds a pre-check that two data endpoint descriptors fit before calling
open_ep_stream_pair().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Open notification endpoint of control interface if any | ||
| if (itf_desc->bNumEndpoints == 1) { | ||
| TU_ASSERT(tu_desc_in_bounds(p_desc, desc_end), 0); |
There was a problem hiding this comment.
good catch. the in-bounds helper only checks p_desc + bLength against the end, so a short bLength at the tail still let tuh_edpt_open read the full 7-byte struct past desc_end. i've swapped that for an explicit check that the whole endpoint descriptor fits before it's touched.
| // data endpoints expected to be in pairs, make sure both fit before reading them | ||
| TU_ASSERT((uint16_t)(desc_end - p_desc) >= 2 * sizeof(tusb_desc_endpoint_t), 0); |
There was a problem hiding this comment.
agreed, the 14-byte guard doesn't stop the walk itself: open_ep_stream_pair reaches the second endpoint via tu_desc_next(desc_ep), so an inflated bLength on the first one could jump past desc_end. rather than re-check in the caller i've pinned bLength == sizeof(tusb_desc_endpoint_t) inside open_ep_stream_pair, which keeps tu_desc_next advancing exactly seven bytes and holds the second read inside the window the caller already validated.
| // whole endpoint descriptor must fit: tuh_edpt_open reads the full struct regardless of bLength | ||
| TU_ASSERT(p_desc + sizeof(tusb_desc_endpoint_t) <= desc_end, 0); | ||
| TU_ASSERT(TUSB_DESC_ENDPOINT == tu_desc_type(p_desc), 0); | ||
| const tusb_desc_endpoint_t *desc_ep = (const tusb_desc_endpoint_t *)p_desc; | ||
| TU_ASSERT(tuh_edpt_open(daddr, desc_ep), 0); |
There was a problem hiding this comment.
right, tu_desc_next there was still advancing by the device bLength, so an inflated value could push p_desc past desc_end before the next check even ran. since we've already asserted the whole 7-byte endpoint fits, i now step forward by sizeof(tusb_desc_endpoint_t) instead, which keeps the pointer inside the window we validated.
| if (p_desc + sizeof(tusb_desc_interface_t) <= desc_end && TUSB_DESC_INTERFACE == tu_desc_type(p_desc)) { | ||
| const tusb_desc_interface_t *data_itf = (const tusb_desc_interface_t *)p_desc; | ||
| if (data_itf->bInterfaceClass == TUSB_CLASS_CDC_DATA) { | ||
| p_desc = tu_desc_next(p_desc); // next to endpoint descriptor | ||
|
|
||
| // data endpoints expected to be in pairs | ||
| // data endpoints expected to be in pairs, make sure both fit before reading them | ||
| TU_ASSERT(p_desc + 2 * sizeof(tusb_desc_endpoint_t) <= desc_end, 0); | ||
| TU_ASSERT(open_ep_stream_pair(p_cdc, (const tusb_desc_endpoint_t *)p_desc), 0); | ||
| p_desc += data_itf->bNumEndpoints * sizeof(tusb_desc_endpoint_t); |
There was a problem hiding this comment.
agreed. open_ep_stream_pair always takes exactly two endpoints, so i've pinned bNumEndpoints == 2 and advanced by the fixed two-endpoint size rather than the reported count. i also switched the interface hop to sizeof(tusb_desc_interface_t) so the walk no longer trusts any device-supplied bLength on this path.
Hardware-in-the-loop (HIL) Test Reporthfp.json✅ 52 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
tinyusb.json✅ 313 passed · ❌ 26 failed · ⚪ 11 skipped · blank not run
|
| // Open notification endpoint of control interface if any | ||
| if (itf_desc->bNumEndpoints == 1) { | ||
| // whole endpoint descriptor must fit: tuh_edpt_open reads the full struct regardless of bLength | ||
| TU_ASSERT(p_desc + sizeof(tusb_desc_endpoint_t) <= desc_end, 0); | ||
| TU_ASSERT(TUSB_DESC_ENDPOINT == tu_desc_type(p_desc), 0); |
There was a problem hiding this comment.
right, the functional descriptor walk was still trusting the loop guard, so a short or zero-length CS_INTERFACE descriptor near desc_end could read the subtype past the end (or spin on bLength==0). i've tightened the loop to require the 3-byte header in bounds plus bLength >= 3 before it reads the type/subtype, and the bmCapabilities read now only happens when the whole cdc_desc_func_acm_t fits inside desc_end.
|
I think a point to debate is how much security we want, failure proof for ban configurator or fully hardened for rogue devices. My idea is to keep fully hardening into an optional config, what do you think @hathach ? |
|
I agree, TinyUSB size is becoming a concern. Not all users need security, we probably need an macro for different security level, the higher the more check/validate. |
|
makes sense, and the +52 bytes here is a fair reason to not force it on everyone. i'm happy to move these acm_open checks behind a config macro that defaults off, so it stays zero-cost for the usual trusted-hub setup and only gets pulled in when someone actually needs to survive a rogue device. on the shape, would you rather have one global level (something like a CFG_TUSB validation/security level) that the classes key off, or just a plain on/off flag per host class? if you want the graduated level i can wire this PR up as the first user of it so the convention is set here, otherwise i'll keep it to a single guard local to cdc_host. either way point me at the name you'd prefer and i'll refactor to match rather than guess. |
I think global level makes more sense named |
add a three-level TUSB_VALIDATION_NONE/BASIC/STRICT knob and default CFG_TUH_VALIDATION_LEVEL to BASIC, then make acm_open the first user so the enumeration bounds checks compile out at NONE for trusted-device setups and stay on by default.
|
wired it up as you described. added the three shared levels in tusb_option.h #define TUSB_VALIDATION_NONE 0 // trusted devices only, minimal code size
#define TUSB_VALIDATION_BASIC 1 // default, mal-configured devices, no OOB reads / zero-length loops
#define TUSB_VALIDATION_STRICT 2 // reject malformed/hostile descriptors, stricter class validationand defaulted CFG_TUH_VALIDATION_LEVEL to BASIC. acm_open is the first user: every bounds check in this PR now sits behind a small TU_DESC_VALIDATE() gate, so at NONE they collapse to a pass and the walk reverts to the old lean path for trusted-hub setups, while BASIC (the default) keeps the enumeration reads in bounds as before. checked it compiles clean at all three levels. a couple of things i left open on purpose. these checks are all BASIC-tier (no OOB, no zero-length spin), so STRICT is defined but not yet exercised here; i'd rather add strict-only checks when there's a concrete one to hang on it than invent one now. i also only added the TUH default, not CFG_TUD, since this PR is host-only and i didn't want to reach into the device side uninvited. happy to move the level constants elsewhere or split the gate macro into a common header if you'd rather it not live in cdc_host, just say. |
acm_open walks a device-supplied CDC-ACM configuration descriptor during host enumeration, but unlike the ftdi/cp210x/ch34x open paths it never checks that the notification endpoint, the data interface, or the two data endpoints actually lie within the descriptor before reading them. A malicious config whose wTotalLength equals CFG_TUH_ENUMERATION_BUFSIZE and which places the CDC-DATA interface right at the tail causes open_ep_stream_pair to read two 7-byte endpoint descriptors past the end of the enumeration buffer, so a rogue device can trigger an out-of-bounds read during enumeration. This bounds each read against desc_end with tu_desc_in_bounds and verifies the endpoint pair fits, which is the same guard the vendor serial drivers already apply.