Add padding to cmdmon requests to prevent amplification attack

To prevent an attacker using chronyd in an amplification attack, change
the protocol to include padding in request packets so that the largest
possible reply is not larger than the request. Request packets that
don't include this padding are ignored as invalid.

This is an incompatible change in the protocol. Clients from chrony
1.27, 1.28 and 1.29 will receive NULL reply with STT_BADPKTVERSION and
print "Protocol version mismatch". Clients from 1.26 and older will not
receive a reply as it would be larger than the request if it was padded
to be compatible with their protocol.

diff --git a/candm.h b/candm.h
index dde526a81aa5aa9cbe9c1a30368bde480f3bf52d..3d5c53bd4f557d1ba379fe578a687374310bcec4 100644 (file)
--- a/candm.h
+++ b/candm.h
@@ -360,13 +360,24 @@ typedef struct {
    Version 5 : auth data moved to the end of the packet to allow hashes with
    different sizes, extended sources, tracking and activity reports, dropped
    subnets accessed and client accesses
+
+   Version 6 : added padding to requests to prevent amplification attack,
+   changed maximum number of samples in manual list to 16
  */
 
-#define PROTO_VERSION_NUMBER 5
+#define PROTO_VERSION_NUMBER 6
+
+/* The oldest protocol versions that are compatible enough with the current
+   version to report a version mismatch for the server and the client */
+#define PROTO_VERSION_MISMATCH_COMPAT_SERVER 5
+#define PROTO_VERSION_MISMATCH_COMPAT_CLIENT 4
+
+/* The first protocol version using padding in requests */
+#define PROTO_VERSION_PADDING 6
 
-/* The oldest protocol version that is compatible enough with
-   the current version to report a version mismatch */
-#define PROTO_VERSION_MISMATCH_COMPAT 4
+/* The maximum length of padding in request packet, currently
+   defined by CLIENT_ACCESSES_BY_INDEX and MANUAL_LIST */
+#define MAX_PADDING_LENGTH 396
 
 /* ================================================== */
 
@@ -424,8 +435,13 @@ typedef struct {
     REQ_ReselectDistance reselect_distance;
   } data; /* Command specific parameters */
 
-  /* authentication of the packet, there is no hole after the actual data
-     from the data union, this field only sets the maximum auth size */
+  /* The following fields only set the maximum size of the packet.
+     There are no holes between them and the actual data. */
+
+  /* Padding used to prevent traffic amplification */
+  uint8_t padding[MAX_PADDING_LENGTH];
+
+  /* Authentication data */
   uint8_t auth[MAX_HASH_LENGTH];
 
 } CMD_Request;
@@ -585,6 +601,7 @@ typedef struct {
   uint32_t next_index;     /* the index 1 beyond those processed on this call */
   uint32_t n_clients;      /* the number of valid entries in the following array */
   RPY_ClientAccesses_Client clients[MAX_CLIENT_ACCESSES];
+  int32_t EOR;
 } RPY_ClientAccessesByIndex;
 
 #define MAX_MANUAL_LIST_SAMPLES 16
@@ -599,6 +616,7 @@ typedef struct {
 typedef struct {
   uint32_t n_samples;
   RPY_ManualListSample samples[MAX_MANUAL_LIST_SAMPLES];
+  int32_t EOR;
 } RPY_ManualList;
 
 typedef struct {

diff --git a/client.c b/client.c
index e6aab07b4bf97a5c04370e8d3f8754c669309e8f..1632a745fbbf0a475d0ad7e7c99d5a540935edb6 100644 (file)
--- a/client.c
+++ b/client.c
@@ -1260,6 +1260,7 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok)
   int read_length;
   int expected_length;
   int command_length;
+  int padding_length;
   int auth_length;
   struct timeval tv;
   int timeout;
@@ -1281,6 +1282,13 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok)
   n_attempts = 0;
 
   do {
+    command_length = PKL_CommandLength(request);
+    padding_length = PKL_CommandPaddingLength(request);
+    assert(command_length > 0 && command_length > padding_length);
+
+    /* Zero the padding to avoid sending uninitialized data. This needs to be
+       done before generating auth data as it includes the padding. */
+    memset(((char *)request) + command_length - padding_length, 0, padding_length);
 
     /* Decide whether to authenticate */
     if (password) {
@@ -1294,9 +1302,6 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok)
       auth_length = 0;
     }
 
