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,10 @@
+New in version 2.2.1
+====================
+
+Security fixes
+--------------
+* Restrict authentication of NTP server/peer to specified key (CVE-2016-1567)
+
 New in version 2.2
 ==================
 

chrony.texi.in

@@ -2508,6 +2508,9 @@ 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

ntp_core.c

@@ -1099,7 +1099,7 @@ static int
 receive_packet(NTP_Packet *message, struct timeval *now, double now_err, NCR_Instance inst, NTP_Local_Address *local_addr, int length)
 {
   int pkt_leap;
-  uint32_t pkt_refid;
+  uint32_t pkt_refid, pkt_key_id;
   double pkt_root_delay;
   double pkt_root_dispersion;
 
@@ -1190,11 +1190,13 @@ receive_packet(NTP_Packet *message, struct timeval *now, double now_err, NCR_Ins
      function is called only for known sources. */
 
   /* Test 5 checks for authentication failure.  If we expect authenticated info
-     from this peer/server and the packet doesn't have it or the authentication
-     is bad, it's got to fail.  If the peer or server sends us an authenticated
-     frame, but we're not bothered about whether he authenticates or not, just
-     ignore the test. */
-  test5 = inst->do_auth ? check_packet_auth(message, length, NULL, NULL) : 1;
+     from this peer/server and the packet doesn't have it, the authentication
+     is bad, or it's authenticated with a different key than expected, it's got
+     to fail.  If we don't expect the packet to be authenticated, just ignore
+     the test. */
+  test5 = !inst->do_auth ||
+          (check_packet_auth(message, length, NULL, &pkt_key_id) &&
+           pkt_key_id == inst->auth_key_id);
 
   /* Test 6 checks for unsynchronised server */
   test6 = pkt_leap != LEAP_Unsynchronised &&

test/simulation/105-ntpauth

@@ -39,4 +39,24 @@ check_chronyd_exit || test_fail
 # This check must fail as the client doesn't know the key
 check_sync && test_fail
 check_packet_interval || test_fail
+
+client_conf="keyfile tmp/keys"
+clients=2
+peers=2
+max_sync_time=300
+base_delay="$default_base_delay (* -1 (equal 0.1 from 3) (equal 0.1 to 1))"
+client_lpeer_options="key 1"
+client_rpeer_options="key 1"
+
+run_test || test_fail
+check_chronyd_exit || test_fail
+check_sync || test_fail
+
+client_rpeer_options="key 2"
+
+run_test || test_fail
+check_chronyd_exit || test_fail
+# This check must fail as the peers are using different keys"
+check_sync && test_fail
+
 test_pass

test/simulation/test.common

@@ -69,7 +69,11 @@ default_client_server_conf=""
 default_server_server_options=""
 default_client_server_options=""
 default_server_peer_options=""
+default_server_lpeer_options=""
+default_server_rpeer_options=""
 default_client_peer_options=""
+default_client_lpeer_options=""
+default_client_rpeer_options=""
 default_server_conf=""
 default_client_conf=""
 default_chronyc_conf=""
@@ -189,7 +193,8 @@ get_chronyd_conf() {
 		done
 		for i in $(seq 1 $peers); do
 			[ $i -eq $peer -o $i -gt $servers ] && continue
-			echo "peer 192.168.123.$[$servers * ($stratum - 1) + $i] $server_peer_options"
+			echo -n "peer 192.168.123.$[$servers * ($stratum - 1) + $i] $server_peer_options "
+			[ $i -lt $peer ] && echo "$server_lpeer_options" || echo "$server_rpeer_options"
 		done
 		echo "$server_conf"
 	else
@@ -202,7 +207,8 @@ get_chronyd_conf() {
 		fi
 		for i in $(seq 1 $peers); do
 			[ $i -eq $peer -o $i -gt $clients ] && continue
-			echo "peer 192.168.123.$[$servers * ($stratum - 1) + $i] $client_peer_options"
+			echo -n "peer 192.168.123.$[$servers * ($stratum - 1) + $i] $client_peer_options "
+			[ $i -lt $peer ] && echo "$client_lpeer_options" || echo "$client_rpeer_options"
 		done
 		echo "$client_conf"
 	fi