From 289f4dc8931f0bb12936e378d2a116a400b7d19e Mon Sep 17 00:00:00 2001 From: Valentin Metz Date: Sun, 15 Nov 2026 06:59:37 +0100 Subject: [PATCH] Fix remaining buffer overflows in the CLI command paths Follow-up to #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 #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 #3807: a prefixed 'aa|regions' writes 3 + 160 = 163 bytes plus NUL. Now 164 with the same sizing comment. - simple_secure_chat 'public ' 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). --- examples/simple_repeater/main.cpp | 6 ++++-- examples/simple_room_server/main.cpp | 6 ++++-- examples/simple_secure_chat/main.cpp | 3 +-- examples/simple_sensor/main.cpp | 3 ++- src/helpers/CommonCLI.cpp | 8 ++++---- src/helpers/CommonRadioPrefs.cpp | 2 +- src/helpers/DynamicConfigSerializer.cpp | 4 ++-- 7 files changed, 18 insertions(+), 14 deletions(-) diff --git a/examples/simple_repeater/main.cpp b/examples/simple_repeater/main.cpp index d475b13d842..d16aa6347a4 100644 --- a/examples/simple_repeater/main.cpp +++ b/examples/simple_repeater/main.cpp @@ -149,7 +149,8 @@ void loop() { if (len > 0 && command[len - 1] == '\r') { // received complete line Serial.print('\n'); command[len - 1] = 0; // replace newline with C string null terminator - char reply[160]; + // 3-byte reflected CLI prefix + 160-byte reply text + NUL + char reply[164]; reply[0] = 0; #ifdef ETHERNET_ENABLED if (!ethernet_handle_command(command, reply)) { @@ -170,7 +171,8 @@ void loop() { #ifdef ETHERNET_ENABLED ethernet_loop_maintain(); if (ethernet_read_line(ethernet_command, sizeof(ethernet_command))) { - char reply[160]; + // 3-byte reflected CLI prefix + 160-byte reply text + NUL + char reply[164]; reply[0] = 0; if (!ethernet_handle_command(ethernet_command, reply)) { the_mesh.handleCommand(0, ethernet_command, reply); diff --git a/examples/simple_room_server/main.cpp b/examples/simple_room_server/main.cpp index 4bfad4e54d7..935eb0b279c 100644 --- a/examples/simple_room_server/main.cpp +++ b/examples/simple_room_server/main.cpp @@ -127,7 +127,8 @@ void loop() { if (len > 0 && command[len - 1] == '\r') { // received complete line command[len - 1] = 0; // replace newline with C string null terminator - char reply[160]; + // 3-byte reflected CLI prefix + 160-byte reply text + NUL + char reply[164]; reply[0] = 0; #ifdef ETHERNET_ENABLED if (!ethernet_handle_command(command, reply)) { @@ -146,7 +147,8 @@ void loop() { #ifdef ETHERNET_ENABLED ethernet_loop_maintain(); if (ethernet_read_line(ethernet_command, sizeof(ethernet_command))) { - char reply[160]; + // 3-byte reflected CLI prefix + 160-byte reply text + NUL + char reply[164]; reply[0] = 0; if (!ethernet_handle_command(ethernet_command, reply)) { the_mesh.handleCommand(0, ethernet_command, reply); diff --git a/examples/simple_secure_chat/main.cpp b/examples/simple_secure_chat/main.cpp index 2e6ba7126d3..2c2d4b310a4 100644 --- a/examples/simple_secure_chat/main.cpp +++ b/examples/simple_secure_chat/main.cpp @@ -409,8 +409,7 @@ class MyMesh : public BaseChatMesh, ContactVisitor { memcpy(temp, ×tamp, 4); // mostly an extra blob to help make packet_hash unique temp[4] = 0; // attempt and flags - sprintf((char *) &temp[5], "%s: %s", _prefs.node_name, &command[7]); // : - temp[5 + MAX_TEXT_LEN] = 0; // truncate if too long + snprintf((char *) &temp[5], MAX_TEXT_LEN + 1, "%s: %s", _prefs.node_name, &command[7]); // : int len = strlen((char *) &temp[5]); auto pkt = createGroupDatagram(PAYLOAD_TYPE_GRP_TXT, _public->channel, temp, 5 + len); diff --git a/examples/simple_sensor/main.cpp b/examples/simple_sensor/main.cpp index 66991daada5..c63241f6c73 100644 --- a/examples/simple_sensor/main.cpp +++ b/examples/simple_sensor/main.cpp @@ -144,7 +144,8 @@ void loop() { if (len > 0 && command[len - 1] == '\r') { // received complete line command[len - 1] = 0; // replace newline with C string null terminator - char reply[160]; + // 3-byte reflected CLI prefix + 160-byte reply text + NUL + char reply[164]; the_mesh.handleCommand(NULL, 0, command, reply); // NOTE: there is no sender_timestamp via serial! if (reply[0]) { Serial.print(" -> "); Serial.println(reply); diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 64fed99b34d..a314e44e538 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -252,7 +252,7 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re strcpy(reply, "ERR: bad pubkey"); } } else if (memcmp(command, "tempradio ", 10) == 0) { - strcpy(tmp, &command[10]); + StrHelper::strncpy(tmp, &command[10], sizeof(tmp)); const char *parts[5]; int num = mesh::Utils::parseTextParts(tmp, parts, 5); float freq = num > 0 ? strtof(parts[0], nullptr) : 0.0f; @@ -308,7 +308,7 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re strcpy(reply, "null"); } } else if (memcmp(command, "sensor set ", 11) == 0) { - strcpy(tmp, &command[11]); + StrHelper::strncpy(tmp, &command[11], sizeof(tmp)); const char *parts[2]; int num = mesh::Utils::parseTextParts(tmp, parts, 2, ' '); const char *key = (num > 0) ? parts[0] : ""; @@ -647,7 +647,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep }; #if defined(USE_LR2021) } else if (memcmp(config, "extra.sf ", 9) == 0) { - strcpy(tmp, &config[9]); + StrHelper::strncpy(tmp, &config[9], sizeof(tmp)); const char *parts[4]; uint8_t sideDetSFs[4]; int num = mesh::Utils::parseTextParts(tmp, parts, 4); @@ -819,7 +819,7 @@ void CommonCLI::handleGetCmd(uint32_t sender_timestamp, char* command, char* rep sprintf(reply, "No extra SF configured"); } } else { - sprintf(reply, "??: %s", config); + snprintf(reply, 160, "??: %s", config); } } diff --git a/src/helpers/CommonRadioPrefs.cpp b/src/helpers/CommonRadioPrefs.cpp index 44e0eba3161..ad194baade3 100644 --- a/src/helpers/CommonRadioPrefs.cpp +++ b/src/helpers/CommonRadioPrefs.cpp @@ -54,7 +54,7 @@ bool CommonRadioPrefs::handleCommand(const char* command, uint32_t sender_timest } if (memcmp(command, "set radio ", 10) == 0) { char tmp[132]; - strcpy(tmp, &command[10]); + StrHelper::strncpy(tmp, &command[10], sizeof(tmp)); const char *parts[4]; int num = mesh::Utils::parseTextParts(tmp, parts, 4); float freq = num > 0 ? strtof(parts[0], nullptr) : 0.0f; diff --git a/src/helpers/DynamicConfigSerializer.cpp b/src/helpers/DynamicConfigSerializer.cpp index b9c6c267efa..12e549b7aae 100644 --- a/src/helpers/DynamicConfigSerializer.cpp +++ b/src/helpers/DynamicConfigSerializer.cpp @@ -67,8 +67,8 @@ bool DynamicConfigSerializer::getByKey(const char* key, char* value, size_t max_ for (int i = 0; i < n; i++) { const char* item = parts[i]; if (item[keylen] == KEY_SEP_CHAR && memcmp(item, key, keylen) == 0) { - strncpy(value, &item[keylen+1], max_len); - value[max_len] = 0; + strncpy(value, &item[keylen+1], max_len - 1); // max_len inclusive of null + value[max_len - 1] = 0; return true; } }