Merge pull request #3536 from Aircoookie/ntp_errorchecking

NTP validation, and rejecting malformed responses (related to #3515)
This commit is contained in:
Frank 2023-11-20 21:13:35 +01:00 committed by GitHub
commit 32e724e744
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 33 additions and 6 deletions

View File

@ -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

View File

@ -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);

View File

@ -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();

View File

@ -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

View File

@ -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);