From 1e29d9463e23d0a9ef2841eb3b92d69a0881ba60 Mon Sep 17 00:00:00 2001 From: Frank <91616163+softhack007@users.noreply.github.com> Date: Mon, 20 Nov 2023 12:24:34 +0100 Subject: [PATCH 1/4] validate NTP responses (fixes #3515) * purge old (not yet processes) NTP responses * validate server responses before updating WLED time * purge receive buffer when package is rejected (avoids mem leak on esp32) --- wled00/const.h | 3 ++- wled00/ntp.cpp | 27 ++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/wled00/const.h b/wled00/const.h index 25fe43773..f1293fd95 100644 --- a/wled00/const.h +++ b/wled00/const.h @@ -375,7 +375,8 @@ #define SUBPAGE_JS 254 #define SUBPAGE_WELCOME 255 -#define NTP_PACKET_SIZE 48 +#define NTP_PACKET_SIZE 48 // size of NTP recive buffer +#define NTP_MIN_PACKET_SIZE 48 // min expected size - NTP v4 allows for "extended information" appended to the standard fields //maximum number of rendered LEDs - this does not have to match max. physical LEDs, e.g. if there are virtual busses #ifndef MAX_LEDS diff --git a/wled00/ntp.cpp b/wled00/ntp.cpp index c9c4874b9..85bdcc7f8 100644 --- a/wled00/ntp.cpp +++ b/wled00/ntp.cpp @@ -199,6 +199,9 @@ void handleNetworkTime() { if (millis() - ntpPacketSentTime > 10000) { + #ifdef ARDUINO_ARCH_ESP32 // I had problems using udp.flush() on 8266 + while (ntpUdp.parsePacket() > 0) ntpUdp.flush(); // flush any existing packets + #endif sendNTPPacket(); ntpPacketSentTime = millis(); } @@ -239,16 +242,38 @@ void sendNTPPacket() ntpUdp.endPacket(); } +static bool isValidNtpResponse(byte * ntpPacket) { + // Perform a few validity checks on the packet + // based on https://github.com/taranais/NTPClient/blob/master/NTPClient.cpp + if((ntpPacket[0] & 0b11000000) == 0b11000000) return false; //reject LI=UNSYNC + // if((ntpPacket[0] & 0b00111000) >> 3 < 0b100) return false; //reject Version < 4 + if((ntpPacket[0] & 0b00000111) != 0b100) return false; //reject Mode == Server + if((ntpPacket[1] < 1) || (ntpPacket[1] > 15)) return false; //reject invalid Stratum + if( ntpPacket[16] == 0 && ntpPacket[17] == 0 && + ntpPacket[18] == 0 && ntpPacket[19] == 0 && + ntpPacket[20] == 0 && ntpPacket[21] == 0 && + ntpPacket[22] == 0 && ntpPacket[23] == 0) //reject ReferenceTimestamp == 0 + return false; + + return true; +} + bool checkNTPResponse() { int cb = ntpUdp.parsePacket(); - if (!cb) return false; + if (cb < NTP_MIN_PACKET_SIZE) { + #ifdef ARDUINO_ARCH_ESP32 // I had problems using udp.flush() on 8266 + if (cb > 0) ntpUdp.flush(); // this avoids memory leaks on esp32 + #endif + return false; + } uint32_t ntpPacketReceivedTime = millis(); DEBUG_PRINT(F("NTP recv, l=")); DEBUG_PRINTLN(cb); byte pbuf[NTP_PACKET_SIZE]; ntpUdp.read(pbuf, NTP_PACKET_SIZE); // read the packet into the buffer + if (!isValidNtpResponse(pbuf)) return false; // verify we have a valid response to client Toki::Time arrived = toki.fromNTP(pbuf + 32); Toki::Time departed = toki.fromNTP(pbuf + 40); From 80c67d37c05988b21277f78b4bb7b84c40b20153 Mon Sep 17 00:00:00 2001 From: Frank <91616163+softhack007@users.noreply.github.com> Date: Mon, 20 Nov 2023 15:13:39 +0100 Subject: [PATCH 2/4] bufix: ntp query when NTP gets enabled from UI when NTP got enabled via UI, WLED would wait up to 12 hours before issuing the first NTP request. --- wled00/ntp.cpp | 2 +- wled00/set.cpp | 2 +- wled00/wled.cpp | 2 +- wled00/wled.h | 5 +++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/wled00/ntp.cpp b/wled00/ntp.cpp index 85bdcc7f8..e7dbf7439 100644 --- a/wled00/ntp.cpp +++ b/wled00/ntp.cpp @@ -247,7 +247,7 @@ static bool isValidNtpResponse(byte * ntpPacket) { // based on https://github.com/taranais/NTPClient/blob/master/NTPClient.cpp if((ntpPacket[0] & 0b11000000) == 0b11000000) return false; //reject LI=UNSYNC // if((ntpPacket[0] & 0b00111000) >> 3 < 0b100) return false; //reject Version < 4 - if((ntpPacket[0] & 0b00000111) != 0b100) return false; //reject Mode == Server + if((ntpPacket[0] & 0b00000111) != 0b100) return false; //reject Mode != Server if((ntpPacket[1] < 1) || (ntpPacket[1] > 15)) return false; //reject invalid Stratum if( ntpPacket[16] == 0 && ntpPacket[17] == 0 && ntpPacket[18] == 0 && ntpPacket[19] == 0 && diff --git a/wled00/set.cpp b/wled00/set.cpp index 01ebf5db6..efc0813c0 100644 --- a/wled00/set.cpp +++ b/wled00/set.cpp @@ -396,7 +396,7 @@ void handleSettingsSet(AsyncWebServerRequest *request, byte subPage) //start ntp if not already connected if (ntpEnabled && WLED_CONNECTED && !ntpConnected) ntpConnected = ntpUdp.begin(ntpLocalPort); - ntpLastSyncTime = 0; // force new NTP query + ntpLastSyncTime = NTP_NEVER; // force new NTP query longitude = request->arg(F("LN")).toFloat(); latitude = request->arg(F("LT")).toFloat(); diff --git a/wled00/wled.cpp b/wled00/wled.cpp index a527f2967..bf1ab2256 100644 --- a/wled00/wled.cpp +++ b/wled00/wled.cpp @@ -133,7 +133,7 @@ void WLED::loop() if (lastMqttReconnectAttempt > millis()) { rolloverMillis++; lastMqttReconnectAttempt = 0; - ntpLastSyncTime = 0; + ntpLastSyncTime = NTP_NEVER; // force new NTP query strip.restartRuntime(); } if (millis() - lastMqttReconnectAttempt > 30000 || lastMqttReconnectAttempt == 0) { // lastMqttReconnectAttempt==0 forces immediate broadcast diff --git a/wled00/wled.h b/wled00/wled.h index 8a9a38d2b..2572c73fc 100644 --- a/wled00/wled.h +++ b/wled00/wled.h @@ -644,10 +644,11 @@ WLED_GLOBAL String escapedMac; WLED_GLOBAL DNSServer dnsServer; // network time +#define NTP_NEVER 999000000L WLED_GLOBAL bool ntpConnected _INIT(false); WLED_GLOBAL time_t localTime _INIT(0); -WLED_GLOBAL unsigned long ntpLastSyncTime _INIT(999000000L); -WLED_GLOBAL unsigned long ntpPacketSentTime _INIT(999000000L); +WLED_GLOBAL unsigned long ntpLastSyncTime _INIT(NTP_NEVER); +WLED_GLOBAL unsigned long ntpPacketSentTime _INIT(NTP_NEVER); WLED_GLOBAL IPAddress ntpServerIP; WLED_GLOBAL uint16_t ntpLocalPort _INIT(2390); WLED_GLOBAL uint16_t rolloverMillis _INIT(0); From d56cefdc7a443b40af9a1cb302e5c8fe99acfc23 Mon Sep 17 00:00:00 2001 From: Frank <91616163+softhack007@users.noreply.github.com> Date: Mon, 20 Nov 2023 19:20:18 +0100 Subject: [PATCH 3/4] indentation fix --- wled00/ntp.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wled00/ntp.cpp b/wled00/ntp.cpp index e7dbf7439..e4442f5dd 100644 --- a/wled00/ntp.cpp +++ b/wled00/ntp.cpp @@ -253,9 +253,9 @@ static bool isValidNtpResponse(byte * ntpPacket) { ntpPacket[18] == 0 && ntpPacket[19] == 0 && ntpPacket[20] == 0 && ntpPacket[21] == 0 && ntpPacket[22] == 0 && ntpPacket[23] == 0) //reject ReferenceTimestamp == 0 - return false; + return false; - return true; + return true; } bool checkNTPResponse() From 5fb891c495c395f0dd4c984416e4d9f484dd7fe2 Mon Sep 17 00:00:00 2001 From: Frank <91616163+softhack007@users.noreply.github.com> Date: Mon, 20 Nov 2023 19:24:04 +0100 Subject: [PATCH 4/4] indentation fix2 --- wled00/ntp.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wled00/ntp.cpp b/wled00/ntp.cpp index e4442f5dd..3d15c8c25 100644 --- a/wled00/ntp.cpp +++ b/wled00/ntp.cpp @@ -249,7 +249,7 @@ static bool isValidNtpResponse(byte * ntpPacket) { // if((ntpPacket[0] & 0b00111000) >> 3 < 0b100) return false; //reject Version < 4 if((ntpPacket[0] & 0b00000111) != 0b100) return false; //reject Mode != Server if((ntpPacket[1] < 1) || (ntpPacket[1] > 15)) return false; //reject invalid Stratum - if( ntpPacket[16] == 0 && ntpPacket[17] == 0 && + if( ntpPacket[16] == 0 && ntpPacket[17] == 0 && ntpPacket[18] == 0 && ntpPacket[19] == 0 && ntpPacket[20] == 0 && ntpPacket[21] == 0 && ntpPacket[22] == 0 && ntpPacket[23] == 0) //reject ReferenceTimestamp == 0