gcoap: Avoid reading beyond defined input buffer

[chrysn]

Contribution description

GCoAP contains a memcopy that reads beyond the bounds of the input.

This is likely not a security issue, but it does trip up ASAN (even in cases when there are otherwise no ill-effects), and it will let an error go unnoticed when trying to send too large a buffer and only copying out a portion of it.

Previously, the full retransmit buffer size was read from the source buffer and stored in the retransmit buffer. (The original length was still stored, so no other read would happen on the extraneously copied data).

I don't consider this a security issue because while there would reads to unwritten memory on retransmissions, any application that actually has its buffers and request content set up in such a way that this happens would already suffer visible breakage due to "garbage" (secrets, in the unlikely case that they are stored right behind the buffer) being retransmitted.

A second commit avoids leaking memos and the lock in two error cases (the new one and the one where I took justification for a plain return from).

Testing procedure

• (if you like to see a result fast, run aiocoap's aiocoap-rd and substitute your local address below -- but the ASAN error happens as well without the running server)
• run cord_lc example with

$ make all-asan
$ ./examples/cord_lc/bin/native/cord_lc.elf tap0
> cord_lc [fe80::f30:40e4:6c93:e17d]:5683 resource

Previously, this would have caused an ASAN abort; now, it merely reports whatever is in the RD (probably -4 for "nothing", or a timeout if you don't have aiocoap-rd running).

Issues/PRs references

• TBD: Why did this not get noticed earlier? Our native tests should be run with asan builds ... or it should even be on-by-default.

[riot-ci]

Murdock results

✔️ PASSED

2f7cbd3 gcoap: Avoid lockup from error paths

Success	Failures	Total	Runtime
10082	0	10083	14m:43s

Artifacts

• Documentation preview

[Teufelchen1]

Looks good to me. Could you help me to asses whether this has an high impact? Can this still be part of the upcoming release or should it be postponed?

[chrysn]

I think the impact is low either way. There's the chance that I did miss some security implication, in which case it'd be good to have it in. I don't expect any breakage, but same there, could be wrong. In total, I think it's a toss-up.

[fabian18]

Just a reminder. I think this is only stalling because the fixup commit must be squashed.

[chrysn]

Thanks for the ping, squashed. As there are unrelated errors popping up, I'm rebasing onto master.

[fabian18]

Although it is probably within the bounds of some static buffer where buf points to, I just saw a couple of lines below for the NON case technically we might also read beyond input bytes if no token or a token less that 8 bytes is used:

        case COAP_TYPE_NON:
            memo->send_limit = GCOAP_SEND_LIMIT_NON;
            memcpy(&memo->msg.hdr_buf[0], buf, GCOAP_HEADER_MAXLEN);
            timeout = CONFIG_GCOAP_NON_TIMEOUT_MSEC;
            break;

a MIN(len, GCOAP_HEADER_MAXLEN) would probably fix it.

[benpicco]

Any reason not to press the merge button on this?