Summary
I spotted an ineffective size check implemented via assert() that may lead to a buffer overflow in RIOT source code at:
https://github.com/RIOT-OS/RIOT/blob/master/pkg/nimble/scanlist/nimble_scanlist.c#L74-L87
Details
Most codebases define assertion macros which compile to a no-op on non-debug builds. If assertions are the only line of defense against untrusted input, the software may be exposed to attacks that leverage the lack of proper input checks. In detail, in the nimble_scanlist_update() function below, len is checked in an assertion and subsequently used in a call to memcpy(). If an attacker is able to provide a larger len value while assertions are compiled-out, they can write past the end of the fixed-length e->ad buffer.
Please refer to the marked lines below:
/**
* @brief Data structure for holding a single scanlist entry
*/
typedef struct {
clist_node_t node; /**< list node */
ble_addr_t addr; /**< a node's BLE address */
uint8_t ad[BLE_ADV_PDU_LEN]; /**< the received raw advertising data */
uint8_t ad_len; /**< length of the advertising data */
int8_t last_rssi; /**< last RSSI of a scanned node */
uint32_t adv_msg_cnt; /**< number of adv packets by a node */
uint32_t first_update; /**< first packet timestamp */
uint32_t last_update; /**< last packet timestamp */
uint8_t type; /**< advertising packet type */
uint8_t phy_pri; /**< primary PHY used */
uint8_t phy_sec; /**< secondary PHY advertised */
} nimble_scanlist_entry_t;
...
void nimble_scanlist_update(uint8_t type, const ble_addr_t *addr,
const nimble_scanner_info_t *info,
const uint8_t *ad, size_t len)
{
assert(addr);
assert(len <= BLE_ADV_PDU_LEN); // VULN: len is checked to be <= BLE_ADV_PDU_LEN only via an assertion
uint32_t now = (uint32_t)ztimer_now(ZTIMER_USEC);
nimble_scanlist_entry_t *e = _find(addr);
if (!e) {
e = (nimble_scanlist_entry_t *)clist_lpop(&_pool);
if (!e) {
/* no space available, dropping newly discovered node */
return;
}
memcpy(&e->addr, addr, sizeof(ble_addr_t));
if (ad) {
memcpy(e->ad, ad, len); // VULN: if len is actually larger than BLE_ADV_PDU_LEN there's a potential buffer overflow
}
e->ad_len = len;
e->last_rssi = info->rssi;
e->first_update = now;
e->adv_msg_cnt = 1;
e->type = type;
e->phy_pri = info->phy_pri;
e->phy_sec = info->phy_sec;
clist_rpush(&_list, (clist_node_t *)e);
}
else {
e->adv_msg_cnt++;
}
e->last_update = now;
}Impact
If the unchecked input above is attacker-controlled and crosses a security boundary, the impact of the buffer overflow vulnerability could range from denial of service to arbitrary code execution.
0xdea Reporter
Summary
I spotted an ineffective size check implemented via
assert()that may lead to a buffer overflow in RIOT source code at:https://github.com/RIOT-OS/RIOT/blob/master/pkg/nimble/scanlist/nimble_scanlist.c#L74-L87
Details
Most codebases define assertion macros which compile to a no-op on non-debug builds. If assertions are the only line of defense against untrusted input, the software may be exposed to attacks that leverage the lack of proper input checks. In detail, in the
nimble_scanlist_update()function below,lenis checked in an assertion and subsequently used in a call tomemcpy(). If an attacker is able to provide a largerlenvalue while assertions are compiled-out, they can write past the end of the fixed-lengthe->adbuffer.Please refer to the marked lines below:
Impact
If the unchecked input above is attacker-controlled and crosses a security boundary, the impact of the buffer overflow vulnerability could range from denial of service to arbitrary code execution.