bound ndp16 wLength against received ntb in recv_validate_datagram#3741
bound ndp16 wLength against received ntb in recv_validate_datagram#3741dxbjavid wants to merge 2 commits into
Conversation
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| metro_m4_express/net_lwip_webserver | 41,504 → 41,520 (+16) | — | — | — | 41,708 → 41,724 (+16) | +0.0% |
| samg55_xplained/net_lwip_webserver | 41,648 → 41,664 (+16) | — | — | — | 41,848 → 41,864 (+16) | +0.0% |
| ea4357/net_lwip_webserver | 44,656 → 44,672 (+16) | — | — | — | 45,350 → 45,366 (+16) | +0.0% |
| stm32c542nucleo/net_lwip_webserver | 41,712 → 41,720 (+8) | — | — | — | 48,640 → 48,648 (+8) | +0.0% |
| stm32l412nucleo/net_lwip_webserver | 43,752 → 43,760 (+8) | — | — | — | 49,088 → 49,096 (+8) | +0.0% |
| ch582m_evt/net_lwip_webserver | 45,912 → 45,920 (+8) | — | — | — | 49,824 → 49,832 (+8) | +0.0% |
| frdm_rw612/net_lwip_webserver | 48,968 → 48,976 (+8) | — | — | — | 51,144 → 51,152 (+8) | +0.0% |
| stlinkv3mini/net_lwip_webserver | 46,096 → 46,104 (+8) | — | — | — | 53,436 → 53,444 (+8) | +0.0% |
| metro_m7_1011/net_lwip_webserver | 47,084 → 47,092 (+8) | — | — | — | 54,968 → 54,976 (+8) | +0.0% |
| ch32v307v_r1_1v0/net_lwip_webserver | 56,332 → 56,340 (+8) | — | — | — | 57,028 → 57,036 (+8) | +0.0% |
Hardware-in-the-loop (HIL) Test Reporthfp.json✅ 52 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
tinyusb.json✅ 200 passed · ❌ 138 failed · ⚪ 11 skipped · blank not run
|
|
for anyone picking this up, here is how i triggered it. i built a small standalone harness that mirrors the recv_validate_datagram arithmetic over a heap-allocated recv_ntb_t (data[3200]) and compiled it under -fsanitize=address. the crafted NTB is a short 64-byte transfer. a valid nth16 (NTH16 signature, wHeaderLength = sizeof(nth16_t), wBlockLength = 64, wNdpIndex = 12), then an ndp16 at data+12 carrying the NCM0 signature, wNextNdpIndex = 0 and wLength = 0xfff0. the checks above only bound wLength from below, so max_ndx works out to (0xfff0 - 8) / 4 = 16380 and the ndp16_datagram[max_ndx - 1] access lands roughly 65 kB into the 3200-byte buffer. before the patch ASan flags a heap-buffer-overflow read of size 2 at that access. with the added wNdpIndex + wLength > len check the NTB is rejected cleanly (12 + 65520 > 64) and there is no out-of-bounds read. happy to fold this into the net fuzz harness as a seed if that is the preferred home for the regression. |
Thank you it would be nice. |
folds the reproducer for the recv_validate_datagram bound into a small self-contained fuzz target. it feeds a raw ntb straight into the validator (the driver is pulled in so the static function is reachable) and ships the crafted 64-byte ntb as a seed. the seed trips an asan heap-buffer-overflow against the unpatched driver and is rejected cleanly with the wLength bound in place.
|
done. added it under test/fuzz/device/net_ncm as a small self-contained harness. it feeds a raw ntb straight into recv_validate_datagram (the driver is pulled in so the static function is reachable, with the handful of usbd symbols stubbed out) and net_ncm_seed_corpus.zip carries the crafted 64-byte ntb. built against the unpatched driver the seed trips the asan heap-buffer-overflow in recv_validate_datagram; with the wLength bound in place it is rejected cleanly. there is a |
the ncm driver validates each incoming ntb in recv_validate_datagram before passing it to the glue logic. the first ndp's wLength is the size of the datagram pointer block and it comes straight from the host on the out endpoint, but the function only checks it against a minimum and never an upper bound. max_ndx is then derived from wLength and used to index ndp16_datagram[max_ndx - 1], and to walk the array, inside ntb->data which is only CFG_TUD_NCM_OUT_NTB_MAX_SIZE bytes (3200 by default). a short transfer carrying a wLength near 0xffff yields a max_ndx of roughly 16000, so the validator reads tens of kilobytes past the receive buffer while it is still deciding whether the packet is valid. i came across it while reading the bounds logic around wNdpIndex, which is capped against len whereas wLength is not. rejecting the ntb when wNdpIndex + wLength runs past the received length keeps the array within the buffer and lines up with the checks already done just above.