Fix buffer overflow when processing crafted command packets

When the length of the REQ_SUBNETS_ACCESSED, REQ_CLIENT_ACCESSES
command requests and the RPY_SUBNETS_ACCESSED, RPY_CLIENT_ACCESSES,
RPY_CLIENT_ACCESSES_BY_INDEX, RPY_MANUAL_LIST command replies is
calculated, the number of items stored in the packet is not validated.

A crafted command request/reply can be used to crash the server/client.
Only clients allowed by cmdallow (by default only localhost) can crash
the server.

With chrony versions 1.25 and 1.26 this bug has a smaller security
impact as the server requires the clients to be authenticated in order
to process the subnet and client accesses commands. In 1.27 and 1.28,
however, the invalid calculated length is included also in the
authentication check which may cause another crash.

diff --git a/client.c b/client.c
index 595d9f4abe355983a5a7c346ab2643fdf4fe5a5c..370b0827bdcff74a46296dd89bf598c6821c172e 100644 (file)
--- a/client.c
+++ b/client.c
@@ -1371,7 +1371,8 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok)
         read_length = recvfrom_status;
         expected_length = PKL_ReplyLength(reply);
 
-        bad_length = (read_length < expected_length);
+        bad_length = (read_length < expected_length ||
+                      expected_length < offsetof(CMD_Reply, data));
         bad_sender = (where_from.u.sa_family != his_addr.u.sa_family ||
                       (where_from.u.sa_family == AF_INET &&
                        (where_from.in4.sin_addr.s_addr != his_addr.in4.sin_addr.s_addr ||

diff --git a/cmdmon.c b/cmdmon.c
index 75bc464600ca6562ad5fedc93894d23dee6bd733..e4f734918d3922221e73cf7688e6d1efae30d48f 100644 (file)
--- a/cmdmon.c
+++ b/cmdmon.c
@@ -1781,29 +1781,10 @@ read_from_cmd_socket(void *anything)
   }
 
   read_length = status;
-  expected_length = PKL_CommandLength(&rx_message);
-  rx_command = ntohs(rx_message.command);
 
   LCL_ReadRawTime(&now);
   LCL_CookTime(&now, &cooked_now, NULL);
 
-  tx_message.version = PROTO_VERSION_NUMBER;
-  tx_message.pkt_type = PKT_TYPE_CMD_REPLY;
-  tx_message.res1 = 0;
-  tx_message.res2 = 0;
-  tx_message.command = rx_message.command;
-  tx_message.sequence = rx_message.sequence;
-  tx_message.reply = htons(RPY_NULL);
-  tx_message.number = htons(1);
-  tx_message.total = htons(1);
-  tx_message.pad1 = 0;
-  tx_message.utoken = htonl(utoken);
-  /* Set this to a default (invalid) value.  This protects against the
-     token field being set to an arbitrary value if we reject the
-     message, e.g. due to the host failing the access check. */
-  tx_message.token = htonl(0xffffffffUL);
-  memset(&tx_message.auth, 0, sizeof(tx_message.auth));
-
   switch (where_from.u.sa_family) {
     case AF_INET:
       remote_ip.family = IPADDR_INET4;
@@ -1830,7 +1811,14 @@ read_from_cmd_socket(void *anything)
 
   allowed = ADF_IsAllowed(access_auth_table, &remote_ip) || localhost;
 
-  if (read_length < offsetof(CMD_Request, data) ||
+  /* Message size sanity check */
+  if (read_length >= offsetof(CMD_Request, data)) {
+    expected_length = PKL_CommandLength(&rx_message);
+  } else {
+    expected_length = 0;
+  }
+
+  if (expected_length < offsetof(CMD_Request, data) ||
       rx_message.pkt_type != PKT_TYPE_CMD_REQUEST ||
       rx_message.res1 != 0 ||
       rx_message.res2 != 0) {
@@ -1842,6 +1830,25 @@ read_from_cmd_socket(void *anything)
     return;
   }
 
+  rx_command = ntohs(rx_message.command);
+
+  tx_message.version = PROTO_VERSION_NUMBER;
+  tx_message.pkt_type = PKT_TYPE_CMD_REPLY;
+  tx_message.res1 = 0;
+  tx_message.res2 = 0;
+  tx_message.command = rx_message.command;
+  tx_message.sequence = rx_message.sequence;
+  tx_message.reply = htons(RPY_NULL);
+  tx_message.number = htons(1);
+  tx_message.total = htons(1);
+  tx_message.pad1 = 0;
+  tx_message.utoken = htonl(utoken);
+  /* Set this to a default (invalid) value.  This protects against the
+     token field being set to an arbitrary value if we reject the
+     message, e.g. due to the host failing the access check. */
+  tx_message.token = htonl(0xffffffffUL);
+  memset(&tx_message.auth, 0, sizeof(tx_message.auth));
+
   if (rx_message.version != PROTO_VERSION_NUMBER) {
     tx_message.status = htons(STT_NOHOSTACCESS);
     if (!LOG_RateLimited()) {

diff --git a/pktlength.c b/pktlength.c
index a2682f7ef76c8784f4abbbb42cc1eab3aa328c67..68d0bda96d7e4d6dd36e26cd77e5934d653f2bd7 100644 (file)
--- a/pktlength.c
+++ b/pktlength.c
@@ -127,6 +127,8 @@ PKL_CommandLength(CMD_Request *r)
         {
           unsigned long ns;
           ns = ntohl(r->data.subnets_accessed.n_subnets);
+          if (ns > MAX_SUBNETS_ACCESSED)
+            return 0;
           return (offsetof(CMD_Request, data.subnets_accessed.subnets) +
                   ns * sizeof(REQ_SubnetsAccessed_Subnet));
         }
@@ -134,6 +136,8 @@ PKL_CommandLength(CMD_Request *r)
         {
           unsigned long nc;
           nc = ntohl(r->data.client_accesses.n_clients);
+          if (nc > MAX_CLIENT_ACCESSES)
+            return 0;
           return (offsetof(CMD_Request, data.client_accesses.client_ips) +
                   nc * sizeof(unsigned long));
         }
@@ -197,6 +201,8 @@ PKL_ReplyLength(CMD_Reply *r)
         {
           unsigned long ns = ntohl(r->data.subnets_accessed.n_subnets);
           if (r->status == htons(STT_SUCCESS)) {
+            if (ns > MAX_SUBNETS_ACCESSED)
+              return 0;
             return (offsetof(CMD_Reply, data.subnets_accessed.subnets) +
                     ns * sizeof(RPY_SubnetsAccessed_Subnet));
           } else {
@@ -207,6 +213,8 @@ PKL_ReplyLength(CMD_Reply *r)
         {
           unsigned long nc = ntohl(r->data.client_accesses.n_clients);
           if (r->status == htons(STT_SUCCESS)) {
+            if (nc > MAX_CLIENT_ACCESSES)
+              return 0;
             return (offsetof(CMD_Reply, data.client_accesses.clients) +
                     nc * sizeof(RPY_ClientAccesses_Client));
           } else {
@@ -217,6 +225,8 @@ PKL_ReplyLength(CMD_Reply *r)
         {
           unsigned long nc = ntohl(r->data.client_accesses_by_index.n_clients);
           if (r->status == htons(STT_SUCCESS)) {
+            if (nc > MAX_CLIENT_ACCESSES)
+              return 0;
             return (offsetof(CMD_Reply, data.client_accesses_by_index.clients) +
                     nc * sizeof(RPY_ClientAccesses_Client));
           } else {
@@ -226,6 +236,8 @@ PKL_ReplyLength(CMD_Reply *r)
       case RPY_MANUAL_LIST:
         {
           unsigned long ns = ntohl(r->data.manual_list.n_samples);
+          if (ns > MAX_MANUAL_LIST_SAMPLES)
+            return 0;
           if (r->status == htons(STT_SUCCESS)) {
             return (offsetof(CMD_Reply, data.manual_list.samples) +
                     ns * sizeof(RPY_ManualListSample));