-    command_length = PKL_CommandLength(request);
-    assert(command_length > 0);
-
     /* add empty MD5 auth so older servers will not drop the request
        due to bad length */
     if (!auth_length) {
@@ -1400,7 +1405,7 @@ submit_request(CMD_Request *request, CMD_Reply *reply, int *reply_auth_ok)
         }
         
         bad_header = ((reply->version != PROTO_VERSION_NUMBER &&
-                       !(reply->version >= PROTO_VERSION_MISMATCH_COMPAT &&
+                       !(reply->version >= PROTO_VERSION_MISMATCH_COMPAT_CLIENT &&
                          ntohs(reply->status) == STT_BADPKTVERSION)) ||
                       (reply->pkt_type != PKT_TYPE_CMD_REPLY) ||
                       (reply->res1 != 0) ||

diff --git a/cmdmon.c b/cmdmon.c
index a917f03af3d735bf9952c036d7d43a5de40dc369..71b74fc574dfff8e0e2917b4148e7ed8dc928d2d 100644 (file)
--- a/cmdmon.c
+++ b/cmdmon.c
@@ -265,11 +265,25 @@ prepare_socket(int family)
 void
 CAM_Initialise(int family)
 {
+  int i;
+
   assert(!initialised);
   initialised = 1;
 
   assert(sizeof (permissions) / sizeof (permissions[0]) == N_REQUEST_TYPES);
 
+  for (i = 0; i < N_REQUEST_TYPES; i++) {
+    CMD_Request r;
+    int command_length, padding_length;
+
+    r.version = PROTO_VERSION_NUMBER;
+    r.command = htons(i);
+    command_length = PKL_CommandLength(&r);
+    padding_length = PKL_CommandPaddingLength(&r);
+    assert(padding_length <= MAX_PADDING_LENGTH && padding_length <= command_length);
+    assert(command_length == 0 || command_length >= offsetof(CMD_Reply, data));
+  }
+
   utoken = (unsigned long) time(NULL);
 
   issued_tokens = returned_tokens = issue_pointer = 0;
@@ -1718,6 +1732,7 @@ read_from_cmd_socket(void *anything)
   }
 
   if (expected_length < offsetof(CMD_Request, data) ||
+      read_length < offsetof(CMD_Reply, data) ||
       rx_message.pkt_type != PKT_TYPE_CMD_REQUEST ||
       rx_message.res1 != 0 ||
       rx_message.res2 != 0) {
@@ -1756,12 +1771,9 @@ read_from_cmd_socket(void *anything)
     if (allowed)
       CLG_LogCommandAccess(&remote_ip, CLG_CMD_BAD_PKT, cooked_now.tv_sec);
 
-    if (rx_message.version >= PROTO_VERSION_MISMATCH_COMPAT) {
+    if (rx_message.version >= PROTO_VERSION_MISMATCH_COMPAT_SERVER) {
       tx_message.status = htons(STT_BADPKTVERSION);
-      /* add empty MD5 auth so older clients will not drop
-         the reply due to bad length */
-      memset(((char *)&tx_message) + PKL_ReplyLength(&tx_message), 0, 16);
-      transmit_reply(&tx_message, &where_from, 16);
+      transmit_reply(&tx_message, &where_from, 0);
     }
     return;
   }

diff --git a/pktlength.c b/pktlength.c
index 761dacd485d601da5c21d29daeb371b1766fb502..03a5a7e335c6413d6dc8d08c97e77ee70845fcac 100644 (file)
--- a/pktlength.c
+++ b/pktlength.c
@@ -35,8 +35,8 @@
 
 /* ================================================== */
 
-int
-PKL_CommandLength(CMD_Request *r)
+static int
+command_unpadded_length(CMD_Request *r)
 {
   int type;
   type = ntohs(r->command);
@@ -157,6 +157,149 @@ PKL_CommandLength(CMD_Request *r)
 }
 
 
+/* ================================================== */
+
+int
+PKL_CommandLength(CMD_Request *r)
+{
+  int command_length;
+
+  command_length = command_unpadded_length(r);
+  if (!command_length)
+    return 0;
+
+  return command_length + PKL_CommandPaddingLength(r);
+}
+
+/* ================================================== */
+
+#define PADDING_LENGTH_(request_length, reply_length) \
+  ((request_length) < (reply_length) ? (reply_length) - (request_length) : 0)
+
+#define PADDING_LENGTH(request_data, reply_data) \
+  PADDING_LENGTH_(offsetof(CMD_Request, request_data), offsetof(CMD_Reply, reply_data))
+
+int
+PKL_CommandPaddingLength(CMD_Request *r)
+{
+  int type;
+
+  if (r->version < PROTO_VERSION_PADDING)
+    return 0;
+
+  type = ntohs(r->command);
+
+  if (type < 0 || type >= N_REQUEST_TYPES)
+    return 0;
+
+  switch (type) {
+    case REQ_NULL:
+      return PADDING_LENGTH(data, data.null.EOR);
+    case REQ_ONLINE:
+      return PADDING_LENGTH(data.online.EOR, data.null.EOR);
+    case REQ_OFFLINE:
+      return PADDING_LENGTH(data.offline.EOR, data.null.EOR);
+    case REQ_BURST:
+      return PADDING_LENGTH(data.burst.EOR, data.null.EOR);
+    case REQ_MODIFY_MINPOLL:
+      return PADDING_LENGTH(data.modify_minpoll.EOR, data.null.EOR);
+    case REQ_MODIFY_MAXPOLL:
+      return PADDING_LENGTH(data.modify_maxpoll.EOR, data.null.EOR);
+    case REQ_DUMP:
+      return PADDING_LENGTH(data.dump.EOR, data.null.EOR);
+    case REQ_MODIFY_MAXDELAY:
+      return PADDING_LENGTH(data.modify_maxdelay.EOR, data.null.EOR);
+    case REQ_MODIFY_MAXDELAYRATIO:
+      return PADDING_LENGTH(data.modify_maxdelayratio.EOR, data.null.EOR);
+    case REQ_MODIFY_MAXDELAYDEVRATIO:
+      return PADDING_LENGTH(data.modify_maxdelaydevratio.EOR, data.null.EOR);
+    case REQ_MODIFY_MAXUPDATESKEW:
+      return PADDING_LENGTH(data.modify_maxupdateskew.EOR, data.null.EOR);
+    case REQ_LOGON:
+      return PADDING_LENGTH(data.logon.EOR, data.null.EOR);
+    case REQ_SETTIME:
+      return PADDING_LENGTH(data.settime.EOR, data.manual_timestamp.EOR);
+    case REQ_LOCAL:
+      return PADDING_LENGTH(data.local.EOR, data.null.EOR);
+    case REQ_MANUAL:
+      return PADDING_LENGTH(data.manual.EOR, data.null.EOR);
+    case REQ_N_SOURCES:
+      return PADDING_LENGTH(data.n_sources.EOR, data.n_sources.EOR);
+    case REQ_SOURCE_DATA:
+      return PADDING_LENGTH(data.source_data.EOR, data.source_data.EOR);
+    case REQ_REKEY:
+      return PADDING_LENGTH(data.rekey.EOR, data.null.EOR);
+    case REQ_ALLOW:
+      return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR);
+    case REQ_ALLOWALL:
+      return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR);
+    case REQ_DENY:
+      return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR);
+    case REQ_DENYALL:
+      return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR);
+    case REQ_CMDALLOW:
+      return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR);
+    case REQ_CMDALLOWALL:
+      return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR);
+    case REQ_CMDDENY:
+      return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR);
+    case REQ_CMDDENYALL:
+      return PADDING_LENGTH(data.allow_deny.EOR, data.null.EOR);
+    case REQ_ACCHECK:
+      return PADDING_LENGTH(data.ac_check.EOR, data.null.EOR);
+    case REQ_CMDACCHECK:
+      return PADDING_LENGTH(data.ac_check.EOR, data.null.EOR);
+    case REQ_ADD_SERVER:
+      return PADDING_LENGTH(data.ntp_source.EOR, data.null.EOR);
+    case REQ_ADD_PEER:
+      return PADDING_LENGTH(data.ntp_source.EOR, data.null.EOR);
+    case REQ_DEL_SOURCE:
+      return PADDING_LENGTH(data.del_source.EOR, data.null.EOR);
+    case REQ_WRITERTC:
+      return PADDING_LENGTH(data.writertc.EOR, data.null.EOR);
+    case REQ_DFREQ:
+      return PADDING_LENGTH(data.dfreq.EOR, data.null.EOR);
+    case REQ_DOFFSET:
+      return PADDING_LENGTH(data.doffset.EOR, data.null.EOR);
+    case REQ_TRACKING:
+      return PADDING_LENGTH(data.tracking.EOR, data.tracking.EOR);
+    case REQ_SOURCESTATS:
+      return PADDING_LENGTH(data.sourcestats.EOR, data.sourcestats.EOR);
+    case REQ_RTCREPORT:
+      return PADDING_LENGTH(data.rtcreport.EOR, data.rtc.EOR);
+    case REQ_TRIMRTC:
+      return PADDING_LENGTH(data.trimrtc.EOR, data.null.EOR);
+    case REQ_CYCLELOGS:
+      return PADDING_LENGTH(data.cyclelogs.EOR, data.null.EOR);
+    case REQ_SUBNETS_ACCESSED:
+    case REQ_CLIENT_ACCESSES:
+      /* No longer supported */
+      return 0;
+    case REQ_CLIENT_ACCESSES_BY_INDEX:
+      return PADDING_LENGTH(data.client_accesses_by_index.EOR, data.client_accesses_by_index.EOR);
+    case REQ_MANUAL_LIST:
+      return PADDING_LENGTH(data.manual_list.EOR, data.manual_list.EOR);
+    case REQ_MANUAL_DELETE:
+      return PADDING_LENGTH(data.manual_delete.EOR, data.null.EOR);
+    case REQ_MAKESTEP:
+      return PADDING_LENGTH(data.make_step.EOR, data.null.EOR);
+    case REQ_ACTIVITY:
+      return PADDING_LENGTH(data.activity.EOR, data.activity.EOR);
+    case REQ_RESELECT:
+      return PADDING_LENGTH(data.reselect.EOR, data.null.EOR);
+    case REQ_RESELECTDISTANCE:
+      return PADDING_LENGTH(data.reselect_distance.EOR, data.null.EOR);
+    case REQ_MODIFY_MINSTRATUM:
+      return PADDING_LENGTH(data.modify_minstratum.EOR, data.null.EOR);
+    case REQ_MODIFY_POLLTARGET:
+      return PADDING_LENGTH(data.modify_polltarget.EOR, data.null.EOR);
+    default:
+      /* If we fall through the switch, it most likely means we've forgotten to implement a new case */
+      assert(0);
+      return 0;
+  }
+}
+
 /* ================================================== */
 
 int

diff --git a/pktlength.h b/pktlength.h
index 026468d81400333f5765b8eb760bfe40e5669fa0..fad4c30f730e4923d58e1a6860e1f33c58587bfb 100644 (file)
--- a/pktlength.h
+++ b/pktlength.h
@@ -33,6 +33,8 @@
 
 extern int PKL_CommandLength(CMD_Request *r);
 
+extern int PKL_CommandPaddingLength(CMD_Request *r);
+
 extern int PKL_ReplyLength(CMD_Reply *r);
 
 #endif /* GOT_PKTLENGTH_H */