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-3-git-send-email-steffan.karger@fox-it.com>
URL: http://www.mail-archive.com/search?l=mid&q=1494358209-4568-3-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 734ef7310ca6cefcc7e8ee9e01a7dec72be91147..5a02ad0d931d2b1a1326e1d5651882356bb2f985 100644 (file)
--- a/Changes.rst
+++ b/Changes.rst
@@ -335,3 +335,7 @@ Security
   to hit an ASSERT() and stop the process.  If ``--tls-auth`` or ``--tls-crypt``
   is used, only attackers that have the ``--tls-auth`` or ``--tls-crypt`` key
   can mount an attack. (OSTIF/Quarkslab audit finding 5.1, CVE-2017-7478)
+- 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)

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index f5250acfd109fee9353ab3fcde59ae780e8c178f..50e6a734bd439814fe7384bc92e12cc166674aee 100644 (file)
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -93,7 +93,11 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
         buf_set_write(&iv_buffer, iv, iv_len);
 
         /* IV starts with packet id to make the IV unique for packet */
-        ASSERT(packet_id_write(&opt->packet_id.send, &iv_buffer, false, false));
+        if (!packet_id_write(&opt->packet_id.send, &iv_buffer, false, false))
+        {
+            msg(D_CRYPT_ERRORS, "ENCRYPT ERROR: packet ID roll over");
+            goto err;
+        }
 
         /* Remainder of IV consists of implicit part (unique per session) */
         ASSERT(buf_write(&iv_buffer, ctx->implicit_iv, ctx->implicit_iv_len));
@@ -191,11 +195,13 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
                 prng_bytes(iv_buf, iv_size);
 
                 /* Put packet ID in plaintext buffer */
-                if (packet_id_initialized(&opt->packet_id))
+                if (packet_id_initialized(&opt->packet_id)
+                    && !packet_id_write(&opt->packet_id.send, buf,
+                                        opt->flags & CO_PACKET_ID_LONG_FORM,
+                                        true))
                 {
-                    ASSERT(packet_id_write(&opt->packet_id.send, buf,
-                                           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))
@@ -251,11 +257,12 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
         }
         else                            /* No Encryption */
         {
-            if (packet_id_initialized(&opt->packet_id))
+            if (packet_id_initialized(&opt->packet_id)
+                && !packet_id_write(&opt->packet_id.send, buf,
+                                    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;
             }
             if (ctx->hmac)
             {

diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c
index 5175fb08dfbaf79457121edfc4b0451941de3b8b..10fe402d6a56d9e5cbbf3bdfee28ae5fa604e327 100644 (file)
--- a/src/openvpn/packet_id.c
+++ b/src/openvpn/packet_id.c
@@ -325,27 +325,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 109e56aad5adac1805b4623272e35325419a6448..aceacf8a6d58336c8b9e29801fbc0694471e8e8c 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/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index e47d25cb73f28ad15523e3adb07b394d1b7e54d4..7f59b1d67cf148ed372e77648f5b8cc93636e278 100644 (file)
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -98,7 +98,11 @@ tls_crypt_wrap(const struct buffer *src, struct buffer *dst,
          format_hex(BPTR(src), BLEN(src), 80, &gc));
 
     /* Get packet ID */
-    ASSERT(packet_id_write(&opt->packet_id.send, dst, true, false));
+    if (!packet_id_write(&opt->packet_id.send, dst, true, false))
+    {
+        msg(D_CRYPT_ERRORS, "TLS-CRYPT ERROR: packet ID roll over.");
+        goto err;
+    }
 
     dmsg(D_PACKET_CONTENT, "TLS-CRYPT WRAP AD: %s",
          format_hex(BPTR(dst), BLEN(dst), 0, &gc));

diff --git a/tests/unit_tests/openvpn/test_packet_id.c b/tests/unit_tests/openvpn/test_packet_id.c
index 5627a5b658e5d141ea97932f5eec30d8d1c9f45a..0a785adba0dfead88c74349d480599c9b82b16f8 100644 (file)
--- a/tests/unit_tests/openvpn/test_packet_id.c
+++ b/tests/unit_tests/openvpn/test_packet_id.c
@@ -129,8 +129,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
@@ -139,8 +138,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));