doc: update NEWS

When a server/peer was specified with a key number to enable
authentication with a symmetric key, packets received from the
server/peer were accepted if they were authenticated with any of
the keys contained in the key file and not just the specified key.

This allowed an attacker who knew one key of a client/peer to modify
packets from its servers/peers that were authenticated with other
keys in a man-in-the-middle (MITM) attack. For example, in a network
where each NTP association had a separate key and all hosts had only
keys they needed, a client of a server could not attack other clients
of the server, but it could attack the server and also attack its own
clients (i.e. modify packets from other servers).

To not allow the server/peer to be authenticated with other keys
extend the authentication test to check if the key ID in the received
packet is equal to the configured key number. As a consequence, it's
no longer possible to authenticate two peers to each other with two
different keys, both peers have to be configured to use the same key.

This issue was discovered by Matt Street of Cisco ASIG.

NEWS

@@ -1,3 +1,20 @@
+New in version 1.31.2
+=====================
+
+Security fixes
+--------------
+* Restrict authentication of NTP server/peer to specified key (CVE-2016-1567)
+
+New in version 1.31.1
+=====================
+
+Security fixes
+--------------
+* Protect authenticated symmetric NTP associations against DoS attacks
+  (CVE-2015-1853)
+* Fix access configuration with subnet size indivisible by 4 (CVE-2015-1821)
+* Fix initialization of reply slots for authenticated commands (CVE-2015-1822)
+
 New in version 1.31
 ===================
 

addrfilt.c

@@ -199,7 +199,10 @@ set_subnet(TableNode *start_node,
 
       /* How many subnet entries to set : 1->8, 2->4, 3->2 */
       N = 1 << (NBITS-bits_to_go);
-      subnet = get_subnet(ip, bits_consumed);
+
+      subnet = get_subnet(ip, bits_consumed) & ~(N - 1);
+      assert(subnet + N <= TABLE_SIZE);
+
       if (!(node->extended)) {
         open_node(node);
       }

chrony.texi.in

@@ -2460,6 +2460,24 @@ be reported using the @code{clients} command in @code{chronyc}.
 The syntax of this directive is identical to that for the @code{server}
 directive (@pxref{server directive}), except that it is used to specify
 an NTP peer rather than an NTP server.
+
+When a key is specified by the @code{key} option to enable authentication, both
+peers must be configured to use the same key and the same key number.
+
+Please note that NTP peers that are not configured with a key to enable
+authentication are vulnerable to a denial-of-service attack.  An attacker
+knowing that NTP hosts A and B are peering with each other can send a packet
+with random timestamps to host A with source address of B which will set the
+NTP state variables on A to the values sent by the attacker.  Host A will then
+send on its next poll to B a packet with originate timestamp that doesn't match
+the transmit timestamp of B and the packet will be dropped.  If the attacker
+does this periodically for both hosts, they won't be able to synchronize to
+each other.
+
+This attack can be prevented by enabling authentication with the key option, or
+using the @code{server} directive on both sides to specify the other host as a
+server instead of peer, the only drawback is that it will double the network
+traffic between the two hosts.
 @c }}}
 @c {{{ pidfile
 @node pidfile directive

cmdmon.c

@@ -566,6 +566,7 @@ get_more_replies(void)
     for (i=1; i<REPLY_EXTEND_QUANTUM; i++) {
       new_replies[i-1].next = new_replies + i;
     }
+    new_replies[REPLY_EXTEND_QUANTUM - 1].next = NULL;
     free_replies = new_replies;
   }
 }

ntp_core.c

@@ -1005,9 +1005,6 @@ receive_packet(NTP_Packet *message, struct timeval *now, double now_err, NCR_Ins
 
   /* ==================== */
 
-  /* Save local receive timestamp */
-  inst->local_rx = *now;
-
   pkt_leap = (message->lvm >> 6) & 0x3;
   if (pkt_leap == 0x3) {
     source_is_synchronized = 0;
@@ -1039,14 +1036,6 @@ receive_packet(NTP_Packet *message, struct timeval *now, double now_err, NCR_Ins
     test2 = 1; /* Success */
   }
 
-  /* Regardless of any validity checks we apply, we are required to
-     save this field from the packet into the ntp source
-     instance record.  See RFC1305 section 3.4.4, peer.org <- pkt.xmt
-     & peer.peerpoll <- pkt.poll.  Note we can't do this assignment
-     before test1 has been carried out!! */
-
-  inst->remote_orig = message->transmit_ts;
-
   /* Test 3 requires that pkt.org != 0 and pkt.rec != 0.  If
      either of these are true it means the association is not properly
      'up'. */
@@ -1151,7 +1140,8 @@ receive_packet(NTP_Packet *message, struct timeval *now, double now_err, NCR_Ins
   if (inst->do_auth) {
     if (auth_len > 0) {
       auth_key_id = ntohl(message->auth_keyid);
-      test5 = check_packet_auth(message, auth_key_id, auth_len);
+      test5 = check_packet_auth(message, auth_key_id, auth_len) &&
+              auth_key_id == inst->auth_key_id;
     } else {
       /* If we expect authenticated info from this peer/server and the packet
          doesn't have it, it's got to fail */
@@ -1219,6 +1209,14 @@ receive_packet(NTP_Packet *message, struct timeval *now, double now_err, NCR_Ins
         kod_rate = 1;
   }
 
+  /* The transmit timestamp and local receive timestamp must not be saved when
+     the authentication test failed to prevent denial-of-service attacks on
+     symmetric associations using authentication */
+  if (test5) {
+    inst->remote_orig = message->transmit_ts;
+    inst->local_rx = *now;
+  }
+
   valid_kod = test1 && test2 && test5;
 
   valid_data = test1 && test2 && test3 && test4 && test4a && test4b;