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

Origin Timestamp as Transaction ID #193

Closed
davidv1992 opened this issue Jul 7, 2022 · 4 comments
Closed

Origin Timestamp as Transaction ID #193

davidv1992 opened this issue Jul 7, 2022 · 4 comments

Comments

@davidv1992
Copy link
Member

reported by jsha

The origin timestamp in outbound packets is used to identify reply packets. This is a good measure against off-path attackers, similar to how DNS uses source port randomization in addition to query id to make it hard for spoofed packets to interfere with operations.

In ntp-proto/src/peer.rs, the next_expected_origin is kept live until either (a) a packet with that timestamp is accepted, or (b) the next poll interval arrives. Since the poll interval can be quite long at the maximum (many hours), this gives a long window for attackers to send packets guessing origin timestamp values. I recommend having a maximum time (on the order of seconds) past which reply packets won't be accepted, independent of the poll interval.

I recommend adding a small random delay to the poll time to make it harder for an attacker to guess when a poll request is inflight. This also has the happy operational effect of preventing a situation where a fleet of clients all make their poll requests at the same time.

Handle_incoming checks for a match with next_expected_origin before checking whether a packet is a Kiss-O'-Death (KoD) packet. The RFC says:

The Receive Timestamp and the Transmit Timestamp (set by the server)
are undefined when in a KoD packet and MUST NOT be relied upon to
have valid values and MUST be discarded.

This suggests KISS checks should come before the next_expected_origin checks and should not be able to redeem next_expected origin. The MUST discard suggests these fields should be cleared during packet parsing.

The downside of following the spec here is that KISS packets can be easily forged by off-path attackers if they are not validated against next_expected_origin. In practice, do common NTP deployments copy the transmit timestamp? I suspect there may be some deployments where KISS packets are generated by a firewall without copying data from the inbound packet.

@davidv1992 davidv1992 added this to the Security review issues milestone Jul 7, 2022
@davidv1992
Copy link
Member Author

On checking for next_expected_origin before handling KoD packets, the RFC specifically states that the receive and transmit timestamps cannot be trusted. Since the next_expected_origin is matched against the origin timestamp, this should still be within spec, although it is a bit unclear if this is really the case. Given the ease with which KISS packets could otherwise be forged, I suggest we keep our current aproach for that.

@davidv1992
Copy link
Member Author

davidv1992 commented Jul 7, 2022

@jsha
Copy link

jsha commented Jul 7, 2022

he RFC specifically states that the receive and transmit timestamps cannot be trusted. Since the next_expected_origin is matched against the origin timestamp, this should still be within spec, although it is a bit unclear if this is really the case.

I made an error in reading the spec - I was confusing the Transmit Timestamp with the Origin Timestamp and believed the spec was saying that the Origin Timestamp comparison could not be used for KoD packets. But on re-reading it I see that's not the case, and Origin Timestamp is still expected to be set even in KoD packets. So I believe the current approach is good.

@davidv1992
Copy link
Member Author

With the changes made, this should now be fixed.

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

No branches or pull requests

2 participants