From 18f5627b2a7614f399de81e0031a39f14d0cef42 Mon Sep 17 00:00:00 2001 From: Jakub <106778416+MarekZegare4@users.noreply.github.com> Date: Mon, 12 Oct 2026 19:53:53 +0200 Subject: [PATCH] fix: upstream security and robustness fixes (MACThenDecrypt alignment, CLI buffer, packet pool, raw data paths, ESP32 time after reset, Bosch sensor address) Co-Authored-By: Claude Opus 5.5 --- examples/companion_radio/MyMesh.cpp | 34 +++++++++++++------ examples/simple_repeater/main.cpp | 12 +++++-- examples/simple_room_server/main.cpp | 12 +++++-- examples/simple_secure_chat/main.cpp | 12 +++++-- examples/simple_sensor/main.cpp | 12 +++++-- src/Utils.cpp | 11 ++++-- src/Utils.h | 2 +- src/helpers/ESP32Board.h | 31 ++++++++++++++--- src/helpers/StaticPoolPacketManager.cpp | 9 +++-- src/helpers/StaticPoolPacketManager.h | 5 ++- .../sensors/EnvironmentSensorManager.cpp | 25 +++++++++++--- 11 files changed, 130 insertions(+), 35 deletions(-) diff --git a/examples/companion_radio/MyMesh.cpp b/examples/companion_radio/MyMesh.cpp index 6791e700..c1444225 100644 --- a/examples/companion_radio/MyMesh.cpp +++ b/examples/companion_radio/MyMesh.cpp @@ -2065,16 +2065,20 @@ void MyMesh::handleCmdFrame(size_t len) { #endif } else if (cmd_frame[0] == CMD_SEND_RAW_DATA && len >= 6) { int i = 1; - int8_t path_len = cmd_frame[i++]; - if (path_len >= 0 && i + path_len + 4 <= len) { // minimum 4 byte payload - uint8_t *path = &cmd_frame[i]; - i += path_len; - auto pkt = createRawData(&cmd_frame[i], len - i); - if (pkt) { - sendDirect(pkt, path, path_len); - writeOKFrame(); + uint8_t path_len = cmd_frame[i++]; // encoded: hash size in the top bits (upstream 0cce9197 / 67d6d42a) + if (mesh::Packet::isValidPathLen(path_len)) { + uint8_t path[MAX_PATH_SIZE]; + i += mesh::Packet::writePath(path, &cmd_frame[i], path_len); + if (i + 4 > len) { // min payload 4 bytes + writeErrFrame(ERR_CODE_ILLEGAL_ARG); } else { - writeErrFrame(ERR_CODE_TABLE_FULL); + auto pkt = createRawData(&cmd_frame[i], len - i); + if (pkt) { + sendDirect(pkt, path, path_len); + writeOKFrame(); + } else { + writeErrFrame(ERR_CODE_TABLE_FULL); + } } } else { writeErrFrame(ERR_CODE_UNSUPPORTED_CMD); // flood, not supported (yet) @@ -2634,6 +2638,13 @@ void MyMesh::enterCLIRescue() { void MyMesh::checkCLIRescueCmd() { int len = strlen(cli_command); + // `cli_command` must stay NUL-terminated within its bounds. If it ever isn't, + // strlen() above can return >= sizeof(cli_command) and the loop below would + // then index past the buffer, so clamp defensively. + if (len >= (int)sizeof(cli_command)) { + cli_command[0] = 0; + len = 0; + } while (Serial.available() && len < sizeof(cli_command)-1) { char c = Serial.read(); if (c != '\n') { @@ -2642,8 +2653,9 @@ void MyMesh::checkCLIRescueCmd() { } Serial.print(c); // echo } - if (len == sizeof(cli_command)-1) { // command buffer full - cli_command[sizeof(cli_command)-1] = '\r'; + if (len == sizeof(cli_command)-1) { // buffer full: treat as a completed line + cli_command[sizeof(cli_command)-2] = '\r'; // place end-of-line marker inside the buffer + cli_command[sizeof(cli_command)-1] = 0; // keep the buffer NUL-terminated } if (len > 0 && cli_command[len - 1] == '\r') { // received complete line diff --git a/examples/simple_repeater/main.cpp b/examples/simple_repeater/main.cpp index 374e2a84..50747e8d 100644 --- a/examples/simple_repeater/main.cpp +++ b/examples/simple_repeater/main.cpp @@ -204,6 +204,13 @@ void setup() { void loop() { // Handle Serial CLI int len = strlen(command); + // `command` must stay NUL-terminated within its bounds. If it ever isn't, + // strlen() above can return >= sizeof(command) and the loop below would then + // index past the buffer, so clamp defensively. + if (len >= (int)sizeof(command)) { + command[0] = 0; + len = 0; + } while (Serial.available() && len < sizeof(command)-1) { char c = Serial.read(); if (c != '\n') { @@ -213,8 +220,9 @@ void loop() { } if (c == '\r') break; } - if (len == sizeof(command)-1) { // command buffer full - command[sizeof(command)-1] = '\r'; + if (len == sizeof(command)-1) { // buffer full: treat as a completed line + command[sizeof(command)-2] = '\r'; // place end-of-line marker inside the buffer + command[sizeof(command)-1] = 0; // keep the buffer NUL-terminated } if (len > 0 && command[len - 1] == '\r') { // received complete line diff --git a/examples/simple_room_server/main.cpp b/examples/simple_room_server/main.cpp index 66cb209a..0adc9c6f 100644 --- a/examples/simple_room_server/main.cpp +++ b/examples/simple_room_server/main.cpp @@ -142,6 +142,13 @@ void setup() { void loop() { int len = strlen(command); + // `command` must stay NUL-terminated within its bounds. If it ever isn't, + // strlen() above can return >= sizeof(command) and the loop below would then + // index past the buffer, so clamp defensively. + if (len >= (int)sizeof(command)) { + command[0] = 0; + len = 0; + } while (Serial.available() && len < sizeof(command)-1) { char c = Serial.read(); if (c != '\n') { @@ -150,8 +157,9 @@ void loop() { } Serial.print(c); } - if (len == sizeof(command)-1) { // command buffer full - command[sizeof(command)-1] = '\r'; + if (len == sizeof(command)-1) { // buffer full: treat as a completed line + command[sizeof(command)-2] = '\r'; // place end-of-line marker inside the buffer + command[sizeof(command)-1] = 0; // keep the buffer NUL-terminated } if (len > 0 && command[len - 1] == '\r') { // received complete line diff --git a/examples/simple_secure_chat/main.cpp b/examples/simple_secure_chat/main.cpp index 807b53e3..34f09c0a 100644 --- a/examples/simple_secure_chat/main.cpp +++ b/examples/simple_secure_chat/main.cpp @@ -529,6 +529,13 @@ public: BaseChatMesh::loop(); int len = strlen(command); + // `command` must stay NUL-terminated within its bounds. If it ever isn't, + // strlen() above can return >= sizeof(command) and the loop below would then + // index past the buffer, so clamp defensively. + if (len >= (int)sizeof(command)) { + command[0] = 0; + len = 0; + } while (Serial.available() && len < sizeof(command)-1) { char c = Serial.read(); if (c != '\n') { @@ -537,8 +544,9 @@ public: } Serial.print(c); } - if (len == sizeof(command)-1) { // command buffer full - command[sizeof(command)-1] = '\r'; + if (len == sizeof(command)-1) { // buffer full: treat as a completed line + command[sizeof(command)-2] = '\r'; // place end-of-line marker inside the buffer + command[sizeof(command)-1] = 0; // keep the buffer NUL-terminated } if (len > 0 && command[len - 1] == '\r') { // received complete line diff --git a/examples/simple_sensor/main.cpp b/examples/simple_sensor/main.cpp index 69182f3a..749ff6ef 100644 --- a/examples/simple_sensor/main.cpp +++ b/examples/simple_sensor/main.cpp @@ -122,6 +122,13 @@ void setup() { void loop() { int len = strlen(command); + // `command` must stay NUL-terminated within its bounds. If it ever isn't, + // strlen() above can return >= sizeof(command) and the loop below would then + // index past the buffer, so clamp defensively. + if (len >= (int)sizeof(command)) { + command[0] = 0; + len = 0; + } while (Serial.available() && len < sizeof(command)-1) { char c = Serial.read(); if (c != '\n') { @@ -130,8 +137,9 @@ void loop() { } Serial.print(c); } - if (len == sizeof(command)-1) { // command buffer full - command[sizeof(command)-1] = '\r'; + if (len == sizeof(command)-1) { // buffer full: treat as a completed line + command[sizeof(command)-2] = '\r'; // place end-of-line marker inside the buffer + command[sizeof(command)-1] = 0; // keep the buffer NUL-terminated } if (len > 0 && command[len - 1] == '\r') { // received complete line diff --git a/src/Utils.cpp b/src/Utils.cpp index 5ae7f0e2..113cb4f6 100644 --- a/src/Utils.cpp +++ b/src/Utils.cpp @@ -147,13 +147,18 @@ int Utils::encryptThenMAC(const uint8_t* shared_secret, uint8_t* dest, const uin int Utils::MACThenDecrypt(const uint8_t* shared_secret, uint8_t* dest, const uint8_t* src, int src_len) { if (src_len <= CIPHER_MAC_SIZE) return 0; // invalid src bytes + // decrypt() works in whole blocks: an unaligned length would write past + // the caller's buffer (upstream 9c9a2ba2 / b74ba545). + int enc_len = src_len - CIPHER_MAC_SIZE; + if (enc_len % CIPHER_BLOCK_SIZE != 0) return 0; // reject non-block-aligned ciphertext + uint8_t hmac[CIPHER_MAC_SIZE]; #ifdef USE_CC310_HW_CRYPTO { static CRYS_HMACUserContext_t hmac_ctx; static CRYS_HASH_Result_t hmac_result; CRYS_HMAC_Init(&hmac_ctx, CRYS_HASH_SHA256_mode, (uint8_t*)shared_secret, PUB_KEY_SIZE); - CRYS_HMAC_Update(&hmac_ctx, (uint8_t*)(src + CIPHER_MAC_SIZE), src_len - CIPHER_MAC_SIZE); + CRYS_HMAC_Update(&hmac_ctx, (uint8_t*)(src + CIPHER_MAC_SIZE), enc_len); CRYS_HMAC_Finish(&hmac_ctx, hmac_result); memcpy(hmac, hmac_result, CIPHER_MAC_SIZE); } @@ -161,12 +166,12 @@ int Utils::MACThenDecrypt(const uint8_t* shared_secret, uint8_t* dest, const uin { SHA256 sha; sha.resetHMAC(shared_secret, PUB_KEY_SIZE); - sha.update(src + CIPHER_MAC_SIZE, src_len - CIPHER_MAC_SIZE); + sha.update(src + CIPHER_MAC_SIZE, enc_len); sha.finalizeHMAC(shared_secret, PUB_KEY_SIZE, hmac, CIPHER_MAC_SIZE); } #endif if (memcmp(hmac, src, CIPHER_MAC_SIZE) == 0) { - return decrypt(shared_secret, dest, src + CIPHER_MAC_SIZE, src_len - CIPHER_MAC_SIZE); + return decrypt(shared_secret, dest, src + CIPHER_MAC_SIZE, enc_len); } return 0; // invalid HMAC } diff --git a/src/Utils.h b/src/Utils.h index 5736b874..9997fe3c 100644 --- a/src/Utils.h +++ b/src/Utils.h @@ -50,7 +50,7 @@ public: /** * \brief checks the MAC (in leading bytes of 'src'), then if valid, decrypts remaining bytes in src. - * \returns zero if MAC is invalid, otherwise the length of decrypted bytes in 'dest' + * \returns zero if MAC is invalid or ciphertext is not block-aligned, otherwise the length of decrypted bytes in 'dest' */ static int MACThenDecrypt(const uint8_t* shared_secret, uint8_t* dest, const uint8_t* src, int src_len); diff --git a/src/helpers/ESP32Board.h b/src/helpers/ESP32Board.h index d7eb5fee..27d429b3 100644 --- a/src/helpers/ESP32Board.h +++ b/src/helpers/ESP32Board.h @@ -197,19 +197,30 @@ public: } }; +static RTC_NOINIT_ATTR uint32_t _rtc_backup_time; +static RTC_NOINIT_ATTR uint32_t _rtc_backup_magic; +#define RTC_BACKUP_MAGIC 0xAA55CC33 +#define RTC_TIME_MIN 1772323200 // 1 Mar 2026 + class ESP32RTCClock : public mesh::RTCClock { public: ESP32RTCClock() { } void begin() { esp_reset_reason_t reason = esp_reset_reason(); - if (reason == ESP_RST_POWERON) { - // start with some date/time in the recent past - struct timeval tv; - tv.tv_sec = 1715770351; // 15 May 2024, 8:50pm + if (reason == ESP_RST_DEEPSLEEP) { + return; // ESP-IDF preserves system time across deep sleep + } + // All other resets (power-on, crash, WDT, brownout) lose system time. + // Restore from RTC backup if valid, otherwise use hardcoded seed. + struct timeval tv; + if (_rtc_backup_magic == RTC_BACKUP_MAGIC && _rtc_backup_time > RTC_TIME_MIN) { + tv.tv_sec = _rtc_backup_time; + } else { + tv.tv_sec = 1772323200; // 1 Mar 2026 + } tv.tv_usec = 0; settimeofday(&tv, NULL); } - } uint32_t getCurrentTime() override { time_t _now; time(&_now); @@ -220,6 +231,16 @@ public: tv.tv_sec = time; tv.tv_usec = 0; settimeofday(&tv, NULL); + _rtc_backup_time = time; + _rtc_backup_magic = RTC_BACKUP_MAGIC; + } + void tick() override { + time_t now; + time(&now); + if (now > RTC_TIME_MIN && (uint32_t)now != _rtc_backup_time) { + _rtc_backup_time = (uint32_t)now; + _rtc_backup_magic = RTC_BACKUP_MAGIC; + } } }; diff --git a/src/helpers/StaticPoolPacketManager.cpp b/src/helpers/StaticPoolPacketManager.cpp index b8926df0..8a8eefe8 100644 --- a/src/helpers/StaticPoolPacketManager.cpp +++ b/src/helpers/StaticPoolPacketManager.cpp @@ -44,7 +44,7 @@ mesh::Packet* PacketQueue::get(uint32_t now) { } mesh::Packet* PacketQueue::removeByIdx(int i) { - if (i >= _num) return NULL; // invalid index + if (i < 0 || i >= _num) return NULL; // invalid index mesh::Packet* item = _table[i]; _num--; @@ -80,10 +80,14 @@ mesh::Packet* StaticPoolPacketManager::allocNew() { } void StaticPoolPacketManager::free(mesh::Packet* packet) { - unused.add(packet, 0, 0); + if (packet == NULL) return; + if (!unused.add(packet, 0, 0)) { + MESH_DEBUG_PRINTLN("free: unused queue full, possible double-free detected"); + } } void StaticPoolPacketManager::queueOutbound(mesh::Packet* packet, uint8_t priority, uint32_t scheduled_for) { + if (packet == NULL) return; if (!send_queue.add(packet, priority, scheduled_for)) { MESH_DEBUG_PRINTLN("queueOutbound: send queue full, dropping packet"); free(packet); @@ -115,6 +119,7 @@ mesh::Packet* StaticPoolPacketManager::removeOutboundByIdx(int i) { } void StaticPoolPacketManager::queueInbound(mesh::Packet* packet, uint32_t scheduled_for) { + if (packet == NULL) return; if (!rx_queue.add(packet, 0, scheduled_for)) { MESH_DEBUG_PRINTLN("queueInbound: rx queue full, dropping packet"); free(packet); diff --git a/src/helpers/StaticPoolPacketManager.h b/src/helpers/StaticPoolPacketManager.h index 59715b4e..f63700e5 100644 --- a/src/helpers/StaticPoolPacketManager.h +++ b/src/helpers/StaticPoolPacketManager.h @@ -14,7 +14,10 @@ public: bool add(mesh::Packet* packet, uint8_t priority, uint32_t scheduled_for); int count() const { return _num; } int countBefore(uint32_t now) const; - mesh::Packet* itemAt(int i) const { return _table[i]; } + mesh::Packet* itemAt(int i) const { + if (i < 0 || i >= _num) return NULL; + return _table[i]; + } mesh::Packet* removeByIdx(int i); }; diff --git a/src/helpers/sensors/EnvironmentSensorManager.cpp b/src/helpers/sensors/EnvironmentSensorManager.cpp index 36301f51..a695fb16 100644 --- a/src/helpers/sensors/EnvironmentSensorManager.cpp +++ b/src/helpers/sensors/EnvironmentSensorManager.cpp @@ -539,6 +539,8 @@ static void query_bme680_bsec(uint8_t ch, uint8_t, CayenneLPP& lpp) { // are compiled in. The sentinel at the end keeps the array // non-empty regardless of which sensors are enabled. // +// Bosch BMP/BME SDO selects 0x76 or 0x77; probe both. +// // Ordering here determines channel assignment at runtime: // the first detected+initialized sensor gets channel 2, the // next gets channel 3, and so on. @@ -551,21 +553,27 @@ struct SensorDef { void (*query)(uint8_t channel, uint8_t sub_channel, CayenneLPP& telemetry); }; +#define TELEM_BOSCH_ALT_ADDR(addr) ((uint8_t)((addr) == 0x76 ? 0x77 : 0x76)) + static const SensorDef SENSOR_TABLE[] = { #if ENV_INCLUDE_AHTX0 { TELEM_AHTX_ADDRESS, "AHT10/AHT20", init_ahtx0, query_ahtx0 }, #endif #ifdef ENV_INCLUDE_BME680 - { TELEM_BME680_ADDRESS, "BME680", init_bme680, query_bme680 }, + { TELEM_BME680_ADDRESS, "BME680", init_bme680, query_bme680 }, + { TELEM_BOSCH_ALT_ADDR(TELEM_BME680_ADDRESS), "BME680", init_bme680, query_bme680 }, #endif #if ENV_INCLUDE_BME680_BSEC - { TELEM_BME680_ADDRESS, "BME680+BSEC", init_bme680_bsec, query_bme680_bsec }, + { TELEM_BME680_ADDRESS, "BME680+BSEC", init_bme680_bsec, query_bme680_bsec }, + { TELEM_BOSCH_ALT_ADDR(TELEM_BME680_ADDRESS), "BME680+BSEC", init_bme680_bsec, query_bme680_bsec }, #endif #if ENV_INCLUDE_BME280 - { TELEM_BME280_ADDRESS, "BME280", init_bme280, query_bme280 }, + { TELEM_BME280_ADDRESS, "BME280", init_bme280, query_bme280 }, + { TELEM_BOSCH_ALT_ADDR(TELEM_BME280_ADDRESS), "BME280", init_bme280, query_bme280 }, #endif #if ENV_INCLUDE_BMP280 - { TELEM_BMP280_ADDRESS, "BMP280", init_bmp280, query_bmp280 }, + { TELEM_BMP280_ADDRESS, "BMP280", init_bmp280, query_bmp280 }, + { TELEM_BOSCH_ALT_ADDR(TELEM_BMP280_ADDRESS), "BMP280", init_bmp280, query_bmp280 }, #endif #if ENV_INCLUDE_SHTC3 { 0x70, "SHTC3", init_shtc3, query_shtc3 }, @@ -603,6 +611,8 @@ static const SensorDef SENSOR_TABLE[] = { { 0, nullptr, nullptr, nullptr } // sentinel — keeps the array non-empty }; +#undef TELEM_BOSCH_ALT_ADDR + static const size_t SENSOR_TABLE_SIZE = (sizeof(SENSOR_TABLE) / sizeof(SENSOR_TABLE[0])) - 1; // ============================================================ @@ -640,6 +650,12 @@ bool EnvironmentSensorManager::begin() { _active_sensor_count = 0; for (size_t i = 0; i < SENSOR_TABLE_SIZE && _active_sensor_count < MAX_ACTIVE_SENSORS; i++) { const SensorDef& def = SENSOR_TABLE[i]; + // One static driver instance per type: an alternate address is a fallback, not a second device. + bool already_active = false; + for (int j = 0; j < _active_sensor_count; j++) { + if (_active_sensors[j].query == def.query) { already_active = true; break; } + } + if (already_active) continue; if (!detected[def.address]) { MESH_DEBUG_PRINTLN("%s not detected at I2C address %02X", def.name, def.address); continue; @@ -650,6 +666,7 @@ bool EnvironmentSensorManager::begin() { continue; } MESH_DEBUG_PRINTLN("Found %s at address: %02X", def.name, def.address); + detected[def.address] = false; // consumed; later entries must not re-claim this device for (uint8_t sub = 0; sub < n && _active_sensor_count < MAX_ACTIVE_SENSORS; sub++) { _active_sensors[_active_sensor_count++] = { def.query, sub }; }