diff --git a/Changes.rst b/Changes.rst index 734ef731..5a02ad0d 100644 --- 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 f5250acf..50e6a734 100644 --- 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 5175fb08..10fe402d 100644 --- 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 109e56aa..aceacf8a 100644 --- 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 e47d25cb..7f59b1d6 100644 --- 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 5627a5b6..0a785adb 100644 --- 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));