Repository navigation
Fix stack-buffer overflow in CLI reply buffers - #3807
Merged
liamcottle merged 1 commit intoNov 11, 2026
Merged
Conversation
The remote-CLI reply buffers in onPeerDataRecv() (repeater, room server, sensor) and BaseChatMesh were sized temp[166] for a 5-byte datagram header plus 161 bytes of reply. But MyMesh::handleCommand() first reflects a 3-byte correlation prefix (e.g. 'aa|') into the reply, after which CommonCLI commands may write up to 160 chars + NUL into the remaining 158 bytes -- overflowing the buffer by 2 bytes (e.g. a full 'regions' listing via exportTo(reply, 160)). Triggering this remotely crashes the node (observed as an unresponsive repeater after a prefixed 'regions' query); the 2 out-of-bounds bytes are a printable region-name character and a NUL. Size the buffers for the full worst case instead: 5 header + 3 prefix + 160 reply + 1 NUL = 169. Verified with a host ASan harness reproducing the exact layout: prefixed 'regions' with a 25-region listing overflowed temp[166] (BufStream::write, RegionMap.cpp) and now passes clean with temp[169]; unprefixed (serial) path unaffected.
Valentin-Metz
added a commit
to Valentin-Metz/MeshCore
that referenced
this pull request
Nov 15, 2026
Follow-up to meshcore-dev#3807: auditing the full CLI dispatch chain for the same class of overflow turned up more sites, all reachable with the same long-input technique (remote datagrams decrypt to commands of up to 171 chars; the serial/ethernet buffers accept up to 159): - CommonRadioPrefs::handleCommand 'set radio' copied the command tail into a 132-byte stack tmp[] with strcpy() before any parameter validation -- ~30 bytes past the end (remote admin CLI, no prefix needed). - CommonCLI::handleCommand 'tempradio' / 'sensor set' and handleSetCmd 'extra.sf' copied into the 132-byte member tmp[] with strcpy(), corrupting adjacent members of the node object. - CommonCLI::handleGetCmd's unknown-config fallback did sprintf(reply, "??: %s", config) -- a remote 'get <167 chars>' writes 8 bytes past the temp[169] sized in meshcore-dev#3807. - The serial/ethernet reply buffers in the repeater/room-server/sensor examples were char[160] but feed the same prefix-reflecting handleCommand() fixed in meshcore-dev#3807: a prefixed 'aa|regions' writes 3 + 160 = 163 bytes plus NUL. Now 164 with the same sizing comment. - simple_secure_chat 'public <msg>' sprintf'ed node_name + user text into a 160-byte region and only truncated after the fact -- a long serial line overflows by ~350 bytes before the truncation runs. Replaced with a bounded snprintf. - DynamicConfigSerializer::getByKey() terminated with value[max_len] = 0 -- one past the end of a max_len-sized buffer for any caller passing sizeof(buf) (current callers pass sizeof(buf)-1, so latent). Bounds the copies with StrHelper::strncpy()/snprintf() following the existing repo idiom, and sizes the example reply buffers for the full worst case. Verified with a host ASan harness compiling the real CommonCLI, CommonRadioPrefs and RegionMap sources with the exact reply-buffer layouts of onPeerDataRecv() and the serial main loops: 'set radio' + 171 junk chars, 'tempradio'/'sensor set' + max-length input and 'get' + 167 junk chars each overflowed before this change and pass clean after it; the secure_chat sprintf-then-truncate shape likewise. pio test -e native: 50/50 pass (incl. config serializer getByKey).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The remote-CLI reply buffers in onPeerDataRecv() (repeater, room server, sensor) and BaseChatMesh were sized temp[166] for a 5-byte datagram header plus 161 bytes of reply. But MyMesh::handleCommand() first reflects a 3-byte correlation prefix (e.g. 'aa|') into the reply, after which CommonCLI commands may write up to 160 chars + NUL into the remaining 158 bytes -- overflowing the buffer by 2 bytes (e.g. a full 'regions' listing via exportTo(reply, 160)).
Triggering this remotely crashes the node (observed as an unresponsive repeater after a prefixed 'regions' query); the 2 out-of-bounds bytes are a printable region-name character and a NUL.
Size the buffers for the full worst case instead:
5 header + 3 prefix + 160 reply + 1 NUL = 169.
Verified with a host ASan harness reproducing the exact layout: prefixed 'regions' with a 25-region listing overflowed temp[166] (BufStream::write, RegionMap.cpp) and now passes clean with temp[169]; unprefixed (serial) path unaffected.