From 835571c0a755108b147ff7b5c6000a2b47e1cea8 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 17 Oct 2024 09:37:01 +0200 Subject: [PATCH] sys/net/nanocoap: fix UB when building hdr Some calls to `coap_build_hdr()` were done with the target buffer for the header and the source buffer for the token overlapping: They reuse the buffer that held the request to assemble the response in. We cannot use `memcpy()` in this case to copy the token into the target buffer, as source and destination would (fully) overlap. This commit makes reusing the request buffer for the response a special case: `memcpy()` is only used to copy the token if source and destination address of the token differ. An alternative fix would have been to use `memmove()` unconditionally. But `memmove()` does not make any assumption about the layout of target and source buffer, while we know that the token either will already be at the right position (when reusing the request buffer for the response) or be in a non-overlapping buffer (when generating a fresh token). This approach is more efficient than `memmove()`. --- sys/include/net/nanocoap.h | 5 +++++ sys/net/application_layer/nanocoap/nanocoap.c | 12 ++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h index d99857e99d..c00b318b1c 100644 --- a/sys/include/net/nanocoap.h +++ b/sys/include/net/nanocoap.h @@ -1946,6 +1946,11 @@ ssize_t coap_block2_build_reply(coap_pkt_t *pkt, unsigned code, * @param[in] code CoAP code (e.g., COAP_CODE_204, ...) * @param[in] id CoAP request id * + * @pre @p token is either not overlapping with the memory buffer + * @p hdr points to, or is already at the right offset (e.g. + * when building the response inside the buffer the contained + * the request). + * * @returns length of resulting header */ ssize_t coap_build_hdr(coap_hdr_t *hdr, unsigned type, const void *token, diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c index c4c57c6304..06d70a918a 100644 --- a/sys/net/application_layer/nanocoap/nanocoap.c +++ b/sys/net/application_layer/nanocoap/nanocoap.c @@ -724,8 +724,16 @@ ssize_t coap_build_hdr(coap_hdr_t *hdr, unsigned type, const void *token, memcpy(hdr + 1, &tkl_ext, tkl_ext_len); } - if (token_len) { - memcpy(coap_hdr_data_ptr(hdr), token, token_len); + /* Some users build a response packet in the same buffer that contained + * the request. In this case, the argument token already points inside + * the target, or more specifically, it is already at the correct place. + * Having `src` and `dest` in `memcpy(dest, src, len)` overlap is + * undefined behavior, so have to treat this explicitly. We could use + * memmove(), but we know that either `src` and `dest` do not overlap + * at all, or fully. So we can be a bit more efficient here. */ + void *token_dest = coap_hdr_data_ptr(hdr); + if (token_dest != token) { + memcpy(token_dest, token, token_len); } return sizeof(coap_hdr_t) + token_len + tkl_ext_len;