diff mbox series

[Openvpn-devel,v2] Fix illegal client float

Message ID 20200415073017.22839-1-lstipakov@gmail.com
State Accepted
Headers
Series [Openvpn-devel,v2] Fix illegal client float |

Commit Message

Lev Stipakov April 14, 2020, 9:30 p.m. UTC 
  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(-)

Comments

Arne Schwabe April 14, 2020, 11:32 p.m. UTC | #1 
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>
Antonio Quartulli April 15, 2020, 6:45 a.m. UTC | #2 
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>
Gert Doering April 15, 2020, 10:16 p.m. UTC | #3 
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 mbox series

Patch

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);
                 }