RIOT 2024.01 Buffer Overflows / Lack Of Size Checks / Out-Of-Bound Access

`--[ HNS-2024-07 - HN Security Advisory - https://security.humanativaspa.it/  
  
* Title: Multiple vulnerabilities in RIOT OS  
* OS: RIOT <= 2024.01  
* Author: Marco Ivaldi <[email protected]>  
* Date: 2024-05-07  
* CVE ID and severity:  
* CVE-2024-31225 - High  
* CVE-2024-32017 - Critical  
* CVE-2024-32018 - High  
(low-severity vulnerabilities were not assigned a CVE ID)  
* Vendor URL: https://www.riot-os.org/  
* Advisory URLs:   
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-2572-7q7c-3965  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-v97j-w9m6-c4h3  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-899m-q6pp-hmp3  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-x3j5-hfrr-5x6q  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-pw2r-pp35-xfmj  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-c4p4-vv7v-3hx8  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-r87w-9vw9-f7cx  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-2hx7-c324-3rxv  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-frp5-4gfp-84j3  
* https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-x27v-gqp4-7jq3  
  
  
--[ 0 - Table of contents  
  
1 - Summary  
2 - Background  
3 - Vulnerabilities  
3.1 - CVE-2024-31225 - Lack of size check and buffer overflow in RIOT /sys/net/application_layer/cord/lc/cord_lc.c  
3.2 - CVE-2024-32017 - Buffer overflows in RIOT /sys/net/application_layer/gcoap/  
3.3 - CVE-2024-32018 - Ineffective size check due to assert() and buffer overflow in RIOT /pkg/nimble/scanlist/nimble_scanlist.c  
3.4 - Unsafe use of the return value of vsnprintf() and out-of-bounds memory access in RIOT /cpu/esp_common/lib_printf.c  
3.5 - Ineffective size check due to assert() and buffer overflow in RIOT /sys/net/ble/skald/skald_eddystone.c  
3.6 - Ineffective size check due to assert() and buffer overflow in RIOT /sys/suit/handlers_command_seq.c  
3.7 - Integer wraparound and buffer overflow in RIOT /drivers/mtd_emulated/mtd_emulated.c  
3.8 - Off-by-one buffer overflow and unterminated string in RIOT /pkg/lwext4/fs/lwext4_fs.c  
3.9 - Unsafe use of the return value of snprintf() and out-of-bounds memory access in RIOT /sys/shell/cmds/vfs.c  
3.10 - Lack of size checks and buffer overflows in RIOT /sys/net/application_layer/emcute/emcute.c  
4 - Affected products  
5 - Remediation  
6 - Disclosure timeline  
7 - Acknowledgments  
8 - References  
  
  
--[ 1 - Summary  
  
"Where there is parsing, there are bugs."   
-- Dr. Silvio Cesare  
  
RIOT [1] is a free, open-source, real-time operating system developed by a  
grassroots community gathering companies, academia, and hobbyists, distributed  
all around the world. It supports most low-power IoT devices, microcontroller  
architectures (32-bit, 16-bit, 8-bit), and external devices. RIOT aims to  
implement all relevant open standards supporting an Internet of Things that is  
connected, secure, durable, and privacy friendly.  
  
We reviewed RIOT's source code hosted on GitHub [2] and identified multiple  
security vulnerabilities that may cause memory corruption. Their impacts range  
from denial of service to potential arbitrary code execution.  
  
  
--[ 2 - Background  
  
Continuing our recent vulnerability research work in the IoT space [3] [4], we  
keep assisting open-source projects in finding and fixing vulnerabilities by  
reviewing their source code, with the final goal to make IoT more secure. In  
January 2024, RIOT was selected as a target of interest.  
  
During the source code review, we put our Semgrep C/C++ ruleset [5] and weggli  
pattern collection [6] to good use to identify hotspots in code on which to  
focus our attention.  
  
  
--[ 3 - Vulnerabilities  
  
The vulnerabilities resulting from our source code review are briefly described  
in the following sections.  
  
  
--[ 3.1 - CVE-2024-31225 - Lack of size check and buffer overflow in RIOT /sys/net/application_layer/cord/lc/cord_lc.c  
  
We spotted the lack of a size check that may lead to a buffer overflow in RIOT  
source code at:  
  
* /sys/net/application_layer/cord/lc/cord_lc.c  
  
The `_on_rd_init()` function does not implement a size check before copying  
data to the `_result_buf` static buffer. If an attacker can craft a long enough  
payload, they could cause a buffer overflow.  
  
See the marked line below:  
```c  
static void _on_rd_init(const gcoap_request_memo_t *memo, coap_pkt_t *pdu,  
const sock_udp_ep_t *remote)  
{  
(void)remote;  
  
thread_flags_t flag = FLAG_NORSC;  
  
if (memo->state == GCOAP_MEMO_RESP) {  
unsigned ct = coap_get_content_type(pdu);  
if (ct != COAP_FORMAT_LINK) {  
DEBUG("cord_lc: error payload not in link format: %u\n", ct);  
goto end;  
}  
if (pdu->payload_len == 0) {  
DEBUG("cord_lc: error empty payload\n");  
goto end;  
}  
memcpy(_result_buf, pdu->payload, pdu->payload_len); // VULN: lack of size check and potential buffer overflow  
_result_buf_len = pdu->payload_len;  
_result_buf[_result_buf_len] = '\0';  
flag = FLAG_SUCCESS;  
} else if (memo->state == GCOAP_MEMO_TIMEOUT) {  
flag = FLAG_TIMEOUT;  
}  
  
end:  
if (flag != FLAG_SUCCESS) {  
_result_buf = NULL;  
_result_buf_len = 0;  
}  
thread_flags_set(_waiter, flag);  
}  
```  
  
Note that the `_on_lookup()` function in the same file does implement such an  
explicit size check:  
```c  
static void _on_lookup(const gcoap_request_memo_t *memo, coap_pkt_t *pdu,  
const sock_udp_ep_t *remote)  
{  
(void)remote;  
  
thread_flags_t flag = FLAG_ERR;  
  
if (memo->state == GCOAP_MEMO_RESP) {  
unsigned ct = coap_get_content_type(pdu);  
if (ct != COAP_FORMAT_LINK) {  
DEBUG("cord_lc: unsupported content format: %u\n", ct);  
thread_flags_set(_waiter, flag);  
}  
if (pdu->payload_len == 0) {  
flag = FLAG_NORSC;  
thread_flags_set(_waiter, flag);  
}  
if (pdu->payload_len >= _result_buf_len) { // CHECK  
flag = FLAG_OVERFLOW;  
thread_flags_set(_waiter, flag);  
}  
memcpy(_result_buf, pdu->payload, pdu->payload_len);  
memset(_result_buf + pdu->payload_len, 0,  
_result_buf_len - pdu->payload_len);  
_result_buf_len = pdu->payload_len;  
flag = FLAG_SUCCESS;  
} else if (memo->state == GCOAP_MEMO_TIMEOUT) {  
flag = FLAG_TIMEOUT;  
}  
  
thread_flags_set(_waiter, flag);  
}  
```  
  
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.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20547  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-2572-7q7c-3965  
  
  
--[ 3.2 - CVE-2024-32017 - Buffer overflows in RIOT /sys/net/application_layer/gcoap/  
  
We spotted a typo in a size check that may lead to a buffer overflow in RIOT  
source code at:  
  
* /sys/net/application_layer/gcoap/dns.c  
  
We also spotted another potential buffer overflow in RIOT source code at:  
  
* /sys/net/application_layer/gcoap/forward_proxy.c  
  
The size check in the `gcoap_dns_server_proxy_get()` function contains a small  
typo that may lead to a buffer overflow in the subsequent `strcpy()`. The  
length of the `_uri` string is checked instead of the length of the `_proxy`  
string.  
  
See the marked lines below:  
```c  
ssize_t gcoap_dns_server_proxy_get(char *proxy, size_t proxy_len)  
{  
ssize_t res = 0;  
mutex_lock(&_client_mutex);  
if (_dns_server_uri_isset()) {  
res = strlen(_uri); // VULN: typo, should be strlen(_proxy)  
if (((size_t)res + 1) > proxy_len) {  
/* account for trailing \0 */  
res = -ENOBUFS;  
}  
else {  
strcpy(proxy, _proxy); // VULN: potential buffer overflow  
}  
}  
mutex_unlock(&_client_mutex);  
return res;  
}  
```  
  
The `_gcoap_forward_proxy_copy_options()` function does not implement an  
explicit size check before copying data to the `cep->req_etag` buffer that is  
`COAP_ETAG_LENGTH_MAX` bytes long. If an attacker can craft input so that  
optlen becomes larger than `COAP_ETAG_LENGTH_MAX`, they can cause a buffer  
overflow.  
  
See the marked line below:  
```c  
static int _gcoap_forward_proxy_copy_options(coap_pkt_t *pkt,  
coap_pkt_t *client_pkt,  
client_ep_t *cep,  
uri_parser_result_t *urip)  
{  
/* copy all options from client_pkt to pkt */  
coap_optpos_t opt = {0, 0};  
uint8_t *value;  
bool uri_path_added = false;  
bool etag_added = false;  
  
for (int i = 0; i < client_pkt->options_len; i++) {  
ssize_t optlen = coap_opt_get_next(client_pkt, &opt, &value, !i);  
/* wrt to ETag option slack: we always have at least the Proxy-URI option in the client_pkt,  
* so we should hit at least once (and it's opt_num is also >= COAP_OPT_ETAG) */  
if (optlen >= 0) {  
if (IS_USED(MODULE_NANOCOAP_CACHE) && !etag_added && (opt.opt_num >= COAP_OPT_ETAG)) {  
static const uint8_t tmp[COAP_ETAG_LENGTH_MAX] = { 0 };  
/* add slack to maybe add an ETag on stale cache hit later, as is done in  
* gcoap_req_send() (which we circumvented in _gcoap_forward_proxy_via_coap()) */  
if (coap_opt_add_opaque(pkt, COAP_OPT_ETAG, tmp, sizeof(tmp))) {  
etag_added = true;  
}  
}  
if (IS_USED(MODULE_NANOCOAP_CACHE) && opt.opt_num == COAP_OPT_ETAG) {  
if (_cep_get_req_etag_len(cep) == 0) {  
/* TODO: what to do on multiple ETags? */  
_cep_set_req_etag_len(cep, (uint8_t)optlen);  
#if IS_USED(MODULE_NANOCOAP_CACHE)  
/* req_tag in cep is pre-processor guarded so we need to as well */  
memcpy(cep->req_etag, value, optlen); // VULN: potential buffer overflow if optlen can become larger than COAP_ETAG_LENGTH_MAX  
#endif  
...  
```  
  
If the input above is attacker-controlled and crosses a security boundary, the  
impact of the buffer overflow vulnerabilities could range from denial of  
service to arbitrary code execution.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20561  
https://github.com/RIOT-OS/RIOT/pull/20579  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-v97j-w9m6-c4h3  
  
  
--[ 3.3 - CVE-2024-32018 - Ineffective size check due to assert() and buffer overflow in RIOT /pkg/nimble/scanlist/nimble_scanlist.c  
  
We spotted an ineffective size check implemented via `assert()` that may lead  
to a buffer overflow in RIOT source code at:  
  
* /pkg/nimble/scanlist/nimble_scanlist.c  
  
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.  
  
See the marked lines below:  
```c  
/**  
* @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;  
}  
```  
  
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.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20555  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-899m-q6pp-hmp3  
  
  
--[ 3.4 - Unsafe use of the return value of vsnprintf() and out-of-bounds memory access in RIOT /cpu/esp_common/lib_printf.c  
  
We spotted an unsafe use of the return value of `vsnprintf()` that leads to an  
out-of-bounds memory access in RIOT source code at:  
  
* /cpu/esp_common/lib_printf.c  
  
The `vsnprintf()` API function returns the number of characters (excluding the  
terminating NUL byte) which would have been written to the destination string  
if enough space had been available.   
  
If an attacker is able to craft input so that the final string would become  
larger than `PRINTF_BUFSIZ`, they can exploit this bug to overwrite a '\n',  
'\r' or ' ' byte (or a sequence of such bytes) with a NUL byte in memory past  
the end of the `_printf_buf` static buffer. In addition, the tainted `len`  
value is returned at the end of the `_lib_printf()` function and may be used  
unsafely (e.g., as an array index) elsewhere in the code.  
  
See the marked lines below:  
```c  
static char _printf_buf[PRINTF_BUFSIZ];  
  
static int _lib_printf(int level, const char* tag, const char* format, va_list arg)  
{  
if (level > LOG_LEVEL) {  
return 0;  
}  
  
int len = vsnprintf(_printf_buf, PRINTF_BUFSIZ - 1, format, arg); // VULN: vsnprintf() returns the total length of the string it tried to create, which may be larger than the actual size of the destination string  
  
/*  
* Since ESP_EARLY_LOG macros add a line break at the end, a terminating  
* line break in the output must be removed if there is one.  
*/  
_printf_buf[PRINTF_BUFSIZ - 1] = 0;  
int i;  
for (i = len - 1; i >= 0; --i) {  
if (_printf_buf[i] != '\n' && _printf_buf[i] != '\r' && _printf_buf[i] != ' ') {  
break;  
}  
_printf_buf[i] = 0; // VULN: very limited oob array write access  
}  
if (i > 0) {  
ESP_EARLY_LOGI(tag, "%s", _printf_buf);  
}  
va_end(arg);  
return len; // VULN: tainted len value is returned  
}  
```  
  
In our understanding, the impact of this vulnerability in this specific case is  
quite limited. However, all bugs that have the potential to cause memory  
corruption should be taken seriously and fixed as security bugs.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20596  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-x3j5-hfrr-5x6q  
  
  
--[ 3.5 - Ineffective size check due to assert() and buffer overflow in RIOT /sys/net/ble/skald/skald_eddystone.c  
  
We spotted an ineffective size check implemented via `assert()` that may lead  
to a buffer overflow in RIOT source code at:  
  
* /sys/net/ble/skald/skald_eddystone.c  
  
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 `skald_eddystone_url_adv()` function below, `len` is checked  
in an assertion and subsequently used in a call to `memcpy()`. If an attacker  
is able to provide a large `url` while assertions are compiled-out, they can  
write past the end of the `pdu->url` buffer.  
  
See the marked lines below:  
```c  
void skald_eddystone_url_adv(skald_ctx_t *ctx,  
uint8_t scheme, const char *url, uint8_t tx_pwr,  
uint32_t adv_itvl_ms)  
{  
assert(url && ctx);  
size_t len = strlen(url);  
assert(len <= (NETDEV_BLE_PDU_MAXLEN - (URL_HDR_LEN + PREAMBLE_LEN))); // VULN: len is checked only via an assertion  
  
eddy_url_t *pdu = (eddy_url_t *)ctx->pkt.pdu;  
_init_pre(&pdu->pre, EDDYSTONE_URL, (URL_HDR_LEN + len));  
  
/* set remaining service data fields */  
pdu->tx_pwr = tx_pwr;  
pdu->scheme = scheme;  
memcpy(pdu->url, url, len); // VULN: if len is actually larger than expected there's a potential buffer overflow  
  
/* start advertising */  
ctx->pkt.len = (sizeof(pre_t) + 2 + len);  
ctx->adv_itvl_ms = adv_itvl_ms;  
skald_adv_start(ctx);  
}  
```  
  
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.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20577  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-pw2r-pp35-xfmj  
  
  
--[ 3.6 - Ineffective size check due to assert() and buffer overflow in RIOT /sys/suit/handlers_command_seq.c  
  
We spotted an ineffective size check implemented via `assert()` that may lead  
to a buffer overflow in RIOT source code at:  
  
* /sys/suit/handlers_command_seq.c  
  
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 `_dtv_fetch()` function below, `url_len` is checked in an  
assertion and subsequently used in a call to `memcpy()`. If an attacker is able  
to provide a large `url` while assertions are compiled-out, they can write past  
the end of the `manifest->urlbuf` buffer.  
  
See the marked lines below:  
```c  
static int _dtv_fetch(suit_manifest_t *manifest, int key,  
nanocbor_value_t *_it)  
{  
(void)key; (void)_it;  
LOG_DEBUG("_dtv_fetch() key=%i\n", key);  
  
const uint8_t *url;  
size_t url_len;  
  
/* Check the policy before fetching anything */  
int res = suit_policy_check(manifest);  
if (res) {  
return SUIT_ERR_POLICY_FORBIDDEN;  
}  
  
suit_component_t *comp = _get_component(manifest);  
  
/* Deny the fetch if the component was already fetched before */  
if (suit_component_check_flag(comp, SUIT_COMPONENT_STATE_FETCHED)) {  
LOG_ERROR("Component already fetched before\n");  
return SUIT_ERR_INVALID_MANIFEST;  
}  
  
nanocbor_value_t param_uri;  
suit_param_ref_to_cbor(manifest, &comp->param_uri,  
&param_uri);  
int err = nanocbor_get_tstr(&param_uri, &url, &url_len);  
if (err < 0) {  
LOG_DEBUG("URL parsing failed\n)");  
return err;  
}  
  
assert(manifest->urlbuf && url_len < manifest->urlbuf_len); // VULN: url_len is checked only via an assertion  
memcpy(manifest->urlbuf, url, url_len); // VULN: if url_len is actually larger than expected there's a potential buffer overflow  
manifest->urlbuf[url_len] = '\0';  
...  
```  
  
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.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20559  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-c4p4-vv7v-3hx8  
  
  
--[ 3.7 - Integer wraparound and buffer overflow in RIOT /drivers/mtd_emulated/mtd_emulated.c  
  
We spotted an integer wraparound in a size check that leads to a buffer  
overflow in RIOT source code at:  
  
* /drivers/mtd_emulated/mtd_emulated.c  
  
If an attacker is able to provide arbitrary values for the `num` and `sector`  
arguments to the `_erase_sector()` function, they can cause an integer  
wraparound to bypass the size check and cause a buffer overflow.  
  
See the marked lines below:  
```c  
static int _erase_sector(mtd_dev_t *dev, uint32_t sector, uint32_t num)  
{  
mtd_emulated_t *mtd = (mtd_emulated_t *)dev;  
  
(void)mtd;  
assert(mtd);  
  
if (/* sector must not exceed the number of sectors */  
(sector >= mtd->base.sector_count) ||  
/* sector + num must not exceed the number of sectors */  
((sector + num) > mtd->base.sector_count)) { // VULN: integer wraparound in size check  
return -EOVERFLOW;  
}  
  
memset(mtd->memory + (sector * (mtd->base.pages_per_sector * mtd->base.page_size)),  
0xff, num * (mtd->base.pages_per_sector * mtd->base.page_size)); // VULN: buffer overflow  
  
return 0;  
}  
```  
  
We put together a quick proof-of-concept to demonstrate this issue:  
```  
raptor@blumenkraft Research % cat wraparound5.c  
#include <stdio.h>  
#include <stdlib.h>  
#include <inttypes.h>  
  
#define SECTOR_COUNT 256  
  
static int _erase_sector(uint32_t sector, uint32_t num)  
{  
if (/* sector must not exceed the number of sectors */  
(sector >= SECTOR_COUNT) ||  
/* sector + num must not exceed the number of sectors */  
((sector + num) > SECTOR_COUNT)) { // VULN: wraparound  
printf("OVERFLOW\n");  
return 1;  
}  
  
printf("sector + num = %"PRIu32"\n", sector + num);  
  
return 0;  
}  
  
int main(int argc, char **argv)  
{  
if (argc < 3)  
return 1;  
  
return _erase_sector(atoi(argv[1]), atoi(argv[2]));  
}  
raptor@blumenkraft Research % make wraparound5  
cc wraparound5.c -o wraparound5  
raptor@blumenkraft Research % ./wraparound5 250 10  
OVERFLOW  
raptor@blumenkraft Research % ./wraparound5 250 4294967295  
sector + num = 249  
```  
  
If the input above is attacker-controlled and crosses a security boundary, the  
impact of the buffer overflow vulnerability could range from denial of service  
to (less likely in this case) arbitrary code execution.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20587  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-r87w-9vw9-f7cx  
  
  
--[ 3.8 - Off-by-one buffer overflow and unterminated string in RIOT /pkg/lwext4/fs/lwext4_fs.c  
  
We spotted an off-by-one buffer overflow in RIOT source code at:  
  
* /pkg/lwext4/fs/lwext4_fs.c  
  
We also spotted the lack of explicit NUL-termination after strncpy() in the  
same file.  
  
If an attacker is able to make `mount_point` at least `CONFIG_EXT4_MAX_MP_NAME`  
bytes large, `strcat()` would write a NUL byte past the end of the `mp.name`  
buffer, thus potentially overwriting the next member of the `ext4_mountpoint`  
struct [7].  
  
See the marked line below:  
```c  
static int prepare(lwext4_desc_t *fs, const char *mount_point)  
{  
mtd_dev_t *dev = fs->dev;  
struct ext4_blockdev_iface *iface = &fs->iface;  
  
memset(&fs->mp, 0, sizeof(fs->mp));  
memset(&fs->bdev, 0, sizeof(fs->bdev));  
memset(&fs->iface, 0, sizeof(fs->iface));  
  
strncpy(fs->mp.name, mount_point, CONFIG_EXT4_MAX_MP_NAME);  
strcat(fs->mp.name, "/"); // VULN: off-by-one buffer overflow  
  
int res = mtd_init(dev);  
if (res) {  
return res;  
}  
...  
```  
  
Since `sizeof(dirent->name)` is 255 and `sizeof(entry->d_name)` is only 32, if  
an attacker can control the input to `strncpy()` they would be able to create a  
non NUL-terminated string in `entry->d_name`. When such corrupted string is  
used in other parts of the code, it may cause information leakage or memory  
corruption.  
  
See the marked line below:  
```c  
static int _readdir(vfs_DIR *dirp, vfs_dirent_t *entry)  
{  
ext4_dir *dir = _get_ext4_dir(dirp);  
  
const ext4_direntry *dirent = ext4_dir_entry_next(dir);  
if (dirent == NULL) {  
return 0;  
}  
  
strncpy(entry->d_name, (char *)dirent->name, sizeof(entry->d_name)); // VULN  
  
return 1;  
}  
```  
  
In our understanding, the impact of these vulnerabilities in this specific case  
is quite limited. However, all bugs that have the potential to cause memory  
corruption should be taken seriously and fixed as security bugs.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20586  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-2hx7-c324-3rxv  
  
  
--[ 3.9 - Unsafe use of the return value of snprintf() and out-of-bounds memory access in RIOT /sys/shell/cmds/vfs.c  
  
We spotted an unsafe use of the return value of `snprintf()` that leads to an  
out-of-bounds memory access in RIOT source code at:  
  
* /sys/shell/cmds/vfs.c  
  
The `snprintf()` function returns the number of characters (excluding the  
terminating NUL byte) which would have been written to the destination string  
if enough space had been available.   
  
If an attacker is able to craft input so that the final string would become  
larger than `bs`, they can exploit this bug to cause a buffer overflow.  
  
See the marked lines below:  
```c  
static void _write_block(int fd, unsigned bs, unsigned i)  
{  
char block[bs];  
char *buf = block;  
  
buf += snprintf(buf, bs, "|%03u|", i); // VULN: snprintf() returns the total length of the string it tried to create, which may be larger than bs  
  
memset(buf, _get_char(i), &block[bs] - buf); // VULN: buf can point past the block buffer, in addition &block[bs] - buf would be negative and become a large unsigned value  
block[bs - 1] = '\n';  
  
vfs_write(fd, block, bs);  
}  
```  
  
We put together a quick proof-of-concept to demonstrate this issue:  
```  
raptor@blumenkraft Research % cat ret1.c  
#include <stdio.h>  
#include <string.h>  
#include <stdlib.h>  
  
static char _get_char(unsigned i)  
{  
i %= 62; /* a-z, A-Z, 0..9, -> 62 characters */  
  
if (i < 10) {  
return '0' + i;  
}  
i -= 10;  
  
if (i <= 'z' - 'a') {  
return 'a' + i;  
}  
i -= 1 + 'z' - 'a';  
  
return 'A' + i;  
}  
  
static void _write_block(unsigned bs, unsigned i)  
{  
char block[bs];  
char *buf = block;  
  
buf += snprintf(buf, bs, "|%03u|", i); // VULN: unsafe use of return value  
  
memset(buf, _get_char(i), &block[bs] - buf); // VULN: oob memory write, size becomes a large unsigned value  
block[bs - 1] = '\n';  
}  
  
int main(int argc, char **argv)  
{  
if (argc < 3)  
return 1;  
  
_write_block(atoi(argv[1]), atoi(argv[2]));  
  
return 0;  
}  
raptor@blumenkraft Research % make ret1  
cc ret1.c -o ret1  
raptor@blumenkraft Research % ./ret1 5 10  
raptor@blumenkraft Research % ./ret1 5 1000000  
zsh: segmentation fault ./ret1 5 1000000  
```  
  
If the input above is attacker-controlled and crosses a security boundary, the  
impact of the buffer overflow vulnerability could range from denial of service  
to (less likely in this case) arbitrary code execution.  
  
Fixes:  
https://github.com/RIOT-OS/RIOT/pull/20546  
https://github.com/RIOT-OS/RIOT/pull/20595  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-frp5-4gfp-84j3  
  
  
--[ 3.10 - Lack of size checks and buffer overflows in RIOT /sys/net/application_layer/emcute/emcute.c  
  
We spotted the lack of size checks that may lead to buffer overflows in RIOT  
source code at:  
  
* /sys/net/application_layer/emcute/emcute.c  
  
The `emcute_con()` function does not implement a size check before copying data  
to the `tbuf` static buffer. If an attacker can craft a long enough payload,  
they could cause a buffer overflow.  
  
See the marked line below:  
```c  
int emcute_con(sock_udp_ep_t *remote, bool clean, const char *will_topic,  
const void *will_msg, size_t will_msg_len, unsigned will_flags)  
{  
int res;  
size_t len;  
  
assert(!will_topic || (will_topic && will_msg && !(will_flags & ~PUB_FLAGS)));  
  
mutex_lock(&txlock);  
  
/* check for existing connections and copy given UDP endpoint */  
if (gateway.port != 0) {  
return EMCUTE_NOGW;  
}  
memcpy(&gateway, remote, sizeof(sock_udp_ep_t));  
  
/* figure out which flags to set */  
uint8_t flags = (clean) ? EMCUTE_CS : 0;  
if (will_topic) {  
flags |= EMCUTE_WILL;  
}  
  
/* compute packet size */  
len = (strlen(cli_id) + 6);  
tbuf[0] = (uint8_t)len;  
tbuf[1] = CONNECT;  
tbuf[2] = flags;  
tbuf[3] = PROTOCOL_VERSION;  
byteorder_htobebufs(&tbuf[4], CONFIG_EMCUTE_KEEPALIVE);  
memcpy(&tbuf[6], cli_id, strlen(cli_id)); // VULN: lack of size check and potential buffer overflow  
...  
```  
  
The `emcute_unsub()` function does not implement a size check before copying  
data to the `tbuf` static buffer. If an attacker can craft a long enough  
payload, they could cause a buffer overflow.  
  
See the marked line below:  
```c  
int emcute_unsub(emcute_sub_t *sub)  
{  
assert(sub && sub->topic.name);  
  
if (gateway.port == 0) {  
return EMCUTE_NOGW;  
}  
  
mutex_lock(&txlock);  
  
tbuf[0] = (strlen(sub->topic.name) + 5);  
tbuf[1] = UNSUBSCRIBE;  
tbuf[2] = 0;  
byteorder_htobebufs(&tbuf[3], id_next);  
waitonid = id_next++;  
memcpy(&tbuf[5], sub->topic.name, strlen(sub->topic.name)); // VULN: lack of size check and potential buffer overflow  
  
int res = syncsend(UNSUBACK, (size_t)tbuf[0], false);  
if (res == EMCUTE_OK) {  
if (subs == sub) {  
subs = sub->next;  
}  
else {  
emcute_sub_t *s;  
for (s = subs; s; s = s->next) {  
if (s->next == sub) {  
s->next = sub->next;  
break;  
}  
}  
}  
}  
  
mutex_unlock(&txlock);  
return res;  
}  
```  
  
If the unchecked input above is attacker-controlled and crosses a security  
boundary, the impact of the buffer overflow vulnerabilities could range from  
denial of service to arbitrary code execution.  
  
There is currently no fix for these issues. RIOT maintainers do not consider  
them to be security critical bugs and therefore they are currently discussing  
them publicly.  
  
See also:  
https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-x27v-gqp4-7jq3  
  
  
--[ 4 - Affected products  
  
RIOT 2024.01 and (likely) earlier versions are affected by the vulnerabilities  
discussed in this advisory.  
  
  
--[ 5 - Remediation  
  
RIOT maintainers have fixed all the vulnerabilities discussed in this advisory,  
except for those reported in GHSA-x27v-gqp4-7jq3, which are not considered  
security critical bugs and are currently being discussed publicly.  
  
Please check the official RIOT channels for further information about fixes.  
  
  
--[ 6 - Disclosure timeline  
  
We reported the vulnerabilities discussed in this advisory to RIOT maintainers  
in January 2024, via the handy private reporting feature [8] that is available  
on GitHub.  
  
We are not sure of the reason of the significant delay in the initial  
maintainers' response. However, once we got their attention they quickly  
triaged and fixed all vulnerabilities. They also informed us that:  
  
* They are treating such delay as an additional security incident.  
* They added another maintainer to the security group as an immediate action.  
* They plan on discussing this shortcoming on the next maintainer assembly to  
find a long term solution.  
  
The coordinated disclosure timeline follows:  
  
2024-01-10: Reported the first vulnerability to the RIOT project.  
2024-01-11: Reported four more vulnerabilities.  
2024-01-12: Reported the rest of the vulnerabilities.  
2024-02-09: Asked for feedback on <[email protected]>.  
2024-03-05: Asked again for feedback on <[email protected]>.  
2024-04-05: Asked again for feedback via GitHub and got the first reply.  
2024-04-06: Started collaborating with RIOT to evaluate proposed fixes.  
2024-04-10: First security advisory published on GitHub.  
2024-04-17: Another security advisory published on GitHub.  
2024-04-24: Asked for a status update on the remaining reports.  
2024-04-25: Two more security advisories published on GitHub.  
2024-04-26: Another security advisory published on GitHub.  
2024-04-30: Three more security advisories published on GitHub.  
2024-05-07: Published advisory and writeup.  
  
  
--[ 7 - Acknowledgments  
  
We would like to thank RIOT maintainers for triaging and fixing the reported  
vulnerabilities in a particularly friendly and professional way. We really  
appreciated working with them!  
  
  
--[ 8 - References  
  
[1] https://www.riot-os.org/  
[2] https://github.com/RIOT-OS/RIOT  
[3] https://security.humanativaspa.it/ost2-zephyr-rtos-and-a-bunch-of-cves/  
[4] https://security.humanativaspa.it/multiple-vulnerabilities-in-rt-thread-rtos/  
[5] https://security.humanativaspa.it/big-update-to-my-semgrep-c-cpp-ruleset/  
[6] https://security.humanativaspa.it/a-collection-of-weggli-patterns-for-c-cpp-vulnerability-research/  
[7] https://github.com/gkostka/lwext4/blob/58bcf89a121b72d4fb66334f1693d3b30e4cb9c5/src/ext4.c#L75  
[8] https://docs.github.com/en/code-security/security-advisories  
  
  
Copyright (c) 2024 Marco Ivaldi and Humanativa Group. All rights reserved.  
`