Drop packets instead of assert out if packet id rolls over (CVE-2017-7479)
PATCH 1/1 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.
--- Changes.rst | 10 ++++++++++ src/openvpn/crypto.c | 18 +++++++++++++----- src/openvpn/packet_id.c | 22 ++++++++++++++++------ src/openvpn/packet_id.h | 1 + tests/unit_tests/openvpn/test_packet_id.c | 11 +++++++++-- 5 files changed, 49 insertions(+), 13 deletions(-)
Changes.rst +10 -0
@@ -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 ==============
src/openvpn/crypto.c +13 -5
@@ -112,9 +112,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))
@@ -126,7 +128,11 @@ openvpn_encrypt (struct buffer *buf, struct buffer work,memset (iv_buf,0 , iv_size);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 */ {
@@ -185,9 +191,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; }
src/openvpn/packet_id.c +16 -6
@@ -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);
src/openvpn/packet_id.h +1 -0
@@ -50,6 +50,7 @@* to for network transmission. */ typedef uint32_t packet_id_type ; +#define PACKET_ID_MAX UINT32_MAXtypedef uint32_t net_time_t ;/*
tests/unit_tests/openvpn/test_packet_id.c +9 -2
@@ -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 ));
Signature · 1 line
2.7.4
PATCH 1/2 Don't assert out on receiving too-large control packets (CVE-2017-7478)
Commit 3c1b19e0 changed the maximum size of accepted control channel packets. This was needed for crypto negotiation (which is needed for a nice transition to a new default cipher), but exposed a DoS vulnerability. The vulnerability was found during the OpenVPN 2.4 code audit by Quarkslab (commisioned by OSTIF).
To fix the issue, we should not ASSERT() on external input (in this case the received packet size), but instead gracefully error out and drop the invalid packet.
--- Changes.rst | 8 ++++++++ src/openvpn/ssl.c | 7 ++++++- 2 files changed, 14 insertions(+), 1 deletion(-)
Changes.rst +8 -0
@@ -327,3 +327,11 @@ Bugfixes -------- - Fix memory leak introduced in 2.4.1: if --remote-cert-tls is used, we leaked some memory on each TLS (re)negotiation. + +Security +-------- +- Fix a pre-authentication denial-of-service attack on both clients and servers. + By sending a too-large control packet, OpenVPN 2.4.0 or 2.4.1 can be forced + 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)
src/openvpn/ssl.c +6 -1
@@ -3716,7 +3716,12 @@ tls_pre_decrypt(struct tls_multi *multi,/* Save incoming ciphertext packet to reliable buffer */ struct buffer *in =reliable_get_buf (ks->rec_reliable);ASSERT (in); -ASSERT (buf_copy (in, buf)); +if (!buf_copy (in, buf)) + { +msg (D_MULTI_DROPPED , +"Incoming control channel packet too big, dropping." ); + goto error; + }reliable_mark_active_incoming (ks->rec_reliable, in, id, op); }
Signature · 1 line
2.7.4
Re: [Openvpn-devel] [PATCH applied] Don't assert out on receiving too-large control packets (CVE-2017-7478)
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
This resolves the issue in my tests, code looks sane ... so ACK!
Your patch has been applied to the following branches
commit 5774cf4c25e1d8bf4e544702db8f157f111c9d93 (master) commit 66b99a0753352c5cc43e11e39835b6423112df98 (release/2.4)
Author: Steffan Karger Date: Tue May 9 21:30:08 2017 +0200
Don't assert out on receiving too-large control packets (CVE-2017-7478)
CVE: 2017-7478
Signed-off-by: Steffan Karger <[email protected]...>
Acked-by: David Sommerseth <[email protected]...>
Message-Id: <[email protected]...>
URL: http://www.mail-archive.com/search?l=mid&[email protected]...
Signed-off-by: David Sommerseth <[email protected]...>- --
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBCAAGBQJZFJLzAAoJEIbPlEyWcf3yMkAP/jQ3M60uZ1i6xcjNiwWlrIbn UXg7/pWGll62M4lBnr0ePTNkXMDEw74amoHtrmSahVMPZuIzHtLU3P+6q5g7aVES iuZuvmUJFbrc6QHqjIuUmWLi5DERt3946hKaVuuV/nug6cYURmHgf6Y9q/9lse/M EVEXqyU5WWqJzcK8ArAyTd0Ade4/PAUtyYdvKVWqAtugbLL+nfJV9hNkZV/iOSiH T9xpW8mhQyu6iqmdtvPux4bPwVZjODHTeCT5aOnpLaTGhRUQmDu3G4KdhPLYI9H5 TYGrjTI5CPvQ5/gRZx6HD93gYhU/uBq2cQROdzQbFCqQc/yMOqWVy4vkAGey4Kj3 RpFhgwDiClChDFNilV3s6OswW3vCml3Dl992jJVxsasUZDCibp3mEWxHGAbTKnQT i7pxOL7yTHEaU4U2tfEk+0sKq0ur0qX2JUBQbOIrBg1TVstyhSBxLVAL306qxCPN GwcgSYtWP0iIOx1/9C3gh3OOex4f6v3rxMFXg0hq1gQbsr9YXoYm0jgE92vADdMc RAUj9sQRl0J8Uy/u8p8tEQKho0gjkXSlowVgsxaOo96y2kj5lncqGrg2xnk7CRvv 5+Xjho26P9RjiC1GH5z7GvM8hL1jKazMRRVWX8FqYOhf7EwBoxnlMY4ZixEz+3iy kFEQazOrRBW4gXAPe/N7
=EVP5
-----END PGP SIGNATURE-----
PATCH 2/2 Drop packets instead of assert out if packet id rolls over (CVE-2017-7479)
In reply to Steffan KargerPreviously, 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.
--- Changes.rst | 4 ++++ 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 +++++++++-- 6 files changed, 51 insertions(+), 18 deletions(-)
Changes.rst +4 -0
@@ -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)
src/openvpn/crypto.c +16 -9
@@ -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) {
src/openvpn/packet_id.c +16 -6
@@ -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);
src/openvpn/packet_id.h +1 -0
@@ -50,6 +50,7 @@* to for network transmission. */ typedef uint32_t packet_id_type ; +#define PACKET_ID_MAX UINT32_MAXtypedef uint32_t net_time_t ;/*
src/openvpn/tls_crypt.c +5 -1
@@ -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));
tests/unit_tests/openvpn/test_packet_id.c +9 -2
@@ -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 ));
Signature · 1 line
2.7.4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK.
Your patch has been applied to the following branches
commit e498cb0ea8d3a451b39eaf6f9b6a7488f18250b8 (master) commit 591a4e574c43cb9e820950f15dcaabda261def78 (release/2.4)
Author: Steffan Karger Date: Tue May 9 21:30:09 2017 +0200
Drop packets instead of assert out if packet id rolls over (CVE-2017-7479)
CVE: 2017-7479
Signed-off-by: Steffan Karger <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Acked-by: David Sommerseth <[email protected]...>
Message-Id: <[email protected]...>
URL: http://www.mail-archive.com/search?l=mid&[email protected]...
Signed-off-by: David Sommerseth <[email protected]...>- --
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBCAAGBQJZFJS+AAoJEIbPlEyWcf3yeowP/3sXzeF+eISWsJBuXdmQMsW6 PGXZcdW6VQFDurVnFn0fzqa8L6D+vkdqgpV/y1b/FuyLkI3ChhWAL94DCnzpyHR1 XnU0KWWY9OaYIfhxG0FGxoSAIvD3ZZhExlixgkitHaUQYWbT55ksUIQXTAJTGgBp PvfGKSLKYrd61USIBtsswa/G28U/F+VfVjNNDK+3qHcALgDH0o+gQSls+4Mp1Nzv OURGMqHHRPvz9Omr6uDi58dgyByUKa9VTd+l/ogUvw13nfXnHFZv+SPPidz9DU1W c9Im/2XZGFbIaQSujsF06lAG4rBLSEt6DZaSrO9pIQEsJkXZb6GDZ5434HVjRnBT CTH2r+/mcIPOpF4514gI34ncksIDJUCxWmZXxFdB8krGU8FXi7WTYr2RLvChVKvv ukbFfWRJqOHUKYvVho5aGTEqRV4M37XFpttg9GmDjSBNNTCOJJIIiKGNjOww5Gle XnavipQ0MlOy0fSEreXZKMPrfEiHnsNTLbgd4GvdOe4p6mHrzCb62DRqVEhckskL SjGuVNnVxkn76QVUoz9u2mclYk5zJb9lE/1lpkYhEBf0vzKESO3Um8KDxPFXoQil JYkvu4kqTjPnt0XhOlpMCw1kViCpIqrZ8uDqyywu2srpGKawk7idqfSUq9Jbuwjv UTJBgCspAV4ca2NoAP06
=CDxG
-----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK.
Your patch has been applied to the release/2.3 branch
commit b727643cdf4e078f132a90e1c474a879a5760578
Author: Steffan Karger Date: Tue May 9 21:30:07 2017 +0200
Drop packets instead of assert out if packet id rolls over (CVE-2017-7479)
CVE: 2017-7479
Signed-off-by: Steffan Karger <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Acked-by: David Sommerseth <[email protected]...>
Message-Id: <[email protected]...>
URL: http://www.mail-archive.com/search?l=mid&[email protected]...
Signed-off-by: David Sommerseth <[email protected]...>- --
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBCAAGBQJZFJTiAAoJEIbPlEyWcf3yLmIQALnv1wF4LwnVxVTxFLz3+41l bxLI7Rkt5isMEJ6iGS6mivfajV3DWkaNTHOI2HcOtzN0OOlboUclT3Dkpk31GOCe EgThx68E2ZNWBmr7t/cJkGHjIOs5pEorBIIzqHFUvUGfq5kOm63y2U/9tOBRpQxg XE+Z+rtxdMQaVzQEhAv8LtFJypghoxUijZU7cj4w42yNL/G6vFlQqZl6v/B7sEoK Yc5aOX72EbTZJUI2HcEDHGG0ZK6S5BnjoAg3u1+/ZVY1GAVNICF5EYVw3LSOoeKp pkJ74FroxlvChohS7hobTjQJ9wnw26mkZM5faljzCWvc97r9eelMqyxd90NN8r/b BwgKiiSJxyVQV0wqqoyjRz2VdYxKgAtjYhN+01b2JDhgVFD8fb0Lp5BzlW1qt4sp tOgBiptbpz2lpVHeLLtjK1aIvUPq4mP8rxW+q2p++lAUZ+jhdIsGTTk1kEF6yCEJ ExeIFMCujPdyAcLdbLJFe9IZnoYQ2tjuiHHCzvpROGW0NH2CIcDJexYIwdej68I9 9Xp6bvJCkz6ti/flF/WC0kiS0B8KqlHA/ZoHTTQAWRzwhNvjEvVmIuR8HN35626b D7ym05llv5cfMgiArK7+Am22n0tMWV+iF1aYC5Z5/3c0BSJRNbFm4/QfZQSF/ShO QSF+QebMj0bbeuxIebz1
=ZLZb
-----END PGP SIGNATURE-----