From 0b6780678f601041b7f4d63c8e087665c335d8ff Mon Sep 17 00:00:00 2001 From: Florian Date: Sun, 14 Jun 2026 17:20:24 -0500 Subject: [PATCH] [SYS] Bound remaining MQTT/HTTP/BLE/RF inputs into fixed buffers (#2324) Follow-up to #2321. That PR fixed five strcpy sites in XtoSYS/createOrUpdateDevice but left adjacent attack surfaces using the same pattern (strncpy with the destination's own size, leaving the buffer non-NUL-terminated when the source is at the limit; or strcpy of WiFiManager/HTTP form values that bypass the client-side maxlength). The over-long string then flows into saveConfig() and back into a strcpy on next boot, corrupting heap/.bss on every reload. main/main.cpp - XtoSYS: length-check before copying mqtt_topic, discovery_prefix, gateway_name, gw_pass (ota_pass), and ble_aes. Reject over-length with THEENGS_LOG_WARNING, preserving existing values. main/main.cpp - loadConfigFromFlash: new loadStrField() helper applies the same guard to every SPIFFS-loaded string (mqtt_server/port/user/pass per slot, mqtt_topic, discovery_prefix, gateway_name, ota_pass, ble_aes). Defense in depth in case a pre-fix build left an over-length value in /config.json. main/main.cpp - WiFiManager save: bound mqtt_topic against the 150-byte portal buffer overflowing the 66-byte destination (HTML maxlength is client-side only; a raw POST bypasses it). main/webUI.cpp - handleCG /gw save: bound gateway-password form input before the strncpy that previously left ota_pass non-NUL-terminated. main/blufi.cpp - ESP_BLUFI_EVENT_RECV_STA_SSID/PASSWD: clamp the wire-supplied ssid_len/passwd_len before the memcpy and the trailing NUL so a crafted BluFi provisioning frame cannot overrun gl_sta_ssid[32]/gl_sta_passwd[64]. main/gatewayBT.cpp - BLEDecryptor (Victron, encr==3): remove `nonce[16] = {0}`, a one-byte stack-OOB write reachable from any BLE adv. The line was dead (the memset two lines later zeroes the whole array); deleting it eliminates the OOB write. main/gatewaySRFB.cpp - SRFBtoX: bound the _uartbuf writer so a 433 MHz frame with no STOP can no longer let _uartpos grow past sizeof(_uartbuf). main/gatewaySRFB.cpp - _rfbSend: the hex output buffer was sized RF_MESSAGE_SIZE (9) but _rawToHex writes 2*N+2 bytes per call. Resize to fit. Tested on a Theengs Bridge: built theengs-bridge-v11 env, OTA-flashed, and verified that publishing {"discovery_prefix":"Z"*200} to MQTTtoSYS/config leaves discovery_prefix unchanged (no Z*65 truncation in subsequent discovery topics), bridge keeps decoding BLE adverts, and SYStoMQTT/LWT stay healthy. Co-authored-by: Claude Opus 4.7 (1M context) --- main/blufi.cpp | 23 +++++++++--- main/gatewayBT.cpp | 5 ++- main/gatewaySRFB.cpp | 13 ++++++- main/main.cpp | 88 +++++++++++++++++++++++++++++++++++--------- main/webUI.cpp | 13 ++++++- 5 files changed, 113 insertions(+), 29 deletions(-) diff --git a/main/blufi.cpp b/main/blufi.cpp index 5449fb9..7b1fef5 100644 --- a/main/blufi.cpp +++ b/main/blufi.cpp @@ -321,16 +321,27 @@ static void example_event_callback(esp_blufi_cb_event_t event, esp_blufi_cb_para THEENGS_LOG_TRACE(F("BLUFI recv slave disconnect a ble connection" CR)); esp_blufi_disconnect(); break; - case ESP_BLUFI_EVENT_RECV_STA_SSID: - strncpy((char*)gl_sta_ssid, (char*)param->sta_ssid.ssid, param->sta_ssid.ssid_len); - gl_sta_ssid[param->sta_ssid.ssid_len] = '\0'; + case ESP_BLUFI_EVENT_RECV_STA_SSID: { + // ssid_len comes from an unauthenticated BLE provisioning frame; clamp it + // before the strncpy and the trailing NUL so a crafted long SSID cannot + // write past gl_sta_ssid. + size_t ssid_len = param->sta_ssid.ssid_len; + if (ssid_len > sizeof(gl_sta_ssid) - 1) + ssid_len = sizeof(gl_sta_ssid) - 1; + memcpy(gl_sta_ssid, param->sta_ssid.ssid, ssid_len); + gl_sta_ssid[ssid_len] = '\0'; THEENGS_LOG_NOTICE(F("Recv STA SSID %s" CR), gl_sta_ssid); break; - case ESP_BLUFI_EVENT_RECV_STA_PASSWD: - strncpy((char*)gl_sta_passwd, (char*)param->sta_passwd.passwd, param->sta_passwd.passwd_len); - gl_sta_passwd[param->sta_passwd.passwd_len] = '\0'; + } + case ESP_BLUFI_EVENT_RECV_STA_PASSWD: { + size_t passwd_len = param->sta_passwd.passwd_len; + if (passwd_len > sizeof(gl_sta_passwd) - 1) + passwd_len = sizeof(gl_sta_passwd) - 1; + memcpy(gl_sta_passwd, param->sta_passwd.passwd, passwd_len); + gl_sta_passwd[passwd_len] = '\0'; THEENGS_LOG_NOTICE(F("Recv STA PASSWORD" CR)); break; + } case ESP_BLUFI_EVENT_GET_WIFI_LIST: { WiFi.scanNetworks(true); break; diff --git a/main/gatewayBT.cpp b/main/gatewayBT.cpp index dfa307d..ee86b83 100644 --- a/main/gatewayBT.cpp +++ b/main/gatewayBT.cpp @@ -1348,7 +1348,10 @@ void process_bledata(JsonObject& BLEdata) { THEENGS_LOG_TRACE(F("[BLEDecryptor] BTHomeV2 nonce %s" CR), NimBLEUtils::dataToHexString(nonce, noncelength).c_str()); } else if (BLEdata["encr"].as() == 3) { - nonce[16] = {0}; // Victron has a 16 byte zero padded nonce with IV bytes 6,7 + // Victron has a 16-byte zero-padded nonce with IV bytes 0,1. + // (The line `nonce[16] = {0};` that lived here wrote a byte one past the + // end of the 16-byte `nonce` array on every Victron-encrypted advert; + // memset below already zeroes the whole buffer, so the typo was dead.) unsigned char iv[2]; int ivlen = hexToBytes(BLEdata["ctr"].as(), iv, 2); if (ivlen != 2) { diff --git a/main/gatewaySRFB.cpp b/main/gatewaySRFB.cpp index 88e6d33..ca7d521 100644 --- a/main/gatewaySRFB.cpp +++ b/main/gatewaySRFB.cpp @@ -60,7 +60,9 @@ void _rfbSend(byte* message) { } void _rfbSend(byte* message, int times) { - char buffer[RF_MESSAGE_SIZE]; + // _rawToHex writes 3 bytes per input byte (two hex chars + CR) followed by a + // terminating NUL, so the buffer needs 2 * RF_MESSAGE_SIZE + 2 bytes. + char buffer[RF_MESSAGE_SIZE * 2 + 2] = {0}; TheengsUtils::_rawToHex(message, buffer, RF_MESSAGE_SIZE); THEENGS_LOG_NOTICE(F("[RFBRIDGE] Sending MESSAGE" CR)); @@ -85,8 +87,15 @@ bool SRFBtoX() { if (c == RF_CODE_STOP) { _rfbDecode(); receiving = false; - } else { + } else if (_uartpos < sizeof(_uartbuf)) { _uartbuf[_uartpos++] = c; + } else { + // Frame longer than expected — drop it rather than overrun _uartbuf. + // A crafted 433 MHz transmission that emits START but never STOP would + // otherwise let _uartpos grow up to 255 and trample adjacent globals. + THEENGS_LOG_WARNING(F("[RFBRIDGE] RF frame too long, dropping" CR)); + receiving = false; + _uartpos = 0; } } else if (c == RF_CODE_START) { _uartpos = 0; diff --git a/main/main.cpp b/main/main.cpp index 639397a..d10b925 100644 --- a/main/main.cpp +++ b/main/main.cpp @@ -2049,6 +2049,19 @@ void saveConfig() { configFile.close(); } +// Length-checked copy used when loading config.json values into fixed-size +// buffers. The SPIFFS file is implicitly trusted once written, but historical +// MQTT/HTTP paths could land over-length values there before we hardened them; +// this prevents one stale config from overflowing globals on every boot. +static void loadStrField(const char* name, char* dst, size_t dstSize, const char* src) { + if (!src) return; + if (strlen(src) >= dstSize) { + THEENGS_LOG_WARNING(F("Config field %s too long (%u), skipping" CR), name, (unsigned)strlen(src)); + return; + } + strcpy(dst, src); +} + bool loadConfigFromFlash() { THEENGS_LOG_TRACE(F("mounting FS..." CR)); bool result = false; @@ -2124,22 +2137,22 @@ bool loadConfigFromFlash() { strcpy(key, "mqtt_server"); strcat(key, index_suffix); if (json.containsKey(key)) { - strcpy(cnt_parameters_array[i].mqtt_server, json[key].as()); + loadStrField(key, cnt_parameters_array[i].mqtt_server, sizeof(cnt_parameters_array[i].mqtt_server), json[key].as()); } strcpy(key, "mqtt_port"); strcat(key, index_suffix); if (json.containsKey(key)) { - strcpy(cnt_parameters_array[i].mqtt_port, json[key].as()); + loadStrField(key, cnt_parameters_array[i].mqtt_port, sizeof(cnt_parameters_array[i].mqtt_port), json[key].as()); } strcpy(key, "mqtt_user"); strcat(key, index_suffix); if (json.containsKey(key)) { - strcpy(cnt_parameters_array[i].mqtt_user, json[key].as()); + loadStrField(key, cnt_parameters_array[i].mqtt_user, sizeof(cnt_parameters_array[i].mqtt_user), json[key].as()); } strcpy(key, "mqtt_pass"); strcat(key, index_suffix); if (json.containsKey(key)) { - strcpy(cnt_parameters_array[i].mqtt_pass, json[key].as()); + loadStrField(key, cnt_parameters_array[i].mqtt_pass, sizeof(cnt_parameters_array[i].mqtt_pass), json[key].as()); } strcpy(key, "mqtt_broker_secure"); strcat(key, index_suffix); @@ -2166,15 +2179,16 @@ bool loadConfigFromFlash() { } # endif if (json.containsKey("mqtt_topic")) - strcpy(mqtt_topic, json["mqtt_topic"]); + loadStrField("mqtt_topic", mqtt_topic, sizeof(mqtt_topic), json["mqtt_topic"]); # ifdef ZmqttDiscovery if (json.containsKey("discovery_prefix")) - strcpy(discovery_prefix, json["discovery_prefix"]); + // discovery_prefix is extern char[] in this TU; size is parameters_size + 1. + loadStrField("discovery_prefix", discovery_prefix, parameters_size + 1, json["discovery_prefix"]); # endif if (json.containsKey("gateway_name")) - strcpy(gateway_name, json["gateway_name"]); + loadStrField("gateway_name", gateway_name, sizeof(gateway_name), json["gateway_name"]); if (json.containsKey("ota_pass")) { - strcpy(ota_pass, json["ota_pass"]); + loadStrField("ota_pass", ota_pass, sizeof(ota_pass), json["ota_pass"]); # ifdef WM_PWD_FROM_MAC // From ESP Mac Address, last 8 digits as the password // Compare the existing ota_pass if ota_pass = OTAPASSWORD then replace with the last 8 digits of the mac address // This enable user migrating from previous version to have the same WiFi portal password as previously unless they changed it @@ -2187,7 +2201,7 @@ bool loadConfigFromFlash() { } # if BLEDecryptor if (json.containsKey("ble_aes")) { - strcpy(ble_aes, json["ble_aes"]); + loadStrField("ble_aes", ble_aes, sizeof(ble_aes), json["ble_aes"]); THEENGS_LOG_TRACE(F("loaded default BLE AES key %s" CR), ble_aes); } if (json.containsKey("ble_aes_keys")) { @@ -2376,9 +2390,17 @@ void setupWiFiManager() { strcpy(cnt_parameters_array[cnt_index].mqtt_pass, custom_mqtt_pass.getValue()); } if (strlen(custom_mqtt_topic.getValue()) > 0) { - strcpy(mqtt_topic, custom_mqtt_topic.getValue()); - if (mqtt_topic[strlen(mqtt_topic) - 1] != '/' && strlen(mqtt_topic) < parameters_size) { - strcat(mqtt_topic, "/"); + // The WiFiManagerParameter buffer is mqtt_topic_max_size (150) but the + // destination is sizeof(mqtt_topic). HTML maxlength is client-side only; + // a raw POST can deliver any length, so length-check before copying. + const char* src = custom_mqtt_topic.getValue(); + if (strlen(src) >= sizeof(mqtt_topic)) { + THEENGS_LOG_WARNING(F("mqtt_topic from portal too long, ignoring" CR)); + } else { + strcpy(mqtt_topic, src); + if (mqtt_topic[strlen(mqtt_topic) - 1] != '/' && strlen(mqtt_topic) < parameters_size) { + strcat(mqtt_topic, "/"); + } } } @@ -3474,20 +3496,45 @@ void XtoSYS(const char* topicOri, JsonObject& SYSdata) { // json object decoding #endif (SYSdata.containsKey("gateway_name") && SYSdata["gateway_name"].is()) || (SYSdata.containsKey("gw_pass") && SYSdata["gw_pass"].is())) { + // Each of the destination buffers is sized at compile time; reject + // over-length MQTT-supplied values up front so a crafted publish cannot + // leave the destination non-NUL-terminated (which would corrupt the + // SPIFFS config on the next saveConfig()). if (SYSdata.containsKey("mqtt_topic")) { - strncpy(mqtt_topic, SYSdata["mqtt_topic"], parameters_size); + const char* src = SYSdata["mqtt_topic"]; + if (strlen(src) >= sizeof(mqtt_topic)) { + THEENGS_LOG_WARNING(F("mqtt_topic too long, ignoring" CR)); + } else { + strcpy(mqtt_topic, src); + } } #ifdef ZmqttDiscovery if (SYSdata.containsKey("discovery_prefix")) { - strncpy(discovery_prefix, SYSdata["discovery_prefix"], parameters_size); + const char* src = SYSdata["discovery_prefix"]; + // discovery_prefix is defined in mqttDiscovery.cpp as char[parameters_size + 1]. + if (strlen(src) > parameters_size) { + THEENGS_LOG_WARNING(F("discovery_prefix too long, ignoring" CR)); + } else { + strcpy(discovery_prefix, src); + } } #endif if (SYSdata.containsKey("gateway_name")) { - strncpy(gateway_name, SYSdata["gateway_name"], parameters_size); + const char* src = SYSdata["gateway_name"]; + if (strlen(src) >= sizeof(gateway_name)) { + THEENGS_LOG_WARNING(F("gateway_name too long, ignoring" CR)); + } else { + strcpy(gateway_name, src); + } } if (SYSdata.containsKey("gw_pass")) { - strncpy(ota_pass, SYSdata["gw_pass"], parameters_size); - restartESP = true; + const char* src = SYSdata["gw_pass"]; + if (strlen(src) >= sizeof(ota_pass)) { + THEENGS_LOG_WARNING(F("gw_pass too long, ignoring" CR)); + } else { + strcpy(ota_pass, src); + restartESP = true; + } } #ifndef ESPWifiManualSetup saveConfig(); @@ -3498,7 +3545,12 @@ void XtoSYS(const char* topicOri, JsonObject& SYSdata) { // json object decoding #if BLEDecryptor if (SYSdata.containsKey("ble_aes") || SYSdata.containsKey("ble_aes_keys")) { if (SYSdata.containsKey("ble_aes")) { - strncpy(ble_aes, SYSdata["ble_aes"], parameters_size); + const char* src = SYSdata["ble_aes"]; + if (strlen(src) >= sizeof(ble_aes)) { + THEENGS_LOG_WARNING(F("ble_aes too long, ignoring" CR)); + } else { + strcpy(ble_aes, src); + } } if (SYSdata.containsKey("ble_aes_keys")) { THEENGS_LOG_WARNING(F("Contains ble_aes_keys" CR)); diff --git a/main/webUI.cpp b/main/webUI.cpp index d456861..609f847 100644 --- a/main/webUI.cpp +++ b/main/webUI.cpp @@ -880,8 +880,17 @@ void handleCG() { } if (server.hasArg("save")) { if (server.hasArg("gp") && server.arg("gp").length() > 0 && strcmp(ota_pass, server.arg("gp").c_str())) { - strncpy(ota_pass, server.arg("gp").c_str(), parameters_size); - pwUpdate = true; + // HTML maxlength is client-side only — a direct POST can deliver any + // length. Reject anything that would not fit (and leave dst + // NUL-terminated) before copying into the fixed-size buffer. + const char* gp = server.arg("gp").c_str(); + // ota_pass is extern char[] in this TU; size is parameters_size. + if (strlen(gp) >= parameters_size) { + THEENGS_LOG_WARNING(F("[WebUI] gateway password too long, ignoring" CR)); + } else { + strcpy(ota_pass, gp); + pwUpdate = true; + } } # ifdef ZmqttDiscovery if (SYSConfig.discovery != server.hasArg("dc")) {