From c027dd225dafffbe586e132334adb998e2924951 Mon Sep 17 00:00:00 2001 From: Florian Date: Sat, 6 Jun 2026 09:35:27 -0500 Subject: [PATCH] [SYS] Fix heap overflows from unbounded strcpy of MQTT input (#2321) Guard createOrUpdateDevice() in gatewayBT.cpp against null or >17-char MAC strings before the strcpy into the 18-byte BLEdevice::macAdr buffer. An MQTT client could previously publish a crafted white-list/black-list/id field to MQTTtoBT/config (and related topics) and corrupt the heap. Apply the same defense to the XtoSYS config branch in main.cpp: mqtt_user, mqtt_pass, mqtt_server, and mqtt_port are now length-checked against their destination buffer sizes before strcpy. The mqtt_port destination is only 6 bytes, so this path was particularly easy to overflow via MQTTtoSYS/config. Oversized fields are now rejected with a warning log; existing values are preserved and the device does not crash. Co-authored-by: Florian <1technophile@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) --- main/gatewayBT.cpp | 7 +++++++ main/main.cpp | 33 ++++++++++++++++++++++++++------- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/main/gatewayBT.cpp b/main/gatewayBT.cpp index b147b68..dfa307d 100644 --- a/main/gatewayBT.cpp +++ b/main/gatewayBT.cpp @@ -370,6 +370,13 @@ void createOrUpdateDevice(const char* mac, uint8_t flags, int model, int mac_typ THEENGS_LOG_ERROR(F("Semaphore NOT taken" CR)); return; } + // A BLE MAC is XX:XX:XX:XX:XX:XX = 17 chars; macAdr is 18 bytes including the null. + // Reject anything longer so untrusted MQTT input cannot overflow the heap buffer. + if (mac == nullptr || strlen(mac) > 17) { + THEENGS_LOG_WARNING(F("Invalid MAC, skipping" CR)); + xSemaphoreGive(semaphoreCreateOrUpdateDevice); + return; + } BLEdevice* device = getDeviceByMac(mac); if (device == &NO_BT_DEVICE_FOUND) { // Evict oldest non-listed device when at capacity diff --git a/main/main.cpp b/main/main.cpp index 79a12d2..639397a 100644 --- a/main/main.cpp +++ b/main/main.cpp @@ -3547,20 +3547,39 @@ void XtoSYS(const char* topicOri, JsonObject& SYSdata) { // json object decoding THEENGS_LOG_NOTICE(F("MQTT cnt index %d" CR), cnt_index); + // Length-check every MQTT-supplied string before copying into a fixed buffer + // to prevent heap overflows. if (SYSdata.containsKey("mqtt_user") && SYSdata["mqtt_user"].is() && SYSdata.containsKey("mqtt_pass") && SYSdata["mqtt_pass"].is()) { - strcpy(cnt_parameters_array[cnt_index].mqtt_user, SYSdata["mqtt_user"]); - strcpy(cnt_parameters_array[cnt_index].mqtt_pass, SYSdata["mqtt_pass"]); - cnt_parameters_array[cnt_index].validConnection = false; + const char* user = SYSdata["mqtt_user"]; + const char* pass = SYSdata["mqtt_pass"]; + if (strlen(user) >= sizeof(cnt_parameters_array[cnt_index].mqtt_user) || + strlen(pass) >= sizeof(cnt_parameters_array[cnt_index].mqtt_pass)) { + THEENGS_LOG_WARNING(F("mqtt_user/mqtt_pass too long, ignoring" CR)); + } else { + strcpy(cnt_parameters_array[cnt_index].mqtt_user, user); + strcpy(cnt_parameters_array[cnt_index].mqtt_pass, pass); + cnt_parameters_array[cnt_index].validConnection = false; + } } if (SYSdata.containsKey("mqtt_server") && SYSdata["mqtt_server"].is()) { - strcpy(cnt_parameters_array[cnt_index].mqtt_server, SYSdata["mqtt_server"]); - cnt_parameters_array[cnt_index].validConnection = false; + const char* server = SYSdata["mqtt_server"]; + if (strlen(server) >= sizeof(cnt_parameters_array[cnt_index].mqtt_server)) { + THEENGS_LOG_WARNING(F("mqtt_server too long, ignoring" CR)); + } else { + strcpy(cnt_parameters_array[cnt_index].mqtt_server, server); + cnt_parameters_array[cnt_index].validConnection = false; + } } if (SYSdata.containsKey("mqtt_port") && SYSdata["mqtt_port"].is()) { - strcpy(cnt_parameters_array[cnt_index].mqtt_port, SYSdata["mqtt_port"]); - cnt_parameters_array[cnt_index].validConnection = false; + const char* port = SYSdata["mqtt_port"]; + if (strlen(port) >= sizeof(cnt_parameters_array[cnt_index].mqtt_port)) { + THEENGS_LOG_WARNING(F("mqtt_port too long, ignoring" CR)); + } else { + strcpy(cnt_parameters_array[cnt_index].mqtt_port, port); + cnt_parameters_array[cnt_index].validConnection = false; + } } if (SYSdata.containsKey("mqtt_secure") && SYSdata["mqtt_secure"].is()) {