Skip to content

Security Fixes: Buffer Overflow Prevention and Packet Validation #1205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
arijanluiken wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Security Fixes: Buffer Overflow Prevention and Packet Validation #1205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b452af
Security fixes: Replace unsafe string operations and improve packet v…
arijanluiken Dec 11, 2025
8471631
Define MAX_SERIAL_MSG_SIZE constant (160 bytes)
arijanluiken Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/Packet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
80 changes: 46 additions & 34 deletions src/helpers/CommonCLI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,35 +184,39 @@ 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();
if (secs > curr) {
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);
Expand All @@ -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;
Expand All @@ -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) {
Expand Down
2 changes: 2 additions & 0 deletions src/helpers/CommonCLI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/helpers/RegionMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down