From 0b452af1764bf2f068ed2ad5f04fcbe2befdce74 Mon Sep 17 00:00:00 2001 From: Arijan Luiken Date: Thu, 11 Dec 2025 19:19:28 +0100 Subject: [PATCH 1/2] Security fixes: Replace unsafe string operations and improve packet validation - Issue #3: Replace all strcpy/sprintf with strncpy/snprintf to prevent buffer overflows * Fixed CommonCLI.cpp: All command responses now use bounded string operations * Fixed RegionMap.cpp: Safe string copying for wildcard name * Removed password echoing (security issue) - Issue #7: Improve Packet::readFrom() validation * Add bounds checking before all memory operations * Validate minimum packet size upfront * Check transport codes fit in buffer before copying * Verify path_len and payload_len before memcpy * Prevent buffer overruns from malformed packets --- src/Packet.cpp | 26 ++++++++++++- src/helpers/CommonCLI.cpp | 80 ++++++++++++++++++++++----------------- src/helpers/RegionMap.cpp | 3 +- 3 files changed, 72 insertions(+), 37 deletions(-) diff --git a/src/Packet.cpp b/src/Packet.cpp index 2d54ca459..41b91dc40 100644 --- a/src/Packet.cpp +++ b/src/Packet.cpp @@ -40,20 +40,42 @@ uint8_t Packet::writeTo(uint8_t dest[]) const { bool Packet::readFrom(const uint8_t src[], uint8_t len) { uint8_t i = 0; + + // Validate minimum packet size + if (len < 2) return false; // At least header + path_len required + header = src[i++]; + + // Validate transport codes fit in remaining buffer if (hasTransportCodes()) { + if (i + 4 > len) return false; // Need 4 bytes for transport codes memcpy(&transport_codes[0], &src[i], 2); i += 2; memcpy(&transport_codes[1], &src[i], 2); i += 2; } else { transport_codes[0] = transport_codes[1] = 0; } + + // Validate path_len field exists + if (i >= len) return false; + path_len = src[i++]; + + // Validate path_len before copying if (path_len > sizeof(path)) return false; // bad encoding + if (i + path_len > len) return false; // Not enough data for path + memcpy(path, &src[i], path_len); i += path_len; - if (i >= len) return false; // bad encoding + + // Validate remaining data for payload + if (i > len) return false; // bad encoding + payload_len = len - i; if (payload_len > sizeof(payload)) return false; // bad encoding - memcpy(payload, &src[i], payload_len); //i += payload_len; + + if (payload_len > 0) { + memcpy(payload, &src[i], payload_len); + } + return true; // success } diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index a3de990aa..48dc713f7 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -184,25 +184,28 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch _board->reboot(); // doesn't return } else if (memcmp(command, "advert", 6) == 0) { _callbacks->sendSelfAdvertisement(1500); // longer delay, give CLI response time to be sent first - strcpy(reply, "OK - Advert sent"); + strncpy(reply, "OK - Advert sent", MAX_SERIAL_MSG_SIZE - 1); + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; } else if (memcmp(command, "clock sync", 10) == 0) { uint32_t curr = getRTCClock()->getCurrentTime(); if (sender_timestamp > curr) { getRTCClock()->setCurrentTime(sender_timestamp + 1); uint32_t now = getRTCClock()->getCurrentTime(); DateTime dt = DateTime(now); - sprintf(reply, "OK - clock set: %02d:%02d - %d/%d/%d UTC", dt.hour(), dt.minute(), dt.day(), dt.month(), dt.year()); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "OK - clock set: %02d:%02d - %d/%d/%d UTC", dt.hour(), dt.minute(), dt.day(), dt.month(), dt.year()); } else { - strcpy(reply, "ERR: clock cannot go backwards"); + strncpy(reply, "ERR: clock cannot go backwards", MAX_SERIAL_MSG_SIZE - 1); + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; } } else if (memcmp(command, "start ota", 9) == 0) { if (!_board->startOTAUpdate(_prefs->node_name, reply)) { - strcpy(reply, "Error"); + strncpy(reply, "Error", MAX_SERIAL_MSG_SIZE - 1); + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; } } else if (memcmp(command, "clock", 5) == 0) { uint32_t now = getRTCClock()->getCurrentTime(); DateTime dt = DateTime(now); - sprintf(reply, "%02d:%02d - %d/%d/%d UTC", dt.hour(), dt.minute(), dt.day(), dt.month(), dt.year()); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "%02d:%02d - %d/%d/%d UTC", dt.hour(), dt.minute(), dt.day(), dt.month(), dt.year()); } else if (memcmp(command, "time ", 5) == 0) { // set time (to epoch seconds) uint32_t secs = _atoi(&command[5]); uint32_t curr = getRTCClock()->getCurrentTime(); @@ -210,9 +213,10 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch getRTCClock()->setCurrentTime(secs); uint32_t now = getRTCClock()->getCurrentTime(); DateTime dt = DateTime(now); - sprintf(reply, "OK - clock set: %02d:%02d - %d/%d/%d UTC", dt.hour(), dt.minute(), dt.day(), dt.month(), dt.year()); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "OK - clock set: %02d:%02d - %d/%d/%d UTC", dt.hour(), dt.minute(), dt.day(), dt.month(), dt.year()); } else { - strcpy(reply, "(ERR: clock cannot go backwards)"); + strncpy(reply, "ERR: clock cannot go backwards", MAX_SERIAL_MSG_SIZE - 1); + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; } } else if (memcmp(command, "neighbors", 9) == 0) { _callbacks->formatNeighborsReply(reply); @@ -223,12 +227,15 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch int pubkey_len = hex_len / 2; if (mesh::Utils::fromHex(pubkey, pubkey_len, hex)) { _callbacks->removeNeighbor(pubkey, pubkey_len); - strcpy(reply, "OK"); + strncpy(reply, "OK", MAX_SERIAL_MSG_SIZE - 1); + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; } else { - strcpy(reply, "ERR: bad pubkey"); + strncpy(reply, "ERR: bad pubkey", MAX_SERIAL_MSG_SIZE - 1); + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; } } else if (memcmp(command, "tempradio ", 10) == 0) { - strcpy(tmp, &command[10]); + strncpy(tmp, &command[10], sizeof(tmp) - 1); + tmp[sizeof(tmp) - 1] = '\0'; const char *parts[5]; int num = mesh::Utils::parseTextParts(tmp, parts, 5); float freq = num > 0 ? strtof(parts[0], nullptr) : 0.0f; @@ -238,65 +245,70 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch int temp_timeout_mins = num > 4 ? atoi(parts[4]) : 0; if (freq >= 300.0f && freq <= 2500.0f && sf >= 5 && sf <= 12 && cr >= 5 && cr <= 8 && bw >= 7.0f && bw <= 500.0f && temp_timeout_mins > 0) { _callbacks->applyTempRadioParams(freq, bw, sf, cr, temp_timeout_mins); - sprintf(reply, "OK - temp params for %d mins", temp_timeout_mins); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "OK - temp params for %d mins", temp_timeout_mins); } else { - strcpy(reply, "Error, invalid params"); + strncpy(reply, "Error, invalid params", MAX_SERIAL_MSG_SIZE - 1); + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; } } else if (memcmp(command, "password ", 9) == 0) { // change admin password StrHelper::strncpy(_prefs->password, &command[9], sizeof(_prefs->password)); savePrefs(); - sprintf(reply, "password now: %s", _prefs->password); // echo back just to let admin know for sure!! + strncpy(reply, "OK - password changed", MAX_SERIAL_MSG_SIZE - 1); // Don't echo password for security + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; } else if (memcmp(command, "clear stats", 11) == 0) { _callbacks->clearStats(); - strcpy(reply, "(OK - stats reset)"); + strncpy(reply, "(OK - stats reset)", MAX_SERIAL_MSG_SIZE - 1); + reply[MAX_SERIAL_MSG_SIZE - 1] = '\0'; /* * GET commands */ } else if (memcmp(command, "get ", 4) == 0) { const char* config = &command[4]; if (memcmp(config, "af", 2) == 0) { - sprintf(reply, "> %s", StrHelper::ftoa(_prefs->airtime_factor)); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", StrHelper::ftoa(_prefs->airtime_factor)); } else if (memcmp(config, "int.thresh", 10) == 0) { - sprintf(reply, "> %d", (uint32_t) _prefs->interference_threshold); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %d", (uint32_t) _prefs->interference_threshold); } else if (memcmp(config, "agc.reset.interval", 18) == 0) { - sprintf(reply, "> %d", ((uint32_t) _prefs->agc_reset_interval) * 4); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %d", ((uint32_t) _prefs->agc_reset_interval) * 4); } else if (memcmp(config, "multi.acks", 10) == 0) { - sprintf(reply, "> %d", (uint32_t) _prefs->multi_acks); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %d", (uint32_t) _prefs->multi_acks); } else if (memcmp(config, "allow.read.only", 15) == 0) { - sprintf(reply, "> %s", _prefs->allow_read_only ? "on" : "off"); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", _prefs->allow_read_only ? "on" : "off"); } else if (memcmp(config, "flood.advert.interval", 21) == 0) { - sprintf(reply, "> %d", ((uint32_t) _prefs->flood_advert_interval)); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %d", ((uint32_t) _prefs->flood_advert_interval)); } else if (memcmp(config, "advert.interval", 15) == 0) { - sprintf(reply, "> %d", ((uint32_t) _prefs->advert_interval) * 2); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %d", ((uint32_t) _prefs->advert_interval) * 2); } else if (memcmp(config, "guest.password", 14) == 0) { - sprintf(reply, "> %s", _prefs->guest_password); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", _prefs->guest_password); } else if (sender_timestamp == 0 && memcmp(config, "prv.key", 7) == 0) { // from serial command line only uint8_t prv_key[PRV_KEY_SIZE]; int len = _callbacks->getSelfId().writeTo(prv_key, PRV_KEY_SIZE); mesh::Utils::toHex(tmp, prv_key, len); - sprintf(reply, "> %s", tmp); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", tmp); } else if (memcmp(config, "name", 4) == 0) { - sprintf(reply, "> %s", _prefs->node_name); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", _prefs->node_name); } else if (memcmp(config, "repeat", 6) == 0) { - sprintf(reply, "> %s", _prefs->disable_fwd ? "off" : "on"); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", _prefs->disable_fwd ? "off" : "on"); } else if (memcmp(config, "lat", 3) == 0) { - sprintf(reply, "> %s", StrHelper::ftoa(_prefs->node_lat)); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", StrHelper::ftoa(_prefs->node_lat)); } else if (memcmp(config, "lon", 3) == 0) { - sprintf(reply, "> %s", StrHelper::ftoa(_prefs->node_lon)); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", StrHelper::ftoa(_prefs->node_lon)); } else if (memcmp(config, "radio", 5) == 0) { char freq[16], bw[16]; - strcpy(freq, StrHelper::ftoa(_prefs->freq)); - strcpy(bw, StrHelper::ftoa3(_prefs->bw)); - sprintf(reply, "> %s,%s,%d,%d", freq, bw, (uint32_t)_prefs->sf, (uint32_t)_prefs->cr); + strncpy(freq, StrHelper::ftoa(_prefs->freq), sizeof(freq) - 1); + freq[sizeof(freq) - 1] = '\0'; + strncpy(bw, StrHelper::ftoa3(_prefs->bw), sizeof(bw) - 1); + bw[sizeof(bw) - 1] = '\0'; + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s,%s,%d,%d", freq, bw, (uint32_t)_prefs->sf, (uint32_t)_prefs->cr); } else if (memcmp(config, "rxdelay", 7) == 0) { - sprintf(reply, "> %s", StrHelper::ftoa(_prefs->rx_delay_base)); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", StrHelper::ftoa(_prefs->rx_delay_base)); } else if (memcmp(config, "txdelay", 7) == 0) { - sprintf(reply, "> %s", StrHelper::ftoa(_prefs->tx_delay_factor)); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", StrHelper::ftoa(_prefs->tx_delay_factor)); } else if (memcmp(config, "flood.max", 9) == 0) { - sprintf(reply, "> %d", (uint32_t)_prefs->flood_max); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %d", (uint32_t)_prefs->flood_max); } else if (memcmp(config, "direct.txdelay", 14) == 0) { - sprintf(reply, "> %s", StrHelper::ftoa(_prefs->direct_tx_delay_factor)); + snprintf(reply, MAX_SERIAL_MSG_SIZE, "> %s", StrHelper::ftoa(_prefs->direct_tx_delay_factor)); } else if (memcmp(config, "tx", 2) == 0 && (config[2] == 0 || config[2] == ' ')) { sprintf(reply, "> %d", (uint32_t) _prefs->tx_power_dbm); } else if (memcmp(config, "freq", 4) == 0) { diff --git a/src/helpers/RegionMap.cpp b/src/helpers/RegionMap.cpp index 368446157..7a143f633 100644 --- a/src/helpers/RegionMap.cpp +++ b/src/helpers/RegionMap.cpp @@ -6,7 +6,8 @@ RegionMap::RegionMap(TransportKeyStore& store) : _store(&store) { next_id = 1; num_regions = 0; home_id = 0; wildcard.id = wildcard.parent = 0; wildcard.flags = 0; // default behaviour, allow flood and direct - strcpy(wildcard.name, "*"); + strncpy(wildcard.name, "*", sizeof(wildcard.name) - 1); + wildcard.name[sizeof(wildcard.name) - 1] = '\0'; } bool RegionMap::is_name_char(char c) { From 847163149d1a55260cd57fcf0064422b04e7682f Mon Sep 17 00:00:00 2001 From: Arijan Luiken Date: Thu, 11 Dec 2025 21:35:37 +0100 Subject: [PATCH 2/2] Define MAX_SERIAL_MSG_SIZE constant (160 bytes) - Matches the buffer size used in all example applications - Required for the bounded string operations added in security fixes --- src/helpers/CommonCLI.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/helpers/CommonCLI.h b/src/helpers/CommonCLI.h index 068783ab1..2fd1d76d9 100644 --- a/src/helpers/CommonCLI.h +++ b/src/helpers/CommonCLI.h @@ -8,6 +8,8 @@ #define WITH_BRIDGE #endif +#define MAX_SERIAL_MSG_SIZE 160 + #define ADVERT_LOC_NONE 0 #define ADVERT_LOC_SHARE 1 #define ADVERT_LOC_PREFS 2