nts: improve NTS-NTP server/client code

Add more comments, assertions, debug messages, and other minor
changes to make the code more robust.

nts_ntp_auth.c

@@ -102,6 +102,10 @@ NNA_GenerateAuthEF(NTP_Packet *packet, NTP_PacketInfo *info, SIV_Instance siv,
   body = (unsigned char *)(header + 1);
   ciphertext = body + nonce_length + nonce_padding;
 
+  if ((unsigned char *)header + auth_length !=
+      ciphertext + ciphertext_length + ciphertext_padding + additional_padding)
+    assert(0);
+
   memcpy(body, nonce, nonce_length);
   memset(body + nonce_length, 0, nonce_padding);
 
@@ -135,7 +139,7 @@ NNA_DecryptAuthEF(NTP_Packet *packet, NTP_PacketInfo *info, SIV_Instance siv, in
                       NULL, &ef_type, &ef_body, &ef_body_length))
     return 0;
 
-  if (ef_type != NTP_EF_NTS_AUTH_AND_EEF)
+  if (ef_type != NTP_EF_NTS_AUTH_AND_EEF || ef_body_length < sizeof (*header))
     return 0;
 
   header = ef_body;
@@ -148,7 +152,7 @@ NNA_DecryptAuthEF(NTP_Packet *packet, NTP_PacketInfo *info, SIV_Instance siv, in
     return 0;
 
   nonce = (unsigned char *)(header + 1);
-  ciphertext = (unsigned char *)(header + 1) + get_padded_length(nonce_length);
+  ciphertext = nonce + get_padded_length(nonce_length);
 
   siv_tag_length = SIV_GetTagLength(siv);
 
@@ -166,6 +170,7 @@ NNA_DecryptAuthEF(NTP_Packet *packet, NTP_PacketInfo *info, SIV_Instance siv, in
   }
 
   *plaintext_length = ciphertext_length - siv_tag_length;
+  assert(*plaintext_length >= 0);
 
   if (!SIV_Decrypt(siv, nonce, nonce_length, packet, ef_start,
                    ciphertext, ciphertext_length, plaintext, *plaintext_length)) {

nts_ntp_client.c

@@ -43,8 +43,10 @@
 #include "siv.h"
 #include "util.h"
 
+/* Maximum length of all cookies to avoid IP fragmentation */
 #define MAX_TOTAL_COOKIE_LENGTH (8 * 108)
 
+/* Magic string of files containing keys and cookies */
 #define DUMP_IDENTIFIER "NNC0\n"
 
 struct NNC_Instance_Record {
@@ -87,6 +89,7 @@ reset_instance(NNC_Instance inst)
 
   memset(&inst->context, 0, sizeof (inst->context));
   inst->context_id = 0;
+  memset(inst->cookies, 0, sizeof (inst->cookies));
   inst->num_cookies = 0;
   inst->cookie_index = 0;
   inst->nak_response = 0;
@@ -136,17 +139,15 @@ NNC_DestroyInstance(NNC_Instance inst)
 static int
 check_cookies(NNC_Instance inst)
 {
-  /* Force NKE if a NAK was received since last valid auth */
+  /* Force a new NTS-KE session if a NAK was received without a valid response,
-  if (inst->nak_response && !inst->ok_response && inst->num_cookies > 0) {
+     or the keys encrypting the cookies need to be refreshed */
+  if (inst->num_cookies > 0 &&
+      ((inst->nak_response && !inst->ok_response) ||
+       SCH_GetLastEventMonoTime() - inst->last_nke_success > CNF_GetNtsRefresh())) {
     inst->num_cookies = 0;
     DEBUG_LOG("Dropped cookies");
   }
 
-  /* Force NKE if the keys encrypting the cookies are too old */
-  if (inst->num_cookies > 0 &&
-      SCH_GetLastEventMonoTime() - inst->last_nke_success > CNF_GetNtsRefresh())
-    inst->num_cookies = 0;
-
   return inst->num_cookies > 0;
 }
 
@@ -203,10 +204,11 @@ get_cookies(NNC_Instance inst)
   double now;
   int got_data;
 
-  assert(!check_cookies(inst));
+  assert(inst->num_cookies == 0);
 
   now = SCH_GetLastEventMonoTime();
 
+  /* Create and start a new NTS-KE session if not already present */
   if (!inst->nke) {
     if (now < inst->next_nke_attempt) {
       DEBUG_LOG("Limiting NTS-KE request rate (%f seconds)",
@@ -231,9 +233,13 @@ get_cookies(NNC_Instance inst)
 
   update_next_nke_attempt(inst, now);
 
+  /* Wait until the session stops */
   if (NKC_IsActive(inst->nke))
     return 0;
 
+  assert(sizeof (inst->cookies) / sizeof (inst->cookies[0]) == NTS_MAX_COOKIES);
+
+  /* Get the new keys, cookies and NTP address if the session was successful */
   got_data = NKC_GetNtsData(inst->nke, &inst->context,
                             inst->cookies, &inst->num_cookies, NTS_MAX_COOKIES,
                             &ntp_address);
@@ -250,17 +256,18 @@ get_cookies(NNC_Instance inst)
 
   inst->context_id++;
 
+  /* Force a new session if the NTP address is used by another source, with
+     an expectation that it will eventually get a non-conflicting address */
   if (!set_ntp_address(inst, &ntp_address)) {
     inst->num_cookies = 0;
     return 0;
   }
 
+  inst->last_nke_success = now;
   inst->cookie_index = 0;
 
   inst->nak_response = 0;
 
-  inst->last_nke_success = now;
-
   return 1;
 }
 
@@ -269,11 +276,13 @@ get_cookies(NNC_Instance inst)
 int
 NNC_PrepareForAuth(NNC_Instance inst)
 {
+  /* Try to reload saved keys and cookies (once for the NTS-KE address) */
   if (!inst->load_attempt) {
     load_cookies(inst);
     inst->load_attempt = 1;
   }
 
+  /* Get new cookies if there are not any, or they are no longer usable */
   if (!check_cookies(inst)) {
     if (!get_cookies(inst))
       return 0;
@@ -288,8 +297,9 @@ NNC_PrepareForAuth(NNC_Instance inst)
     return 0;
   }
 
-  UTI_GetRandomBytes(&inst->uniq_id, sizeof (inst->uniq_id));
+  /* Prepare data for NNC_GenerateRequestAuth() */
-  UTI_GetRandomBytes(&inst->nonce, sizeof (inst->nonce));
+  UTI_GetRandomBytes(inst->uniq_id, sizeof (inst->uniq_id));
+  UTI_GetRandomBytes(inst->nonce, sizeof (inst->nonce));
 
   return 1;
 }
@@ -315,7 +325,7 @@ NNC_GenerateRequestAuth(NNC_Instance inst, NTP_Packet *packet,
                     MAX_TOTAL_COOKIE_LENGTH / (cookie->length + 4));
 
   if (!NEF_AddField(packet, info, NTP_EF_NTS_UNIQUE_IDENTIFIER,
-                    &inst->uniq_id, sizeof (inst->uniq_id)))
+                    inst->uniq_id, sizeof (inst->uniq_id)))
     return 0;
 
   if (!NEF_AddField(packet, info, NTP_EF_NTS_COOKIE,
@@ -372,6 +382,9 @@ extract_cookies(NNC_Instance inst, unsigned char *plaintext, int length)
       continue;
 
     index = (inst->cookie_index + inst->num_cookies) % NTS_MAX_COOKIES;
+    assert(index >= 0 && index < NTS_MAX_COOKIES);
+    assert(sizeof (inst->cookies) / sizeof (inst->cookies[0]) == NTS_MAX_COOKIES);
+
     memcpy(inst->cookies[index].cookie, ef_body, ef_body_length);
     inst->cookies[index].length = ef_body_length;
     inst->num_cookies++;
@@ -398,7 +411,7 @@ NNC_CheckResponseAuth(NNC_Instance inst, NTP_Packet *packet,
   if (info->ext_fields == 0 || info->mode != MODE_SERVER)
     return 0;
 
-  /* Accept only one response per request */
+  /* Accept at most one response per request */
   if (inst->ok_response)
     return 0;
 
@@ -411,7 +424,8 @@ NNC_CheckResponseAuth(NNC_Instance inst, NTP_Packet *packet,
   for (parsed = NTP_HEADER_LENGTH; parsed < info->length; parsed += ef_length) {
     if (!NEF_ParseField(packet, info->length, parsed,
                         &ef_length, &ef_type, &ef_body, &ef_body_length))
-      break;
+      /* This is not expected as the packet already passed NAU_ParsePacket() */
+      return 0;
 
     switch (ef_type) {
       case NTP_EF_NTS_UNIQUE_IDENTIFIER:

nts_ntp_server.c

@@ -112,15 +112,18 @@ NNS_CheckRequestAuth(NTP_Packet *packet, NTP_PacketInfo *info, uint32_t *kod)
   for (parsed = NTP_HEADER_LENGTH; parsed < info->length; parsed += ef_length) {
     if (!NEF_ParseField(packet, info->length, parsed,
                         &ef_length, &ef_type, &ef_body, &ef_body_length))
-      break;
+      /* This is not expected as the packet already passed NAU_ParsePacket() */
+      return 0;
 
     switch (ef_type) {
       case NTP_EF_NTS_UNIQUE_IDENTIFIER:
         has_uniq_id = 1;
         break;
       case NTP_EF_NTS_COOKIE:
-        if (has_cookie || ef_body_length > sizeof (cookie.cookie))
+        if (has_cookie || ef_body_length > sizeof (cookie.cookie)) {
+          DEBUG_LOG("Unexpected cookie/length");
           return 0;
+        }
         cookie.length = ef_body_length;
         memcpy(cookie.cookie, ef_body, ef_body_length);
         has_cookie = 1;
@@ -199,13 +202,18 @@ NNS_CheckRequestAuth(NTP_Packet *packet, NTP_PacketInfo *info, uint32_t *kod)
     return 0;
   }
 
+  /* Prepare data for NNS_GenerateResponseAuth() to minimise the time spent
+     there (when the TX timestamp is already set) */
+
   UTI_GetRandomBytes(server->nonce, sizeof (server->nonce));
 
-  server->num_cookies = MIN(NTS_MAX_COOKIES, requested_cookies);
+  assert(sizeof (server->cookies) / sizeof (server->cookies[0]) == NTS_MAX_COOKIES);
-  for (i = 0; i < server->num_cookies; i++)
+  for (i = 0; i < NTS_MAX_COOKIES && i < requested_cookies; i++)
     if (!NKS_GenerateCookie(&context, &server->cookies[i]))
       return 0;
 
+  server->num_cookies = i;
+
   return 1;
 }
 
@@ -224,14 +232,16 @@ NNS_GenerateResponseAuth(NTP_Packet *request, NTP_PacketInfo *req_info,
   if (!server || req_info->mode != MODE_CLIENT || res_info->mode != MODE_SERVER)
     return 0;
 
-  /* Make sure this is a response to the expected request */
+  /* Make sure this is a response to the request from the last call
+     of NNS_CheckRequestAuth() */
   if (UTI_CompareNtp64(&server->req_tx, &request->transmit_ts) != 0)
     assert(0);
 
   for (parsed = NTP_HEADER_LENGTH; parsed < req_info->length; parsed += ef_length) {
     if (!NEF_ParseField(request, req_info->length, parsed,
                         &ef_length, &ef_type, &ef_body, &ef_body_length))
-      break;
+      /* This is not expected as the packet already passed NAU_ParsePacket() */
+      return 0;
 
     switch (ef_type) {
       case NTP_EF_NTS_UNIQUE_IDENTIFIER:
@@ -249,7 +259,7 @@ NNS_GenerateResponseAuth(NTP_Packet *request, NTP_PacketInfo *req_info,
 
   for (i = 0, plaintext_length = 0; i < server->num_cookies; i++) {
     if (!NEF_SetField(plaintext, sizeof (plaintext), plaintext_length,
-                      NTP_EF_NTS_COOKIE, &server->cookies[i].cookie,
+                      NTP_EF_NTS_COOKIE, server->cookies[i].cookie,
                       server->cookies[i].length, &ef_length))
       return 0;
 
@@ -259,6 +269,8 @@ NNS_GenerateResponseAuth(NTP_Packet *request, NTP_PacketInfo *req_info,
 
   server->num_cookies = 0;
 
+  /* Generate an authenticator field which will make the length
+     of the response equal to the length of the request */
   if (!NNA_GenerateAuthEF(response, res_info, server->siv,
                           server->nonce, sizeof (server->nonce),
                           plaintext, plaintext_length,