tinycbor buffer overflow causing mcumgr image upload failure

[jimparis]

Hi,
Reporting this here because zephyrproject-rtos/tinycbor doesn't have an issue tracker.

There is a buffer overflow in zephyr's tinycbor, in cborparser.c:

CborError _cbor_value_copy_string(const CborValue *value, void *buffer,
                                 size_t *buflen, CborValue *next)
{
    //...
    CborError err = iterate_string_chunks(value, (char*)buffer, buflen, &copied_all, next,
                                          buffer ? (IterateFunction) value->parser->d->cpy : iterate_noop);
    //...
    if (buffer) {
        *((uint8_t *)buffer + *buflen) = '\0';
    }

If iterate_string_chunks fills the buffer completely, the byte past the buffer is zeroed. This is in contrast to the function comment, which says "If the buffer is large enough, this function will insert a null byte after the last copied byte".

The bug is not present in the upstream intel/tinycbor, where they add termination inside iterate_string_chunks instead.

The bug was introduced by zephyrproject-rtos/tinycbor#1, which appears intended to bring the code more in-line with the upstream mynewt tinycbor, but their code doesn't have the bug either.

---

The specific symptom I'm seeing is:

$ mcumgr --conntype serial --connstring dev=/dev/foo image upload foo.bin
Error: 3

and it's happening because data_sha overflows into img_mgmt_data in img_mgmt_upload, during a cbor_read_object call:

static int
img_mgmt_upload(struct mgmt_ctxt *ctxt)
{
    uint8_t img_mgmt_data[IMG_MGMT_UL_CHUNK_SIZE];
    uint8_t data_sha[IMG_MGMT_DATA_SHA_LEN];

which clobbers the IMAGE_MAGIC checked by img_mgmt_check_header.

---

This same tinycbor bug was the root cause of an earlier mcumgr fix in #7924, which ended up in a patch to upstream's mcumgr apache/mynewt-mcumgr#5. There, it was noted by @ccollins476ad that it was actually a zephyr tinycbor bug, but the workaround was merged to mynewt's mcumgr anyway (apache/mynewt-mcumgr@c2da8ca), and then also into zephyr's fork (zephyrproject-rtos/mcumgr@d9b889e).

I think both of those commits should be reverted in their repos once this is fixed.

[dleach02]

@nvlsianpu, can you provide some guidance on priority level for this problem?

[nvlsianpu]

@jimparis zephyrproject-rtos/tinycbor#7 Is this full fix for the issue?

[jimparis]

As far as I'm aware yes, for tinycbor. Then an update to zephyr's west.yml here to point to the new version. Then the workarounds in mcumgr could be removed, but they are harmless.

[de-nordic]

@carlescufi is already updating west.yml manifest here: #19838