ntp: move initial packet parsing from ntp_auth to ntp_core

Since commit fdfcabd79bd3 ("ntp: drop support for long NTPv4 MACs"), the
parser doesn't need to check validify of MACs in NTPv4 packets to
distinguish them from extension fields. Move the parser to ntp_core to
avoid having a separate iteration looking for non-authentication
extension fields.

diff --git a/ntp.h b/ntp.h
index 38f3e82cecfdf38a9d066b94e275020e754aa923..fe2dd0d21b34b8c6b1b303d183c6510de4fa4b6f 100644 (file)
--- a/ntp.h
+++ b/ntp.h
@@ -113,6 +113,13 @@ typedef struct {
 #define NTP_REFID_LOCAL 0x7F7F0101UL /* 127.127.1.1 */
 #define NTP_REFID_SMOOTH 0x7F7F01FFUL /* 127.127.1.255 */
 
+/* Authentication extension fields */
+
+#define NTP_EF_NTS_UNIQUE_IDENTIFIER    0x0104
+#define NTP_EF_NTS_COOKIE               0x0204
+#define NTP_EF_NTS_COOKIE_PLACEHOLDER   0x0304
+#define NTP_EF_NTS_AUTH_AND_EEF         0x0404
+
 /* Enumeration for authentication modes of NTP packets */
 typedef enum {
   NTP_AUTH_NONE = 0,            /* No authentication */

diff --git a/ntp_auth.c b/ntp_auth.c
index 67d28b663f08df085d644043c6ac048d03e905fb..58374c57baef5d91fa876af7e5318483dd33ca1e 100644 (file)
--- a/ntp_auth.c
+++ b/ntp_auth.c
@@ -32,7 +32,6 @@
 #include "logging.h"
 #include "memory.h"
 #include "ntp_auth.h"
-#include "ntp_ext.h"
 #include "ntp_signd.h"
 #include "nts_ntp.h"
 #include "nts_ntp_client.h"
@@ -105,19 +104,6 @@ check_symmetric_auth(NTP_Packet *packet, NTP_PacketInfo *info)
 
 /* ================================================== */
 
-static int
-is_zero_data(unsigned char *data, int length)
-{
-  int i;
-
-  for (i = 0; i < length; i++)
-    if (data[i] != 0)
-      return 0;
-  return 1;
-}
-
-/* ================================================== */
-
 static NAU_Instance
 create_instance(NTP_AuthMode mode)
 {
@@ -247,101 +233,6 @@ NAU_GenerateRequestAuth(NAU_Instance instance, NTP_Packet *request, NTP_PacketIn
 
 /* ================================================== */
 
-int
-NAU_ParsePacket(NTP_Packet *packet, NTP_PacketInfo *info)
-{
-  int parsed, remainder, ef_length, ef_type;
-  unsigned char *data;
-
-  data = (void *)packet;
-  parsed = NTP_HEADER_LENGTH;
-  remainder = info->length - parsed;
-
-  info->ext_fields = 0;
-
-  /* Check if this is a plain NTP packet with no extension fields or MAC */
-  if (remainder <= 0)
-    return 1;
-
-  assert(remainder % 4 == 0);
-
-  /* In NTPv3 and older packets don't have extension fields.  Anything after
-     the header is assumed to be a MAC. */
-  if (info->version <= 3) {
-    info->auth.mode = NTP_AUTH_SYMMETRIC;
-    info->auth.mac.start = parsed;
-    info->auth.mac.length = remainder;
-    info->auth.mac.key_id = ntohl(*(uint32_t *)(data + parsed));
-
-    /* Check if it is an MS-SNTP authenticator field or extended authenticator
-       field with zeroes as digest */
-    if (info->version == 3 && info->auth.mac.key_id != 0) {
-      if (remainder == 20 && is_zero_data(data + parsed + 4, remainder - 4))
-        info->auth.mode = NTP_AUTH_MSSNTP;
-      else if (remainder == 72 && is_zero_data(data + parsed + 8, remainder - 8))
-        info->auth.mode = NTP_AUTH_MSSNTP_EXT;
-    }
-
-    return 1;
-  }
-
-  /* Check for a crypto NAK */
-  if (remainder == 4 && ntohl(*(uint32_t *)(data + parsed)) == 0) {
-    info->auth.mode = NTP_AUTH_SYMMETRIC;
-    info->auth.mac.start = parsed;
-    info->auth.mac.length = remainder;
-    info->auth.mac.key_id = 0;
-    return 1;
-  }
-
-  /* Parse the rest of the NTPv4 packet */
-
-  while (remainder > 0) {
-    /* Check if the remaining data is a MAC */
-    if (remainder >= NTP_MIN_MAC_LENGTH && remainder <= NTP_MAX_V4_MAC_LENGTH)
-      break;
-
-    /* Check if this is a valid NTPv4 extension field and skip it */
-    if (!NEF_ParseField(packet, info->length, parsed, &ef_length, &ef_type, NULL, NULL)) {
-      DEBUG_LOG("Invalid format");
-      return 0;
-    }
-
-    assert(ef_length > 0 && ef_length % 4 == 0);
-
-    switch (ef_type) {
-      case NTP_EF_NTS_UNIQUE_IDENTIFIER:
-      case NTP_EF_NTS_COOKIE:
-      case NTP_EF_NTS_COOKIE_PLACEHOLDER:
-      case NTP_EF_NTS_AUTH_AND_EEF:
-        info->auth.mode = NTP_AUTH_NTS;
-        break;
-      default:
-        DEBUG_LOG("Unknown extension field type=%x", (unsigned int)ef_type);
-    }
-
-    info->ext_fields++;
-    parsed += ef_length;
-    remainder = info->length - parsed;
-  }
-
-  if (remainder == 0) {
-    /* No MAC */
-    return 1;
-  } else if (remainder >= NTP_MIN_MAC_LENGTH) {
-    info->auth.mode = NTP_AUTH_SYMMETRIC;
-    info->auth.mac.start = parsed;
-    info->auth.mac.length = remainder;
-    info->auth.mac.key_id = ntohl(*(uint32_t *)(data + parsed));
-    return 1;
-  }
-
-  DEBUG_LOG("Invalid format");
-  return 0;
-}
-
-/* ================================================== */
-
 int
 NAU_CheckRequestAuth(NTP_Packet *request, NTP_PacketInfo *info, uint32_t *kod)
 {

diff --git a/ntp_auth.h b/ntp_auth.h
index d99966556ac97e4dad30de533a0767f4106a8c11..0b8a8253e10630f7c32238d2e935c00e7015891c 100644 (file)
--- a/ntp_auth.h
+++ b/ntp_auth.h
@@ -55,9 +55,6 @@ extern int NAU_PrepareRequestAuth(NAU_Instance instance);
 extern int NAU_GenerateRequestAuth(NAU_Instance instance, NTP_Packet *request,
                                    NTP_PacketInfo *info);
 
-/* Parse a request or response to detect the authentication mode */
-extern int NAU_ParsePacket(NTP_Packet *packet, NTP_PacketInfo *info);
-
 /* Verify that a request is authentic.  If it is not authentic and a non-zero
    kod code is returned, a KoD response should be sent back. */
 extern int NAU_CheckRequestAuth(NTP_Packet *request, NTP_PacketInfo *info, uint32_t *kod);

diff --git a/ntp_core.c b/ntp_core.c
index 595118ed4aab33bfaabd534840f5b6def2d8e6e4..9ff339c9e408d2956c5c79d77d671a5b8d7bac4c 100644 (file)
--- a/ntp_core.c
+++ b/ntp_core.c
@@ -32,6 +32,7 @@
 #include "array.h"
 #include "ntp_auth.h"
 #include "ntp_core.h"
+#include "ntp_ext.h"
 #include "ntp_io.h"
 #include "memory.h"
 #include "sched.h"
@@ -1296,9 +1297,25 @@ transmit_timeout(void *arg)
 
 /* ================================================== */
 
+static int
+is_zero_data(unsigned char *data, int length)
+{
+  int i;
+
+  for (i = 0; i < length; i++)
+    if (data[i] != 0)
+      return 0;
+  return 1;
+}
+
+/* ================================================== */
+
 static int
 parse_packet(NTP_Packet *packet, int length, NTP_PacketInfo *info)
 {
+  int parsed, remainder, ef_length, ef_type;
+  unsigned char *data;
+
   if (length < NTP_HEADER_LENGTH || length % 4U != 0) {
     DEBUG_LOG("NTP packet has invalid length %d", length);
     return 0;
@@ -1315,11 +1332,89 @@ parse_packet(NTP_Packet *packet, int length, NTP_PacketInfo *info)
     return 0;
   }
 
-  /* Parse authentication extension fields or MAC */
-  if (!NAU_ParsePacket(packet, info))
-    return 0;
+  data = (void *)packet;
+  parsed = NTP_HEADER_LENGTH;
+  remainder = info->length - parsed;
 
-  return 1;
+  /* Check if this is a plain NTP packet with no extension fields or MAC */
+  if (remainder <= 0)
+    return 1;
+
+  assert(remainder % 4 == 0);
+
+  /* In NTPv3 and older packets don't have extension fields.  Anything after
+     the header is assumed to be a MAC. */
+  if (info->version <= 3) {
+    info->auth.mode = NTP_AUTH_SYMMETRIC;
+    info->auth.mac.start = parsed;
+    info->auth.mac.length = remainder;
+    info->auth.mac.key_id = ntohl(*(uint32_t *)(data + parsed));
+
+    /* Check if it is an MS-SNTP authenticator field or extended authenticator
+       field with zeroes as digest */
+    if (info->version == 3 && info->auth.mac.key_id != 0) {
+      if (remainder == 20 && is_zero_data(data + parsed + 4, remainder - 4))
+        info->auth.mode = NTP_AUTH_MSSNTP;
+      else if (remainder == 72 && is_zero_data(data + parsed + 8, remainder - 8))
+        info->auth.mode = NTP_AUTH_MSSNTP_EXT;
+    }
+
+    return 1;
+  }
+
+  /* Check for a crypto NAK */
+  if (remainder == 4 && ntohl(*(uint32_t *)(data + parsed)) == 0) {
+    info->auth.mode = NTP_AUTH_SYMMETRIC;
+    info->auth.mac.start = parsed;
+    info->auth.mac.length = remainder;
+    info->auth.mac.key_id = 0;
+    return 1;
+  }
+
+  /* Parse the rest of the NTPv4 packet */
+
+  while (remainder > 0) {
+    /* Check if the remaining data is a MAC */
+    if (remainder >= NTP_MIN_MAC_LENGTH && remainder <= NTP_MAX_V4_MAC_LENGTH)
+      break;
+
+    /* Check if this is a valid NTPv4 extension field and skip it */
+    if (!NEF_ParseField(packet, info->length, parsed, &ef_length, &ef_type, NULL, NULL)) {
+      DEBUG_LOG("Invalid format");
+      return 0;
+    }
+
+    assert(ef_length > 0 && ef_length % 4 == 0);
+
+    switch (ef_type) {
+      case NTP_EF_NTS_UNIQUE_IDENTIFIER:
+      case NTP_EF_NTS_COOKIE:
+      case NTP_EF_NTS_COOKIE_PLACEHOLDER:
+      case NTP_EF_NTS_AUTH_AND_EEF:
+        info->auth.mode = NTP_AUTH_NTS;
+        break;
+      default:
+        DEBUG_LOG("Unknown extension field type=%x", (unsigned int)ef_type);
+    }
+
+    info->ext_fields++;
+    parsed += ef_length;
+    remainder = info->length - parsed;
+  }
+
+  if (remainder == 0) {
+    /* No MAC */
+    return 1;
+  } else if (remainder >= NTP_MIN_MAC_LENGTH) {
+    info->auth.mode = NTP_AUTH_SYMMETRIC;
+    info->auth.mac.start = parsed;
+    info->auth.mac.length = remainder;
+    info->auth.mac.key_id = ntohl(*(uint32_t *)(data + parsed));
+    return 1;
+  }
+
+  DEBUG_LOG("Invalid format");
+  return 0;
 }
 
 /* ================================================== */

diff --git a/nts_ntp.h b/nts_ntp.h
index a34eee129e024119320de19ce5870232e5b31cad..e39def28d86b70a4908d0192874a2dfd156931fe 100644 (file)
--- a/nts_ntp.h
+++ b/nts_ntp.h
@@ -27,11 +27,6 @@
 #ifndef GOT_NTS_NTP_H
 #define GOT_NTS_NTP_H
 
-#define NTP_EF_NTS_UNIQUE_IDENTIFIER    0x0104
-#define NTP_EF_NTS_COOKIE               0x0204
-#define NTP_EF_NTS_COOKIE_PLACEHOLDER   0x0304
-#define NTP_EF_NTS_AUTH_AND_EEF         0x0404
-
 #define NTP_KOD_NTS_NAK                 0x4e54534e
 
 #define NTS_MIN_UNIQ_ID_LENGTH          32

diff --git a/nts_ntp_client.c b/nts_ntp_client.c
index 4b16ffdb950fd248ad129621f51b6eb921845565..34412a62efb9b9c8ed81e3859637b254fc3113b3 100644 (file)
--- a/nts_ntp_client.c
+++ b/nts_ntp_client.c
@@ -457,7 +457,7 @@ 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))
-      /* This is not expected as the packet already passed NAU_ParsePacket() */
+      /* This is not expected as the packet already passed parsing */
       return 0;
 
     switch (ef_type) {

diff --git a/nts_ntp_server.c b/nts_ntp_server.c
index a3da48516626e71e1e0a8d9bb4e185ba11f30fe5..9226c895005019c703f2dea95fc10709e84d48df 100644 (file)
--- a/nts_ntp_server.c
+++ b/nts_ntp_server.c
@@ -242,7 +242,7 @@ NNS_GenerateResponseAuth(NTP_Packet *request, NTP_PacketInfo *req_info,
   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))
-      /* This is not expected as the packet already passed NAU_ParsePacket() */
+      /* This is not expected as the packet already passed parsing */
       return 0;
 
     switch (ef_type) {

diff --git a/test/unit/ntp_auth.c b/test/unit/ntp_auth.c
index 5c4b25558a469f1422aa006704f56ee6dbfb18b8..5f2a9bc322a71d7a9f21f1346c30f1094457fbdf 100644 (file)
--- a/test/unit/ntp_auth.c
+++ b/test/unit/ntp_auth.c
@@ -229,12 +229,8 @@ test_unit(void)
     if (!inst || !can_auth_req)
       add_dummy_auth(mode, key_id, &req, &req_info);
 
-    TEST_CHECK(req_info.auth.mode == mode);
-
-    memset(&req_info.auth, 0, sizeof (req_info.auth));
-    TEST_CHECK(NAU_ParsePacket(&req, &req_info));
-    TEST_CHECK(req_info.auth.mode == mode);
-    TEST_CHECK(req_info.auth.mac.key_id == key_id);
+    assert(req_info.auth.mode == mode);
+    assert(req_info.auth.mac.key_id == key_id);
 
     kod = 1;
     TEST_CHECK(NAU_CheckRequestAuth(&req, &req_info, &kod) == can_auth_req);
@@ -259,10 +255,8 @@ test_unit(void)
     if (!can_auth_res)
       add_dummy_auth(mode, key_id, &res, &res_info);
 
-    memset(&res_info.auth, 0, sizeof (res_info.auth));
-    TEST_CHECK(NAU_ParsePacket(&res, &res_info));
-    TEST_CHECK(res_info.auth.mode == mode);
-    TEST_CHECK(res_info.auth.mac.key_id == key_id);
+    assert(res_info.auth.mode == mode);
+    assert(res_info.auth.mac.key_id == key_id);
 
     if (inst) {
       if (mode == NTP_AUTH_SYMMETRIC) {

diff --git a/test/unit/ntp_core.c b/test/unit/ntp_core.c
index 9aac5d632a7b28b0a235c7fdf0c36689abf52a18..74145b9630daac1be1a93769901bb07ddfc9f582 100644 (file)
--- a/test/unit/ntp_core.c
+++ b/test/unit/ntp_core.c
@@ -331,6 +331,55 @@ process_replay(NCR_Instance inst, NTP_Packet *packet_queue,
   advance_time(1e-6);
 }
 
+static void
+add_dummy_auth(NTP_AuthMode auth_mode, uint32_t key_id, NTP_Packet *packet, NTP_PacketInfo *info)
+{
+  unsigned char buf[64];
+  int len, fill;
+
+  info->auth.mode = auth_mode;
+
+  switch (auth_mode) {
+    case NTP_AUTH_NONE:
+      break;
+    case NTP_AUTH_SYMMETRIC:
+    case NTP_AUTH_MSSNTP:
+    case NTP_AUTH_MSSNTP_EXT:
+      switch (auth_mode) {
+        case NTP_AUTH_SYMMETRIC:
+          len = 16 + random() % 2 * 4;
+          fill = 1 + random() % 255;
+          break;
+        case NTP_AUTH_MSSNTP:
+          len = 16;
+          fill = 0;
+          break;
+        case NTP_AUTH_MSSNTP_EXT:
+          len = 68;
+          fill = 0;
+          break;
+        default:
+          assert(0);
+      }
+
+      assert(info->length + 4 + len <= sizeof (*packet));
+
+      *(uint32_t *)((unsigned char *)packet + info->length) = htonl(key_id);
+      info->auth.mac.key_id = key_id;
+      info->length += 4;
+
+      memset((unsigned char *)packet + info->length, fill, len);
+      info->length += len;
+      break;
+    case NTP_AUTH_NTS:
+      memset(buf, 0, sizeof (buf));
+      TEST_CHECK(NEF_AddField(packet, info, NTP_EF_NTS_AUTH_AND_EEF, buf, sizeof (buf)));
+      break;
+    default:
+      assert(0);
+  }
+}
+
 #define PACKET_QUEUE_LENGTH 10
 
 void
@@ -347,7 +396,8 @@ test_unit(void)
   CPS_NTP_Source source;
   NTP_Remote_Address remote_addr;
   NCR_Instance inst1, inst2;
-  NTP_Packet packet_queue[PACKET_QUEUE_LENGTH];
+  NTP_Packet packet_queue[PACKET_QUEUE_LENGTH], packet;
+  NTP_PacketInfo info;
 
   CNF_Initialise(0, 0);
   for (i = 0; i < sizeof conf / sizeof conf[0]; i++)
@@ -504,6 +554,47 @@ test_unit(void)
     NCR_DestroyInstance(inst2);
   }
 
+  memset(&packet, 0, sizeof (packet));
+  packet.lvm = NTP_LVM(LEAP_Normal, NTP_VERSION, MODE_CLIENT);
+
+  TEST_CHECK(parse_packet(&packet, NTP_HEADER_LENGTH, &info));
+  TEST_CHECK(info.auth.mode == NTP_AUTH_NONE);
+
+  TEST_CHECK(parse_packet(&packet, NTP_HEADER_LENGTH, &info));
+  add_dummy_auth(NTP_AUTH_SYMMETRIC, 100, &packet, &info);
+  memset(&info.auth, 0, sizeof (info.auth));
+  TEST_CHECK(parse_packet(&packet, info.length, &info));
+  TEST_CHECK(info.auth.mode == NTP_AUTH_SYMMETRIC);
+  TEST_CHECK(info.auth.mac.start == NTP_HEADER_LENGTH);
+  TEST_CHECK(info.auth.mac.length == info.length - NTP_HEADER_LENGTH);
+  TEST_CHECK(info.auth.mac.key_id == 100);
+
+  TEST_CHECK(parse_packet(&packet, NTP_HEADER_LENGTH, &info));
+  add_dummy_auth(NTP_AUTH_NTS, 0, &packet, &info);
+  memset(&info.auth, 0, sizeof (info.auth));
+  TEST_CHECK(parse_packet(&packet, info.length, &info));
+  TEST_CHECK(info.auth.mode == NTP_AUTH_NTS);
+
+  packet.lvm = NTP_LVM(LEAP_Normal, 3, MODE_CLIENT);
+
+  TEST_CHECK(parse_packet(&packet, NTP_HEADER_LENGTH, &info));
+  add_dummy_auth(NTP_AUTH_MSSNTP, 200, &packet, &info);
+  memset(&info.auth, 0, sizeof (info.auth));
+  TEST_CHECK(parse_packet(&packet, info.length, &info));
+  TEST_CHECK(info.auth.mode == NTP_AUTH_MSSNTP);
+  TEST_CHECK(info.auth.mac.start == NTP_HEADER_LENGTH);
+  TEST_CHECK(info.auth.mac.length == 20);
+  TEST_CHECK(info.auth.mac.key_id == 200);
+
+  TEST_CHECK(parse_packet(&packet, NTP_HEADER_LENGTH, &info));
+  add_dummy_auth(NTP_AUTH_MSSNTP_EXT, 300, &packet, &info);
+  memset(&info.auth, 0, sizeof (info.auth));
+  TEST_CHECK(parse_packet(&packet, info.length, &info));
+  TEST_CHECK(info.auth.mode == NTP_AUTH_MSSNTP_EXT);
+  TEST_CHECK(info.auth.mac.start == NTP_HEADER_LENGTH);
+  TEST_CHECK(info.auth.mac.length == 72);
+  TEST_CHECK(info.auth.mac.key_id == 300);
+
   KEY_Finalise();
   REF_Finalise();
   NCR_Finalise();