| Message ID | 20200415073017.22839-1-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] Fix illegal client float | expand |
Am 15.04.20 um 09:30 schrieb Lev Stipakov: > From: Lev Stipakov <lev@openvpn.net> > > There is a time frame between allocating peer-id and initializing data > channel key, which is performed on receiving push request. > > If a "rogue" data channel packet arrives during that time frame from > another address and with same peer-id, this would cause client to float > to that new address. This is because: > > - tls_pre_decrypt() sets packet length to zero if > data channel key has not been initialized, which leads to > > - openvpn_decrypt() returns true if packet length is zero, > which leads to > > - process_incoming_link_part1() returns true, which > calls multi_process_float(), which commits float > > Note that problem doesn't happen when data channel key > is initialized, since in this case openvpn_decrypt() returns false. > > Fix illegal float by adding buffer length check before calling > multi_process_float(). > > This should fix Trac #1272. > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > v2: add comment explaning why nonzero check needed > > src/openvpn/multi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index da0aeeba..37d2a6f2 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2555,7 +2555,8 @@ multi_process_incoming_link(struct multi_context *m, struct multi_instance *inst > orig_buf = c->c2.buf.data; > if (process_incoming_link_part1(c, lsi, floated)) > { > - if (floated) > + /* nonzero length means that we have a valid, decrypted packed */ > + if (floated && c->c2.buf.len > 0) > { > multi_process_float(m, m->pending); > } > Thanks! Acked-By: Arne Schwabe <arne@rfc2549.org>
Hi, On 15/04/2020 11:32, Arne Schwabe wrote: > Am 15.04.20 um 09:30 schrieb Lev Stipakov: >> From: Lev Stipakov <lev@openvpn.net> >> >> There is a time frame between allocating peer-id and initializing data >> channel key, which is performed on receiving push request. >> >> If a "rogue" data channel packet arrives during that time frame from >> another address and with same peer-id, this would cause client to float >> to that new address. This is because: >> >> - tls_pre_decrypt() sets packet length to zero if >> data channel key has not been initialized, which leads to >> >> - openvpn_decrypt() returns true if packet length is zero, >> which leads to >> >> - process_incoming_link_part1() returns true, which >> calls multi_process_float(), which commits float >> >> Note that problem doesn't happen when data channel key >> is initialized, since in this case openvpn_decrypt() returns false. >> >> Fix illegal float by adding buffer length check before calling >> multi_process_float(). >> >> This should fix Trac #1272. >> >> Signed-off-by: Lev Stipakov <lev@openvpn.net> >> --- >> v2: add comment explaning why nonzero check needed >> >> src/openvpn/multi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c >> index da0aeeba..37d2a6f2 100644 >> --- a/src/openvpn/multi.c >> +++ b/src/openvpn/multi.c >> @@ -2555,7 +2555,8 @@ multi_process_incoming_link(struct multi_context *m, struct multi_instance *inst >> orig_buf = c->c2.buf.data; >> if (process_incoming_link_part1(c, lsi, floated)) >> { >> - if (floated) >> + /* nonzero length means that we have a valid, decrypted packed */ >> + if (floated && c->c2.buf.len > 0) >> { >> multi_process_float(m, m->pending); >> } >> > Thanks! > > Acked-By: Arne Schwabe <arne@rfc2549.org> just went through the entire flow with Lev and David and this all makes a lot of sense now. Couldn't come up with a better fix for this issue. Acked-by: Antonio Quartulli <antonio@openvpn.net>
Your patch has been applied to the master and release/2.4 branch (bugfix).
I have amended the commit message to make it more clear what is the
risk (DoS against another random user of the same server, but no traffic
injection or stealing)
Code change is "obviously correct". Have still given it a t_client
run for good measure :-)
commit 37bc691e7d26ea4eb61a8a434ebd7a9ae76225ab (master)
commit f7b318f811bb43c0d3aa7f337ec6242ed2c33881 (release/2.4)
Author: Lev Stipakov
Date: Wed Apr 15 10:30:17 2020 +0300
Fix illegal client float (CVE-2020-11810)
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20200415073017.22839-1-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19720.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index da0aeeba..37d2a6f2 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2555,7 +2555,8 @@ multi_process_incoming_link(struct multi_context *m, struct multi_instance *inst orig_buf = c->c2.buf.data; if (process_incoming_link_part1(c, lsi, floated)) { - if (floated) + /* nonzero length means that we have a valid, decrypted packed */ + if (floated && c->c2.buf.len > 0) { multi_process_float(m, m->pending); }