Add security mechanisms against replay attacks

[aggarw13]

This PR adds mechanisms for protecting against replay attacks of either SNTP request packets from client OR SNTP response packets from server. Protection against replay attack is achieved by:

1. Clearing the lastRequestTime member of SntpContext_tafter a valid SNTP response (either accepted or rejected) is received from a server so that anyfurther packets from the server generated as a result of the original client request packet being replayed are rejected. (When client request packets are replayed, server genuinely may respond to the repeated request packets as NTP/SNTP servers are sstateless.) By clearing the state of lastRequestTime, the server response(s) to replayed packet(s) will be discarded because the "originate timestamp" in them will not match the lastRequestTime anymore.

2. Not accepting server responses that use zero timestamp value for "originate timestamp" (or T1), "receive timestamp" (or T2) OR "transmit timestamp" (or T3). This is to primarily protect an abuse of 1.s functionality of clearing the lastRequestTime so that an off-path attacker (i.e. an attacker who cannot see client request packet) does not spoof a server response with zero as the "originate timestamp",thereby, matching the cleared lastRequestTime state of client context, and thus, incorrectly causing the client to accept the spoofed server response.

Note: The above mechanisms also protects attack of server response packet being replayed or duplicated as the client clears/resets the lastRequestTime state after receiving the first server response, thereby, making the "originate timestamp" of the duplicated/replayed server response packet invalid for the client.

[paulbartell]

Could you clarify the reasoning as to why this condition is (status == SntpSuccess) || ( status == SntpRejectedResponse ) rather than just (status == SntpSuccess)? Seems like this could lead to a denial of service issue.

[paulbartell]

This is only really a potential issue if server authentication is disabled.

[aggarw13]

The SntpRejectedResponse represents the state of the library when it has determined from parsing the response packet after performing the sanity checks against duplicated or replay attacks that the server rejected the time request with a Kiss-o'-Death packet. Such a state represents that the received packet passes all the necessary checks for a valid server response, and the server actually rejected the time request.

A Denial of service attack can take place be achieved if an attacker is successfully able to spoof a server response (for example, if an "in-path" attacker modifies a genuine server response, containing server time, to inject a rejection message). However, the mitigation against that is to use security mechanism for server authentication on client side with the authentication keys being uncompromised.

[paulbartell]

I would suggest changing line 690 to something like:

if( ( status == SntpSuccess ) || 
    ( ( pContext->authIntf.validateServerAuth != NULL ) && ( status == SntpRejectedResponse ) ) )

to better protect against denial of service.

[aggarw13]

By limiting the functionality of clearing the lastRequestTime to only clients that authenticate the server, the door of replay attack will remain open for clients that do not authenticate server. which has a higher cost of allowing an "on-path" attacker to inject incorrect time in system.

For a denial of service attack, the cost to the system is relatively lower as it does not allow the attacker to inject incorrect time. However, for a DoS attack, if the attacker is in between the client and server path (like "man-in-the-middle"), then the attacker can launch more serious attacks like injecting incorrect time for clients that do not authenticate the server.

In non-authenticated communication, both DoS and incorrect time injection attacks can be launched by both "on-path" and "man-in-the-middle" attackers by spoofing server response; which is orthogonal to the functionality of clearing the lastRequestTime state. However, by clearing the lastRequestTime, the possibility of an "on-path" attackers spoofing server response is lowered as they have to compete with the genuine server to send a response packet back to the client (as the client will only accept the first response packet received and discard subsequent ones as the subsequent will have "originate" timestamp not matching lastRequestTime anymore).

Thus, I don't think that removing this functionality for non-authenticated communication will improve the security story for such clients.

[paulbartell]

My primary assumption in making the opposite tradeoff decision is that an "on-path" attacker is likely able to generate response packets and return them to the client faster than the actual server.

My second assumption is that there is not a simple "off-path" exploit unless udp source port randomization is "not to random".

This implies that the "on-path" attacker could choose whether to

1. inject incorrect time or
2. cause denial of service via a forged kiss of death packet

We can't really mitigate (1), but we can mitigate (2).

[aggarw13]

Okay, with the primary assumption that an "on-path" (or even "man-in-the-middle") attacker can generate a spoof response packet faster than the server to cause a denial-of-service attack on the client, I'm good with making your suggested change.

[paulbartell]

I would suggest setting each member of SntpTimestamp_t here directly rather than using memset.

[aggarw13]

For readability purposes or something more than that?

[paulbartell]

Mostly for readability.

Using memset is less that ideal since future code changes could modify the type of pContext->lastRequestTime, but this line would not cause a compiler error unless the definition of SntpTimestamp_t is removed as well.