Conclusion
In order to exploit this bug one needs the key to sign firmware updates. But if an adversary would have the key needed to sign arbitrary firmware updates, the adversary could just use the firmware update mechanism instead.
Hence: It is a valid bug, but of no value to an adversary.
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/sys/suit/handlers_command_seq.c#L427-L428
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 _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.
Please refer to the marked lines below:
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,
¶m_uri);
int err = nanocbor_get_tstr(¶m_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';
...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
Conclusion
In order to exploit this bug one needs the key to sign firmware updates. But if an adversary would have the key needed to sign arbitrary firmware updates, the adversary could just use the firmware update mechanism instead.
Hence: It is a valid bug, but of no value to an adversary.
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/sys/suit/handlers_command_seq.c#L427-L428
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
_dtv_fetch()function below,url_lenis checked in an assertion and subsequently used in a call tomemcpy(). If an attacker is able to provide a largeurlwhile assertions are compiled-out, they can write past the end of themanifest->urlbufbuffer.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.