Browse code

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>

Steffan Karger authored on 2017/05/10 04:30:09
Showing 6 changed files
... ...
@@ -335,3 +335,7 @@ Security
335 335
   to hit an ASSERT() and stop the process.  If ``--tls-auth`` or ``--tls-crypt``
336 336
   is used, only attackers that have the ``--tls-auth`` or ``--tls-crypt`` key
337 337
   can mount an attack. (OSTIF/Quarkslab audit finding 5.1, CVE-2017-7478)
338
+- Fix an authenticated remote DoS vulnerability that could be triggered by
339
+  causing a packet id roll over.  An attack is rather inefficient; a peer
340
+  would need to get us to send at least about 196 GB of data.
341
+  (OSTIF/Quarkslab audit finding 5.2, CVE-2017-7479)
... ...
@@ -93,7 +93,11 @@ openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
93 93
         buf_set_write(&iv_buffer, iv, iv_len);
94 94
 
95 95
         /* IV starts with packet id to make the IV unique for packet */
96
-        ASSERT(packet_id_write(&opt->packet_id.send, &iv_buffer, false, false));
96
+        if (!packet_id_write(&opt->packet_id.send, &iv_buffer, false, false))
97
+        {
98
+            msg(D_CRYPT_ERRORS, "ENCRYPT ERROR: packet ID roll over");
99
+            goto err;
100
+        }
97 101
 
98 102
         /* Remainder of IV consists of implicit part (unique per session) */
99 103
         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,
191 191
                 prng_bytes(iv_buf, iv_size);
192 192
 
193 193
                 /* Put packet ID in plaintext buffer */
194
-                if (packet_id_initialized(&opt->packet_id))
194
+                if (packet_id_initialized(&opt->packet_id)
195
+                    && !packet_id_write(&opt->packet_id.send, buf,
196
+                                        opt->flags & CO_PACKET_ID_LONG_FORM,
197
+                                        true))
195 198
                 {
196
-                    ASSERT(packet_id_write(&opt->packet_id.send, buf,
197
-                                           opt->flags & CO_PACKET_ID_LONG_FORM,
198
-                                           true));
199
+                    msg(D_CRYPT_ERRORS, "ENCRYPT ERROR: packet ID roll over");
200
+                    goto err;
199 201
                 }
200 202
             }
201 203
             else if (cipher_kt_mode_ofb_cfb(cipher_kt))
... ...
@@ -251,11 +257,12 @@ openvpn_encrypt_v1(struct buffer *buf, struct buffer work,
251 251
         }
252 252
         else                            /* No Encryption */
253 253
         {
254
-            if (packet_id_initialized(&opt->packet_id))
254
+            if (packet_id_initialized(&opt->packet_id)
255
+                && !packet_id_write(&opt->packet_id.send, buf,
256
+                                    opt->flags & CO_PACKET_ID_LONG_FORM, true))
255 257
             {
256
-                ASSERT(packet_id_write(&opt->packet_id.send, buf,
257
-                        BOOL_CAST(opt->flags & CO_PACKET_ID_LONG_FORM),
258
-                        true));
258
+                msg(D_CRYPT_ERRORS, "ENCRYPT ERROR: packet ID roll over");
259
+                goto err;
259 260
             }
260 261
             if (ctx->hmac)
261 262
             {
... ...
@@ -325,27 +325,37 @@ packet_id_read(struct packet_id_net *pin, struct buffer *buf, bool long_form)
325 325
     return true;
326 326
 }
327 327
 
328
-static void
328
+static bool
329 329
 packet_id_send_update(struct packet_id_send *p, bool long_form)
330 330
 {
331 331
     if (!p->time)
332 332
     {
333 333
         p->time = now;
334 334
     }
335
-    p->id++;
336
-    if (!p->id)
335
+    if (p->id == PACKET_ID_MAX)
337 336
     {
338
-        ASSERT(long_form);
337
+        /* Packet ID only allowed to roll over if using long form and time has
338
+         * moved forward since last roll over.
339
+         */
340
+        if (!long_form || now <= p->time)
341
+        {
342
+            return false;
343
+        }
339 344
         p->time = now;
340
-        p->id = 1;
345
+        p->id = 0;
341 346
     }
347
+    p->id++;
348
+    return true;
342 349
 }
343 350
 
344 351
 bool
345 352
 packet_id_write(struct packet_id_send *p, struct buffer *buf, bool long_form,
346 353
         bool prepend)
347 354
 {
348
-    packet_id_send_update(p, long_form);
355
+    if (!packet_id_send_update(p, long_form))
356
+    {
357
+        return false;
358
+    }
349 359
 
350 360
     const packet_id_type net_id = htonpid(p->id);
351 361
     const net_time_t net_time = htontime(p->time);
... ...
@@ -50,6 +50,7 @@
50 50
  * to for network transmission.
51 51
  */
52 52
 typedef uint32_t packet_id_type;
53
+#define PACKET_ID_MAX UINT32_MAX
53 54
 typedef uint32_t net_time_t;
54 55
 
55 56
 /*
... ...
@@ -98,7 +98,11 @@ tls_crypt_wrap(const struct buffer *src, struct buffer *dst,
98 98
          format_hex(BPTR(src), BLEN(src), 80, &gc));
99 99
 
100 100
     /* Get packet ID */
101
-    ASSERT(packet_id_write(&opt->packet_id.send, dst, true, false));
101
+    if (!packet_id_write(&opt->packet_id.send, dst, true, false))
102
+    {
103
+        msg(D_CRYPT_ERRORS, "TLS-CRYPT ERROR: packet ID roll over.");
104
+        goto err;
105
+    }
102 106
 
103 107
     dmsg(D_PACKET_CONTENT, "TLS-CRYPT WRAP AD: %s",
104 108
          format_hex(BPTR(dst), BLEN(dst), 0, &gc));
... ...
@@ -129,8 +129,7 @@ test_packet_id_write_short_wrap(void **state)
129 129
     struct test_packet_id_write_data *data = *state;
130 130
 
131 131
     data->pis.id = ~0;
132
-    expect_assert_failure(
133
-            packet_id_write(&data->pis, &data->test_buf, false, false));
132
+    assert_false(packet_id_write(&data->pis, &data->test_buf, false, false));
134 133
 }
135 134
 
136 135
 static void
... ...
@@ -139,8 +138,16 @@ test_packet_id_write_long_wrap(void **state)
139 139
     struct test_packet_id_write_data *data = *state;
140 140
 
141 141
     data->pis.id = ~0;
142
+    data->pis.time = 5006;
143
+
144
+    /* Write fails if time did not change */
145
+    now = 5006;
146
+    assert_false(packet_id_write(&data->pis, &data->test_buf, true, false));
147
+
148
+    /* Write succeeds if time moved forward */
142 149
     now = 5010;
143 150
     assert_true(packet_id_write(&data->pis, &data->test_buf, true, false));
151
+
144 152
     assert(data->pis.id == 1);
145 153
     assert(data->pis.time == now);
146 154
     assert_true(data->test_buf_data.buf_id == htonl(1));