Drop packets instead of assert out if packet id rolls over (CVE-2017-7479)

Previously, if a mode was selected where packet ids are not allowed to roll
over, but renegotiation does not succeed for some reason (e.g. no password
entered in time, certificate expired or a malicious peer that refuses the
renegotiaion on purpose) we would continue to use the old keys.  Until the
packet ID would roll over and we would ASSERT() out.

Given that this can be triggered on purpose by an authenticated peer, this
is a fix for an authenticated remote DoS vulnerability.  An attack is
rather inefficient though; a peer would need to get us to send 2^32
packets (min-size packet is IP+UDP+OPCODE+PID+TAG (no payload), results in
(20+8+1+4+16)*2^32 bytes, or approx. 196 GB).

This is a fix for finding 5.2 from the OSTIF / Quarkslab audit.

CVE: 2017-7479
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <1494358209-4568-1-git-send-email-steffan.karger@fox-it.com>
URL: http://www.mail-archive.com/search?l=mid&q=1494358209-4568-1-git-send-email-steffan.karger@fox-it.com
Signed-off-by: David Sommerseth <davids@openvpn.net>

diff --git a/Changes.rst b/Changes.rst
index 1c0154c46fb108edcf12f2a3e6bdc318b3ddbcae..183e9fa4ae24e182f6364cb419ce45f402166ba1 100644 (file)
--- a/Changes.rst
+++ b/Changes.rst
@@ -104,6 +104,16 @@ Behavioral changes
 
 - Do not randomize resolving of IP addresses in getaddr()
 
+Version 2.3.15
+==============
+
+Security fixes
+--------------
+- Fix an authenticated remote DoS vulnerability that could be triggered by
+  causing a packet id roll over.  An attack is rather inefficient; a peer
+  would need to get us to send at least about 196 GB of data.
+  (OSTIF/Quarkslab audit finding 5.2, CVE-2017-7479)
+
 Version 2.3.14
 ==============
 

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 6fc97ddc9953ad1576456028c8a5b18635dc81dd..9d96f1247a67a8a3df29a02276c8d7ad9126396a 100644 (file)
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -110,9 +110,11 @@ openvpn_encrypt (struct buffer *buf, struct buffer work,
                prng_bytes (iv_buf, iv_size);
 
              /* Put packet ID in plaintext buffer or IV, depending on cipher mode */
-             if (opt->packet_id)
+             if (opt->packet_id
+                 && !packet_id_write (&opt->packet_id->send, buf, BOOL_CAST (opt->flags & CO_PACKET_ID_LONG_FORM), true))
                {
-                 ASSERT (packet_id_write (&opt->packet_id->send, buf, BOOL_CAST (opt->flags & CO_PACKET_ID_LONG_FORM), true));
+                 msg (D_CRYPT_ERRORS, "ENCRYPT ERROR: packet ID roll over");
+                 goto err;
                }
            }
          else if (cipher_kt_mode_ofb_cfb(cipher_kt))
@@ -123,7 +125,11 @@ openvpn_encrypt (struct buffer *buf, struct buffer work,
              ASSERT (opt->packet_id); /*  for this mode. */
 
              buf_set_write (&b, iv_buf, iv_size);
-             ASSERT (packet_id_write (&opt->packet_id->send, &b, true, false));
+             if (!packet_id_write (&opt->packet_id->send, &b, true, false))
+               {
+                 msg (D_CRYPT_ERRORS, "ENCRYPT ERROR: packet ID roll over");
+                 goto err;
+               }
            }
          else /* We only support CBC, CFB, or OFB modes right now */
            {
@@ -182,9 +188,11 @@ openvpn_encrypt (struct buffer *buf, struct buffer work,
        }
       else                             /* No Encryption */
        {
-         if (opt->packet_id)
+         if (opt->packet_id
+             && !packet_id_write (&opt->packet_id->send, buf, BOOL_CAST (opt->flags & CO_PACKET_ID_LONG_FORM), true))
            {
-             ASSERT (packet_id_write (&opt->packet_id->send, buf, BOOL_CAST (opt->flags & CO_PACKET_ID_LONG_FORM), true));
+             msg (D_CRYPT_ERRORS, "ENCRYPT ERROR: packet ID roll over");
+             goto err;
            }
          work = *buf;
        }

diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c
index c637be66d7c7d4d33af3d33c64727fbcc24451c1..9af74a1e7fe989ae85656b3d7b530b439eaec596 100644 (file)
--- a/src/openvpn/packet_id.c
+++ b/src/openvpn/packet_id.c
@@ -294,27 +294,37 @@ packet_id_read (struct packet_id_net *pin, struct buffer *buf, bool long_form)
   return true;
 }
 
-static void
+static bool
 packet_id_send_update(struct packet_id_send *p, bool long_form)
 {
   if (!p->time)
     {
       p->time = now;
     }
-  p->id++;
-  if (!p->id)
+  if (p->id == PACKET_ID_MAX)
     {
-      ASSERT(long_form);
+      /* Packet ID only allowed to roll over if using long form and time has
+       * moved forward since last roll over.
+       */
+      if (!long_form || now <= p->time)
+        {
+            return false;
+        }
       p->time = now;
-      p->id = 1;
+      p->id = 0;
     }
+    p->id++;
+    return true;
 }
 
 bool
 packet_id_write (struct packet_id_send *p, struct buffer *buf, bool long_form,
         bool prepend)
 {
-  packet_id_send_update(p, long_form);
+  if (!packet_id_send_update(p, long_form))
+    {
+      return false;
+    }
 
   const packet_id_type net_id = htonpid(p->id);
   const net_time_t net_time = htontime(p->time);

diff --git a/src/openvpn/packet_id.h b/src/openvpn/packet_id.h
index f6fa9bd1e973de89f0cbb786a56eb7177c62d60d..bfdfb771b98e0729b91037cc6f389c759b5148c5 100644 (file)
--- a/src/openvpn/packet_id.h
+++ b/src/openvpn/packet_id.h
@@ -50,6 +50,7 @@
  * to for network transmission.
  */
 typedef uint32_t packet_id_type;
+#define PACKET_ID_MAX UINT32_MAX
 typedef uint32_t net_time_t;
 
 /*

diff --git a/tests/unit_tests/openvpn/test_packet_id.c b/tests/unit_tests/openvpn/test_packet_id.c
index 3662e17f25918e74778c9144c552ef9f25f406d2..1ab3fd850c6b2dd12c9a6d13dcff444a7d31df9a 100644 (file)
--- a/tests/unit_tests/openvpn/test_packet_id.c
+++ b/tests/unit_tests/openvpn/test_packet_id.c
@@ -130,8 +130,7 @@ test_packet_id_write_short_wrap(void **state)
     struct test_packet_id_write_data *data = *state;
 
     data->pis.id = ~0;
-    expect_assert_failure(
-            packet_id_write(&data->pis, &data->test_buf, false, false));
+    assert_false(packet_id_write(&data->pis, &data->test_buf, false, false));
 }
 
 static void
@@ -140,8 +139,16 @@ test_packet_id_write_long_wrap(void **state)
     struct test_packet_id_write_data *data = *state;
 
     data->pis.id = ~0;
+    data->pis.time = 5006;
+
+    /* Write fails if time did not change */
+    now = 5006;
+    assert_false(packet_id_write(&data->pis, &data->test_buf, true, false));
+
+    /* Write succeeds if time moved forward */
     now = 5010;
     assert_true(packet_id_write(&data->pis, &data->test_buf, true, false));
+
     assert(data->pis.id == 1);
     assert(data->pis.time == now);
     assert_true(data->test_buf_data.buf_id == htonl(1));