From 0160e3fa87d08cee401bebfe82c0c42e2bd6935d Mon Sep 17 00:00:00 2001 From: Wouter Gritter Date: Wed, 20 Nov 2024 12:39:39 +0100 Subject: [PATCH] Use MQTT_MAX_TOPIC_LEN in places where it was not used before to avoid buffer overflows when value is increased --- wled00/button.cpp | 8 ++++---- wled00/mqtt.cpp | 22 +++++++++++----------- wled00/wled.h | 8 ++++---- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/wled00/button.cpp b/wled00/button.cpp index 4d6f954f6..6f9c84560 100644 --- a/wled00/button.cpp +++ b/wled00/button.cpp @@ -29,7 +29,7 @@ void shortPressAction(uint8_t b) #ifndef WLED_DISABLE_MQTT // publish MQTT message if (buttonPublishMqtt && WLED_MQTT_CONNECTED) { - char subuf[64]; + char subuf[MQTT_MAX_TOPIC_LEN + 32]; sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b); mqtt->publish(subuf, 0, false, "short"); } @@ -62,7 +62,7 @@ void longPressAction(uint8_t b) #ifndef WLED_DISABLE_MQTT // publish MQTT message if (buttonPublishMqtt && WLED_MQTT_CONNECTED) { - char subuf[64]; + char subuf[MQTT_MAX_TOPIC_LEN + 32]; sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b); mqtt->publish(subuf, 0, false, "long"); } @@ -83,7 +83,7 @@ void doublePressAction(uint8_t b) #ifndef WLED_DISABLE_MQTT // publish MQTT message if (buttonPublishMqtt && WLED_MQTT_CONNECTED) { - char subuf[64]; + char subuf[MQTT_MAX_TOPIC_LEN + 32]; sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b); mqtt->publish(subuf, 0, false, "double"); } @@ -151,7 +151,7 @@ void handleSwitch(uint8_t b) #ifndef WLED_DISABLE_MQTT // publish MQTT message if (buttonPublishMqtt && WLED_MQTT_CONNECTED) { - char subuf[64]; + char subuf[MQTT_MAX_TOPIC_LEN + 32]; if (buttonType[b] == BTN_TYPE_PIR_SENSOR) sprintf_P(subuf, PSTR("%s/motion/%d"), mqttDeviceTopic, (int)b); else sprintf_P(subuf, _mqtt_topic_button, mqttDeviceTopic, (int)b); mqtt->publish(subuf, 0, false, !buttonPressedBefore[b] ? "off" : "on"); diff --git a/wled00/mqtt.cpp b/wled00/mqtt.cpp index a476db87a..d909494ee 100644 --- a/wled00/mqtt.cpp +++ b/wled00/mqtt.cpp @@ -23,24 +23,24 @@ static void parseMQTTBriPayload(char* payload) static void onMqttConnect(bool sessionPresent) { //(re)subscribe to required topics - char subuf[38]; + char subuf[MQTT_MAX_TOPIC_LEN + 6]; if (mqttDeviceTopic[0] != 0) { - strlcpy(subuf, mqttDeviceTopic, 33); + strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1); mqtt->subscribe(subuf, 0); strcat_P(subuf, PSTR("/col")); mqtt->subscribe(subuf, 0); - strlcpy(subuf, mqttDeviceTopic, 33); + strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1); strcat_P(subuf, PSTR("/api")); mqtt->subscribe(subuf, 0); } if (mqttGroupTopic[0] != 0) { - strlcpy(subuf, mqttGroupTopic, 33); + strlcpy(subuf, mqttGroupTopic, MQTT_MAX_TOPIC_LEN + 1); mqtt->subscribe(subuf, 0); strcat_P(subuf, PSTR("/col")); mqtt->subscribe(subuf, 0); - strlcpy(subuf, mqttGroupTopic, 33); + strlcpy(subuf, mqttGroupTopic, MQTT_MAX_TOPIC_LEN + 1); strcat_P(subuf, PSTR("/api")); mqtt->subscribe(subuf, 0); } @@ -158,19 +158,19 @@ void publishMqtt() #ifndef USERMOD_SMARTNEST char s[10]; - char subuf[48]; + char subuf[MQTT_MAX_TOPIC_LEN + 16]; sprintf_P(s, PSTR("%u"), bri); - strlcpy(subuf, mqttDeviceTopic, 33); + strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1); strcat_P(subuf, PSTR("/g")); mqtt->publish(subuf, 0, retainMqttMsg, s); // optionally retain message (#2263) sprintf_P(s, PSTR("#%06X"), (col[3] << 24) | (col[0] << 16) | (col[1] << 8) | (col[2])); - strlcpy(subuf, mqttDeviceTopic, 33); + strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1); strcat_P(subuf, PSTR("/c")); mqtt->publish(subuf, 0, retainMqttMsg, s); // optionally retain message (#2263) - strlcpy(subuf, mqttDeviceTopic, 33); + strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1); strcat_P(subuf, PSTR("/status")); mqtt->publish(subuf, 0, true, "online"); // retain message for a LWT @@ -178,7 +178,7 @@ void publishMqtt() DynamicBuffer buf(1024); bufferPrint pbuf(buf.data(), buf.size()); XML_response(pbuf); - strlcpy(subuf, mqttDeviceTopic, 33); + strlcpy(subuf, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1); strcat_P(subuf, PSTR("/v")); mqtt->publish(subuf, 0, retainMqttMsg, buf.data(), pbuf.size()); // optionally retain message (#2263) #endif @@ -211,7 +211,7 @@ bool initMqtt() if (mqttUser[0] && mqttPass[0]) mqtt->setCredentials(mqttUser, mqttPass); #ifndef USERMOD_SMARTNEST - strlcpy(mqttStatusTopic, mqttDeviceTopic, 33); + strlcpy(mqttStatusTopic, mqttDeviceTopic, MQTT_MAX_TOPIC_LEN + 1); strcat_P(mqttStatusTopic, PSTR("/status")); mqtt->setWill(mqttStatusTopic, 0, true, "offline"); // LWT message #endif diff --git a/wled00/wled.h b/wled00/wled.h index 2b3a77d24..29b43753a 100644 --- a/wled00/wled.h +++ b/wled00/wled.h @@ -483,10 +483,10 @@ WLED_GLOBAL unsigned long lastMqttReconnectAttempt _INIT(0); // used for other #endif WLED_GLOBAL AsyncMqttClient *mqtt _INIT(NULL); WLED_GLOBAL bool mqttEnabled _INIT(false); -WLED_GLOBAL char mqttStatusTopic[40] _INIT(""); // this must be global because of async handlers -WLED_GLOBAL char mqttDeviceTopic[MQTT_MAX_TOPIC_LEN+1] _INIT(""); // main MQTT topic (individual per device, default is wled/mac) -WLED_GLOBAL char mqttGroupTopic[MQTT_MAX_TOPIC_LEN+1] _INIT("wled/all"); // second MQTT topic (for example to group devices) -WLED_GLOBAL char mqttServer[MQTT_MAX_SERVER_LEN+1] _INIT(""); // both domains and IPs should work (no SSL) +WLED_GLOBAL char mqttStatusTopic[MQTT_MAX_TOPIC_LEN + 8] _INIT(""); // this must be global because of async handlers +WLED_GLOBAL char mqttDeviceTopic[MQTT_MAX_TOPIC_LEN + 1] _INIT(""); // main MQTT topic (individual per device, default is wled/mac) +WLED_GLOBAL char mqttGroupTopic[MQTT_MAX_TOPIC_LEN + 1] _INIT("wled/all"); // second MQTT topic (for example to group devices) +WLED_GLOBAL char mqttServer[MQTT_MAX_SERVER_LEN + 1] _INIT(""); // both domains and IPs should work (no SSL) WLED_GLOBAL char mqttUser[41] _INIT(""); // optional: username for MQTT auth WLED_GLOBAL char mqttPass[65] _INIT(""); // optional: password for MQTT auth WLED_GLOBAL char mqttClientID[41] _INIT(""); // override the client ID