fix #35: reject short and zero-timestamp NTP responses, poll interval fix - #47
Conversation
Issue adafruit#35 reports `OverflowError: overflow converting long int to machine word` from NTP.datetime. The datetime rewrite in 784a8b4 was thought to have fixed it. It did not, and it added a second, quieter bug. The failure only exists on a 32-bit board: time.localtime() there converts its argument to a machine word, and CPython's does not, so the crash is invisible under plain CPython. harness.board_time() substitutes a `time` whose localtime() enforces the int32 limit and whose monotonic clock is controllable, which makes the board-only failure deterministic on a laptop with no hardware. Root cause: the library uses one bytearray as both request and response buffer, zeroes it before sending, and discards recv_into's return value. It reads back its own zeros whenever a datagram fails to overwrite offsets 40-43. What that produces depends on where the datagram truncates: bytes received | result ---------------+----------------------------- 0-32 | OverflowError 33-40 | silently returns year 1962 41-43 | silently wrong by days 44-48 | correct The 1962 case is the new bug: datetime averages the server's receive and transmit timestamps, so a valid receive plus a zeroed transmit lands halfway between 1900 and now, which is inside int32 and raises nothing at all. test_real_sockets.py confirms the mechanism outside the fake socketpool, using loopback UDP: truncation, an over-48-byte response being harmless, and, since the socket is never connect()ed, acceptance of a datagram from a source that is not the server. These tests document current behaviour and are expected to fail once the library validates the timestamp; the two xfails asserting what a fix should do turn into XPASS at the same moment. Adds no changes to any existing file.
The library uses one bytearray as both the request and the response buffer,
zeroes it before sending, and discarded recv_into's return value. Any datagram
that failed to overwrite offsets 40-43 was parsed as the client's own zeros,
giving a unix time of -2208988800, which underflows time.localtime() on a
32-bit board. That is the OverflowError in the issue.
Two checks, because one is not enough:
- recv_into's return value is now checked against PACKET_SIZE. This is the
only thing that catches a datagram of 41-43 bytes, where just the low bytes
of the transmit timestamp are zeroed: the year still reads 2024, so no
range check on the timestamp would reject it, but the clock is wrong by
weeks. A response longer than 48 bytes still fills the buffer and reports
48, so NTS and extension-field responses are unaffected.
- A server timestamp earlier than the unix epoch is rejected. That covers a
full-length response carrying a zero or otherwise impossible timestamp, and
keeps the negative value out of the arithmetic entirely.
Both raise ArithmeticError, which _update_time_sync and utc_ns have documented
since 784a8b4 without any code path being able to produce it.
This also fixes the quieter half of the bug. A 33-40 byte datagram carries a
valid receive timestamp and a zeroed transmit timestamp; averaging them landed
around 1962, which is inside int32, so nothing was raised at all and the board
simply ran on a clock sixty years out. utc_ns had the same problem in a worse
form: it never calls localtime(), so it returned -2208988800000000000 with no
error whatsoever.
The tests from the previous commit are inverted to assert the corrected
behaviour, and gain two cases: that a rejected sync does not half-apply, and
that an over-48-byte response is still accepted. Verified against a live NTP
server as well as the fakes.
8e79756 to
83c32a0
Compare
|
So far my testing indicates that this PR fixes the issues from #35 and in a simpler way that the draft I was working on. Both my on hardware (TinyS2) testing of the edge cases from #35 using my separately developed HW test and also in some pytests on my Mac. I also put the library on a few production controllers that use adafruit_ntp and they have not seen any regressions. |
|
As discussed in #35 I'll add a test+fix to deal with the poll clamping. Let me know if there are any other problems- if you don't want the github action/tests or anything, happy to adjust it. |
|
Just to be clear, I'm not an Adafruit approver. I've just been fairly active enhancing and fixing bugs across a bunch of libraries. I am happy to be in the tester/reviewer role for once rather than the fixer role so I'll test things as you tweak. I know in some of my previous PRs Tim (FoamyGuy) actually requested some hw_tests so they could do hardware in the loop testing so I suspect tests will be welcome. Also the GPS library has a bunch of automated tests that check for regressions. |
The poll field is one byte taken straight off the wire and used as 2**poll seconds until the next sync, with no validation. Both ends of the range misbehave. A poll of 255 sets the next sync 2**255 seconds away, roughly 10**69 years, so a single corrupt or hostile byte stops the client re-syncing for good. It still returns a valid-looking time, so nothing downstream notices that the clock has quietly stopped being maintained. A poll of 0 re-queries every second. That is abusive by NTP standards and gets the client rate limited, which is what made adafruit#35 fire as often as it did before cache_seconds landed in adafruit#37. This is not hypothetical: 0.adafruit.pool.ntp.org returns a poll byte of 0, so with the default cache_seconds=0 the library polls it once a second. Clamping to NTP_MINPOLL..NTP_MAXPOLL (2**4 to 2**17 seconds, about 16 seconds to 36 hours) fixes both. cache_seconds is unaffected: it is a floor via max(2**poll, cache_seconds), so a caller asking for longer than the clamped interval still gets it. Five tests, verified to fail without the clamp and pass with it. Found by an on-device validation harness whose poll=255 case reported a next_sync horizon of 57896044618658097711785492504343953926634992332820282019728792003956564819968 seconds.
|
oh gotcha :) I didn't look to see if you're the approver. |
|
Notably:
Going to edit the PR description to include the polling, and let the LLM write on 35 about this discovery. |
tedder
changed the title
Data behind the poll clamp, since "the library asks to be polled once per second" deserves more than one server as evidence. This sends the byte-for-byte packet import socket, time
HOSTS = ["0.adafruit.pool.ntp.org", "1.adafruit.pool.ntp.org", "2.adafruit.pool.ntp.org",
"3.adafruit.pool.ntp.org", "pool.ntp.org", "us.pool.ntp.org", "europe.pool.ntp.org",
"time.google.com", "time.cloudflare.com", "time.apple.com", "time.nist.gov",
"time.windows.com"]
for h in HOSTS:
ip = socket.getaddrinfo(h, 123)[0][4]
p = bytearray(48)
p[0] = 0b00100011 # exactly what adafruit_ntp sends
s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
s.settimeout(5)
s.sendto(p, ip); s.recv_into(p); s.close()
print(f"{h:28s} {ip[0]:16s} stratum={p[1]} poll={p[2]} -> {2**p[2]}s")
time.sleep(1)Results, 2026-08-22: Nine of twelve return poll=0, including three of the four The three that do not are the tell: So this is not servers behaving badly. The library sends 0, gets 0 back, and reads its own zero as an instruction to poll every second. Worth noting for a follow-up: setting a sensible poll value in the request would be the more complete fix, since these servers would then echo something reasonable. That is a behaviour change beyond what #35 needs, so it is not in this PR. |
|
@tedder This version looks good to me. Working without any issues and poll clamp fixes that issue. |
|
Some interesting notes from Claude on the poll 0 from the servers. This nails it — and the answer is the opposite of what you might expect: sending poll = 0 in the request is exactly what the spec calls for, and servers echoing 0 back are also complying. The confusion is that the 4–17 range applies to a different thing than the request field. Let me pull the KoD/backoff best-practices text to ground the last piece, since it connects directly to issue #35. Fetched: https://www.rfc-editor.org/rfc/rfc4330.txt Great line of investigation — and the RFC text turns your observation on its head in a useful way. Short version: the library sending poll = 0 is correct, servers echoing 0 back is also correct, and the 4–17 range you read about doesn't apply to the request at all. The interesting bug is downstream of that. Why poll = 0 in the request is compliant. The 4–17 range is defined only for server messages — the poll field is significant only in SNTP server messages, where it ranges from 4 (16 s) to 17 (~36 h). For a client request, the spec's field table lists Poll as 0, and more broadly a conforming client sets every header field to zero except Mode, VN, and the (optional but recommended) Transmit Timestamp. So zeroing poll isn't just tolerated, it's what RFC 4330 §5 tells you to do. Judging it against the 4–17 range is an apples-to-oranges comparison. Why servers echo 0 (and why some don't). A strict SNTP server is stateless and unicast and manycast servers copy the VN and Poll fields of the request intact into the reply. So a compliant SNTP server must hand you back poll = 0. The servers that return something else are full ntpd-style NTP servers filling in their own poll exponent — also legal, just a different spec surface. The variation you're seeing is exactly the SNTP-echo vs full-NTP-server split. And notably, on the client side the reply's Poll field is marked "ignore" in the client validation table — the spec says a client shouldn't act on it at all. The actual bug this exposes. The Adafruit library does act on the reply's poll — it sets its re-query interval from it: cache_offset_s = max(2poll, self._cache_seconds). With an echoed poll = 0 and the default cache_seconds = 0, that's 20 = 1 second. So if you read .datetime on any regular cadence, the library re-queries the pool roughly once a second. That collides head-on with the good-citizen rules: a client MUST NOT under any conditions use a poll interval less than 15 seconds, and §5 adds that requests should under no conditions be sent at intervals shorter than one minute. And here's the part that ties it back to issue #35: the Kiss-o'-Death mechanism was invented precisely for this failure. The RFC recounts a case where many home/office routers, under an error condition, sent packets every second and caused a dramatic traffic spike. When a pool server decides you're over its rate threshold it returns a RATE KoD — stratum 0, with a four-character kiss code like DENY, RSTR, or RATE. A KoD packet has zeroed timestamp fields… which is one of the exact malformed shapes that produces the overflow. So the poll handling isn't just a separate compliance nit — a client that hammers every second is manufacturing the KoD/zero-timestamp replies that crash it. Plausibly a contributing cause, not a coincidence. On backoff — you're right, it's absent. The spec wants exponential backoff of the poll interval when the server doesn't respond in a reasonable time, and for KoD specifically, on a stratum-0 reply the client should stop sending to that server and switch to an alternate, or if none is available, retry with exponential backoff. The library implements none of that. The raise_on_error=False back-off I added earlier (fixed 60 s on any failed sync) is a crude down-payment on it — and since the patch already rejects stratum-0 replies, a KoD now trips that 60 s back-off rather than poisoning the clock. A fuller version would be exponential and would rotate servers on KoD. One nice corroboration for the validation PR: the guards we added aren't just defensive instinct — they're almost verbatim RFC 4330 §5. Its suggested sanity check discards the reply if the LI, Stratum, or Transmit Timestamp field is 0, or the Mode field isn't 4. That's exactly the LI/stratum/mode/zero-timestamp set. Worth citing in the PR — "these are the spec's own recommended client checks" is a strong argument to a reviewer, and it also tells you the mode != 4 guard you were eyeing as possibly-too-strict is explicitly RFC-endorsed. So for the PR/issue discussion, my suggestions: Don't "fix" poll = 0 in the request, and specifically don't add a guard that rejects a reply with poll < 4 — that would reject every spec-compliant SNTP server echoing your 0. That's the trap hiding in "poll must be 4–17." Treat the re-query interval as a client-side policy, not something derived from the reply. At minimum floor it: max(2**poll, self._cache_seconds, _MIN_POLL_S) with _MIN_POLL_S ≥ 15 (realistically 60+). Better still, per spec, ignore the reply poll entirely and drive the interval from cache_seconds with a sane default minimum. Either way it kills the accidental 1 Hz polling. Two things worth flagging as you weigh it: the "floor the poll interval" change is arguably a more impactful good-citizen fix than the overflow guard itself, but it's also a behavior change (people relying on frequent updates would need to set cache_seconds), so it may deserve its own PR. And the library never sets the request's Transmit Timestamp, so it can't do the §5 replay check (origin timestamp in the reply matching what you sent); it uses local monotonic times for the offset instead, which works but forgoes that bogus-packet protection — a possible future hardening, not urgent. |
tannewt
left a comment
tannewt
left a comment
There was a problem hiding this comment.
One minor comment. Good otherwise. Thanks for the fix and tests!
"issue35" says nothing about what is under test, and the file had grown to
cover two unrelated behaviours:
test_response_validation.py - short, zeroed, and truncated responses are
rejected instead of reaching localtime() or returning a plausible-
looking wrong time.
test_poll_interval.py - the server's poll byte is clamped to the RFC 5905
range before it sets the resync interval.
The issue adafruit#35 link and the description of the underlying defect (one bytearray
reused as request and response buffer, recv_into's return value discarded) move
from the module docstring to a comment above the rejection tests, so the
reference survives the rename. Also drops an unused import and updates the
README table and the cross-reference in test_real_sockets.py.
No test bodies or assertions changed; 30 passed before and after.
|
@tedder I wonder if the poll issue you fixed could have been the cause of the server timeouts in Issue #42. I think perhaps if one queries a NTP server every second because of the 0 poll that was returned before maybe the server will not respond for a while. And a discussion with Claude netted that a NTP server will often send a KoD and then stop sending replies for a while to keep a chatty client from causing a DoS of KoD replies. However I tried the old and new code and still got an ETIMEDOUT from one of the servers even with a 16s poll time. I don't think this affects your fix but I can't recommend closing out Issue #42 yet. |
3 participants
Fixes #35.
Stacked on #46. GitHub will not let a cross-fork PR use a fork branch as its base, so this PR also contains that PR's test commit. Once #46 merges, only the two fix commits remain here.
Net change to the library: +24/-1 in
adafruit_ntp.py.1. Reject short and zero-timestamp responses
The library uses one
bytearrayas both the request and the response buffer, zeroes it before sending, and discardedrecv_into's return value. Any datagram that failed to overwrite offsets 40-43 was parsed as the client's own zeros, giving a unix time of-2208988800, which underflowstime.localtime()on a 32-bit board.Two checks, because one is not enough, and this was settled by experiment rather than argument. Applying only a
seconds == 0guard tomainleft the 41- and 43-byte cases still passing — that is, still producing a silently wrong clock — because only the low bytes of the transmit timestamp are zeroed there and the value is not zero. The length check is the only thing that catches that row.OverflowErrorArithmeticErrorArithmeticErrorArithmeticErrorThe 33-40 row is the quiet half:
_update_time_syncaverages the server's receive and transmit timestamps, so a valid receive plus a zeroed transmit landed halfway between 1900 and now — inside int32, so nothing raised at all.utc_nswas worse in the same way: it never callslocaltime(), so it returned-2208988800000000000with no error whatsoever.A response longer than 48 bytes still fills the buffer and reports 48, so NTS and extension-field replies do not regress into "short read". There is a test for that.
ArithmeticErroris not a new exception type here —_update_time_syncandutc_nshave documented it since 784a8b4 without any code path being able to raise it. This makes the existing docstrings true.2. Clamp the poll interval
The poll field is one byte taken straight off the wire and used as
2**pollseconds until the next sync, unvalidated. Both ends misbehave.255 sets the next sync
2**255seconds away, roughly 10^69 years. A single corrupt or hostile byte stops the client resyncing for good, while still returning a valid-looking time so nothing downstream notices.0 resyncs every second. This is not hypothetical, and it is self-inflicted: the library sends an all-zero request, so the poll field goes out as 0, and pool servers echo it back. Measured against
0.adafruit.pool.ntp.org:So with the default
cache_seconds=0this library polls a public NTP pool once per second. That is abusive by NTP standards, gets the client rate limited, and is the mechanism that made #35 fire as often as it did — details in the issue comment.cache_secondsis unaffected: it remains a floor viamax(2**poll, cache_seconds), so a caller asking for longer than the clamped interval still gets it.Tests
Verified red-to-green for the poll clamp: reverting the single clamp line fails 4 tests, restoring it passes all 30. Verified on 3.9 and 3.13, and against a live server — three syncs against
0.adafruit.pool.ntp.org, 0.055s drift from the system clock.Deliberately out of scope
RFC 5905 section 9.3 also requires rejecting
mode != 4,stratum == 0or> 15, andLI == 3. The library currently accepts all of them, which means it will happily take the timestamp out of a Kiss-o'-Death packet. That is a real bug but a separate one, and it is left out to keep this PR reviewable. Happy to follow up with it if wanted.