Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NTPClient accepts malformed responses #2040

Closed
fooker opened this issue Feb 2, 2020 · 2 comments · Fixed by #2041
Closed

NTPClient accepts malformed responses #2040

fooker opened this issue Feb 2, 2020 · 2 comments · Fixed by #2041

Comments

@fooker
Copy link
Contributor

fooker commented Feb 2, 2020

It looks like NTPClient is accepting malformed packages. Sometimes we receive NTP responses which are interpreted as 2036-02-07T06:28:16Z (which is the uint32 NTP rollover date).

It looks like the the received packet contains a all-zero timestamp which is processed as a valid packet. I've tried to capture these invalid packets on the used gateway but failed to do so - therefore I'm not sure if it's either a bogus response, an unsupported protocol version, an transition error on the WiFi or an firmware bug. Nevertheless, other libraries have seen these problems, too. They just add a check for timestamp == 0 to circumvent this (see gmag11/NtpClient#77).

Do you think adding such a check is the right way to fix this, or is it worth investigating whats the cause for this malformed packets?

@mikee47
Copy link
Contributor

mikee47 commented Feb 2, 2020

This comments suggests that zero timestamps are a legitimate response gmag11/NtpClient#70 (comment).

I just found the following in RFC 958 / Network Time Protocol:

In some cases a particular timestamp may not be available, such as
when the protocol first starts up. In these cases the 64-bit field
is set to zero, indicating the value is invalid or undefined.

So I'd say that even if you do get a response, a 0-timestamp can occur and needs to be handled.

@fooker
Copy link
Contributor Author

fooker commented Feb 2, 2020

@mikee47 I think so, too. I will come up with a PR after some testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants