HNS-2024-07-riot.txt

--[ HNS-2024-07 - HN Security Advisory - https://hnsecurity.it/

* Title: Multiple vulnerabilities in RIOT OS
* OS: RIOT <= 2024.01
* Author: Marco Ivaldi <marco.ivaldi@hnsecurity.it>
* 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 <security@riot-os.org>.
2024-03-05: Asked again for feedback on <security@riot-os.org>.
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://hnsecurity.it/blog/ost2-zephyr-rtos-and-a-bunch-of-cves/
[4] https://hnsecurity.it/blog/multiple-vulnerabilities-in-rt-thread-rtos/
[5] https://hnsecurity.it/blog/big-update-to-my-semgrep-c-cpp-ruleset/
[6] https://hnsecurity.it/blog/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.