[Bug 3072] Attack on interface selection

bk: 57d78deeyfknMUHpF4CWmP8gRRf6qg

diff --git a/ChangeLog b/ChangeLog
index 0805467dc6b9b1ce7768a039f6a2d87af37546b9..0ae8a6787a968631dfb80be2449a61b0b8eb0d9c 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+---
+* [Bug 3072] Attack on interface selection <perlinger@ntp.org>
+  - implemented Miroslav Lichvars <mlichvar@redhat.com> suggestion
+    to skip interface updates based on incoming packets
+
 ---
 (4.2.8p8) 2016/06/02 Released by Harlan Stenn <stenn@ntp.org>
 
@@ -19,7 +24,7 @@
 * Fix typo in ntp-wait and plot_summary.  HStenn.
 * Make sure we have an "author" file for git imports.  HStenn.
 * Update the sntp problem tests for MacOS.  HStenn.
-  
+
 ---
 (4.2.8p7) 2016/04/26 Released by Harlan Stenn <stenn@ntp.org>
 

diff --git a/ntpd/ntp_peer.c b/ntpd/ntp_peer.c
index cc23a382309c3023595ce61bb72644fed1bf32c1..b60188266ea348d90053eb34a78a0b54a88ec7d4 100644 (file)
--- a/ntpd/ntp_peer.c
+++ b/ntpd/ntp_peer.c
@@ -273,6 +273,22 @@ findexistingpeer(
 /*
  * findpeer - find and return a peer match for a received datagram in
  *           the peer_hash table.
+ *
+ * [Bug 3072] To faciliate a faster reorganisation after routing changes
+ * the original code re-assigned the peer address to be the destination
+ * of the received packet and initiated another round on a mismatch.
+ * Unfortunately this leaves us wide open for a DoS attack where the
+ * attacker directs a packet with forged destination address to us --
+ * this results in a wrong interface assignment, actually creating a DoS
+ * situation.
+ *
+ * This condition would persist until the next update of the interface
+ * list, but a continued attack would put us out of business again soon
+ * enough. Authentication alone does not help here, since it does not
+ * protect the UDP layer and leaves us open for a replay attack.
+ *
+ * So we do not update the adresses and wait until the next interface
+ * list update does the right thing for us.
  */
 struct peer *
 findpeer(
@@ -291,61 +307,50 @@ findpeer(
        srcadr = &rbufp->recv_srcadr;
        hash = NTP_HASH_ADDR(srcadr);
        for (p = peer_hash[hash]; p != NULL; p = p->adr_link) {
-               if (ADDR_PORT_EQ(srcadr, &p->srcadr)) {
-
-                       /*
-                        * if the association matching rules determine
-                        * that this is not a valid combination, then
-                        * look for the next valid peer association.
-                        */
-                       *action = MATCH_ASSOC(p->hmode, pkt_mode);
 
-                       /*
-                        * A response to our manycastclient solicitation
-                        * might be misassociated with an ephemeral peer
-                        * already spun for the server.  If the packet's
-                        * org timestamp doesn't match the peer's, check
-                        * if it matches the ACST prototype peer's.  If
-                        * so it is a redundant solicitation response,
-                        * return AM_ERR to discard it.  [Bug 1762]
-                        */
-                       if (MODE_SERVER == pkt_mode &&
-                           AM_PROCPKT == *action) {
-                               pkt = &rbufp->recv_pkt;
-                               NTOHL_FP(&pkt->org, &pkt_org);
-                               if (!L_ISEQU(&p->aorg, &pkt_org) &&
-                                   findmanycastpeer(rbufp))
-                                       *action = AM_ERR;
-                       }
+               /* [Bug 3072] ensure interface of peer matches */
+               if (p->dstadr != rbufp->dstadr)
+                       continue;
+
+               /* ensure peer source address matches */
+               if ( ! ADDR_PORT_EQ(srcadr, &p->srcadr))
+                       continue;
+               
+               /* If the association matching rules determine that this
+                * is not a valid combination, then look for the next
+                * valid peer association.
+                */
+               *action = MATCH_ASSOC(p->hmode, pkt_mode);
+
+               /* A response to our manycastclient solicitation might
+                * be misassociated with an ephemeral peer already spun
+                * for the server.  If the packet's org timestamp
+                * doesn't match the peer's, check if it matches the
+                * ACST prototype peer's.  If so it is a redundant
+                * solicitation response, return AM_ERR to discard it.
+                * [Bug 1762]
+                */
+               if (MODE_SERVER == pkt_mode && AM_PROCPKT == *action) {
+                       pkt = &rbufp->recv_pkt;
+                       NTOHL_FP(&pkt->org, &pkt_org);
+                       if (!L_ISEQU(&p->aorg, &pkt_org) &&
+                           findmanycastpeer(rbufp))
+                               *action = AM_ERR;
+               }
 
-                       /*
-                        * if an error was returned, exit back right
-                        * here.
-                        */
-                       if (*action == AM_ERR)
-                               return NULL;
+               /* if an error was returned, exit back right here. */
+               if (*action == AM_ERR)
+                       return NULL;
 
-                       /*
-                        * if a match is found, we stop our search.
-                        */
-                       if (*action != AM_NOMATCH)
-                               break;
-               }
+               /* if a match is found, we stop our search. */
+               if (*action != AM_NOMATCH)
+                       break;
        }
 
-       /*
-        * If no matching association is found
-        */
-       if (NULL == p) {
+       /* If no matching association is found... */
+       if (NULL == p)
                *action = MATCH_ASSOC(NO_PEER, pkt_mode);
-       } else if (p->dstadr != rbufp->dstadr) {
-               set_peerdstadr(p, rbufp->dstadr);
-               if (p->dstadr == rbufp->dstadr) {
-                       DPRINTF(1, ("Changed %s local address to match response\n",
-                                   stoa(&p->srcadr)));
-                       return findpeer(rbufp, pkt_mode, action);
-               }
-       }
+
        return p;
 }
 
@@ -621,7 +626,8 @@ set_peerdstadr(
 {
        struct peer *   unlinked;
 
-       if (p->dstadr == dstadr)
+       /* check for impossible or identical assignment */
+       if (p == NULL || p->dstadr == dstadr)
                return;
 
        /*
@@ -632,6 +638,8 @@ set_peerdstadr(
            (INT_MCASTIF & dstadr->flags) && MODE_CLIENT == p->hmode) {
                return;
        }
+
+       /* unlink from list if we have an address prior to assignment */
        if (p->dstadr != NULL) {
                p->dstadr->peercnt--;
                UNLINK_SLIST(unlinked, p->dstadr->peers, p, ilink,
@@ -640,8 +648,11 @@ set_peerdstadr(
                        stoa(&p->srcadr), latoa(p->dstadr),
                        latoa(dstadr));
        }
+       
        p->dstadr = dstadr;
-       if (dstadr != NULL) {
+
+       /* link to list if we have an address after assignment */
+       if (p->dstadr != NULL) {
                LINK_SLIST(dstadr->peers, p, ilink);
                dstadr->peercnt++;
        }