[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) <noreply@anthropic.com>
This commit is contained in:
2 files changed
+33
-7
No files matched your search
@@ -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
|
||||
|
||||
+26
-7
@@ -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<const char*>() && SYSdata.containsKey("mqtt_pass") && SYSdata["mqtt_pass"].is<const char*>()) {
|
||||
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<const char*>()) {
|
||||
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<const char*>()) {
|
||||
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<bool>()) {
|
||||
|
||||
Reference in new issue
Block a user