From ac08b27cfa693d9be592bb2597c260635aee9e68 Mon Sep 17 00:00:00 2001 From: Steffan Karger Date: Tue, 25 Apr 2017 10:00:44 +0200 Subject: [PATCH 2/2] Drop packets instead of asserting out if packet id rolls over MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). Signed-off-by: Steffan Karger CVE-2017-7479 [Ubuntu note: 2.3.x does not support tlscrypt, so that portion of patch was dropped; other backporting to 2.3.x]. --sbeattie --- src/openvpn/crypto.c | 25 ++++++++++++++++--------- src/openvpn/packet_id.c | 22 ++++++++++++++++------ src/openvpn/packet_id.h | 1 + src/openvpn/tls_crypt.c | 6 +++++- tests/unit_tests/openvpn/test_packet_id.c | 11 +++++++++-- 5 files changed, 47 insertions(+), 18 deletions(-) Index: openvpn-2.3.2/src/openvpn/crypto.c =================================================================== --- openvpn-2.3.2.orig/src/openvpn/crypto.c +++ openvpn-2.3.2/src/openvpn/crypto.c @@ -112,9 +112,13 @@ openvpn_encrypt (struct buffer *buf, str 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, + 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 (mode == OPENVPN_MODE_CFB || mode == OPENVPN_MODE_OFB) @@ -182,9 +186,12 @@ openvpn_encrypt (struct buffer *buf, str } else /* No Encryption */ { - if (opt->packet_id) + if (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; } work = *buf; } Index: openvpn-2.3.2/src/openvpn/packet_id.c =================================================================== --- openvpn-2.3.2.orig/src/openvpn/packet_id.c +++ openvpn-2.3.2/src/openvpn/packet_id.c @@ -294,27 +294,37 @@ packet_id_read (struct packet_id_net *pi 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); Index: openvpn-2.3.2/src/openvpn/packet_id.h =================================================================== --- openvpn-2.3.2.orig/src/openvpn/packet_id.h +++ openvpn-2.3.2/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; /* Index: openvpn-2.3.2/tests/unit_tests/openvpn/test_packet_id.c =================================================================== --- openvpn-2.3.2.orig/tests/unit_tests/openvpn/test_packet_id.c +++ openvpn-2.3.2/tests/unit_tests/openvpn/test_packet_id.c @@ -130,8 +130,7 @@ test_packet_id_write_short_wrap(void **s 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 **st 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));