diff mbox series

[Openvpn-devel] Fix illegal client float

Message ID 20200413155650.114-1-lstipakov@gmail.com
State Superseded
Headers show
Series [Openvpn-devel] Fix illegal client float | expand

Commit Message

Lev Stipakov April 13, 2020, 5:56 a.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 buffer length to zero if
   data channel key has not been initialized, which leads to

 - openvpn_decrypt() returns true if buffer 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>
---
 src/openvpn/multi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Arne Schwabe April 14, 2020, 7:37 a.m. UTC | #1 
Am 13.04.20 um 17:56 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 buffer length to zero if
>    data channel key has not been initialized, which leads to
> 
>  - openvpn_decrypt() returns true if buffer 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>
> ---
>  src/openvpn/multi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index da0aeeba..58892a87 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2555,7 +2555,7 @@ 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)
> +                if (floated && c->c2.buf.len > 0)
>                  {
>                      multi_process_float(m, m->pending);
>                  }
> 

I think the commit is right but should we add a comment here saying that
len >0 is required to have valid decrypted packet?

Arne
diff mbox series

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index da0aeeba..58892a87 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2555,7 +2555,7 @@  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)
+                if (floated && c->c2.buf.len > 0)
                 {
                     multi_process_float(m, m->pending);
                 }