diff mbox series

[Openvpn-devel,v2] Fix port-share option with TLS-Crypt v2

Message ID 20201130123813.21388-1-arne@rfc2549.org
State Accepted
Headers
Series [Openvpn-devel,v2] Fix port-share option with TLS-Crypt v2 |

Commit Message

Arne Schwabe Nov. 30, 2020, 1:38 a.m. UTC 
  The port-share option assumed that all openvpn initial reset packets
are between 14 and 255 bytes long. This is not true for tls-crypt-v2.

Patch V2: use correct length for TLS-Crypt v2, use length variable
          non-tlscryptv2 test

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ps.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

Comments

Steffan Karger Dec. 3, 2020, 3:10 a.m. UTC | #1 
Hi,

On 30-11-2020 13:38, Arne Schwabe wrote:
> The port-share option assumed that all openvpn initial reset packets
> are between 14 and 255 bytes long. This is not true for tls-crypt-v2.
> 
> Patch V2: use correct length for TLS-Crypt v2, use length variable
>           non-tlscryptv2 test
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/ps.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
> index 2089e6b9..5d760782 100644
> --- a/src/openvpn/ps.c
> +++ b/src/openvpn/ps.c
> @@ -983,14 +983,38 @@ is_openvpn_protocol(const struct buffer *buf)
>      const int len = BLEN(buf);
>      if (len >= 3)
>      {
> -        return p[0] == 0
> -               && p[1] >= 14
> -               && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT)
> -                   || p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT));
> +        int plen = (p[0] << 8) | p[1];
> +
> +        if (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT))
> +        {
> +            /* WKc is at least 290 byte (not including metadata):
> +             *
> +             * 16 bit len + 256 bit HMAC + 2048 bit Kc = 2320 bit
> +             *
> +             * This is increased by the normal length of client handshake +
> +             * tls-crypt overhead (32)
> +             *
> +             * For metadata tls-crypt-v2.txt does not explicitly specify
> +             * an upper limit but we also have TLS_CRYPT_V2_MAX_WKC_LEN
> +             * as 1024 bytes. We err on the safe side with 255 extra overhead
> +             *
> +             * We don't do the 2 byte check for tls-crypt-v2 because it is very
> +             * unrealistic to have only 2 bytes available.
> +             */
> +            return  (plen >= 336 && plen < (1024 + 255));
> +        }
> +        else
> +        {
> +            /* For non tls-crypt2 we assume the packet length to valid between
> +             * 14 and 255 */
> +            return plen >= 14 && plen <= 255
> +                   && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT));
> +        }
>      }
>      else if (len >= 2)
>      {
> -        return p[0] == 0 && p[1] >= 14;
> +        int plen = (p[0] << 8) | p[1];
> +        return plen >= 14 && plen <= 255;
>      }
>      else
>      {
> 

Thanks. This looks good to me now.

Ran some tests to check that this correctly resolves the issue when
using --tls-crypt-v2 in combination with --port-share.

Acked-by: Steffan Karger <steffan@karger.me>

-Steffan
Gert Doering Dec. 4, 2020, 12:20 a.m. UTC | #2 
Magic heuristics... but if Arne and Steffan agree that this is correct,
who am I to dispute it :-) - it passes client tests and is not
"obviously wrong".

Your patch has been applied to the master and release/2.5 branch.

commit 1387f52682dcd3789c56c9979ccedca281ff88f4 (master)
commit c27f97dc0ac99344659cca57bd048543a9899266 (release/2.5)
Author: Arne Schwabe
Date:   Mon Nov 30 13:38:13 2020 +0100

     Fix port-share option with TLS-Crypt v2

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Steffan Karger <steffan.karger@foxcrypto.com>
     Message-Id: <20201130123813.21388-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21290.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
index 2089e6b9..5d760782 100644
--- a/src/openvpn/ps.c
+++ b/src/openvpn/ps.c
@@ -983,14 +983,38 @@  is_openvpn_protocol(const struct buffer *buf)
     const int len = BLEN(buf);
     if (len >= 3)
     {
-        return p[0] == 0
-               && p[1] >= 14
-               && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT)
-                   || p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT));
+        int plen = (p[0] << 8) | p[1];
+
+        if (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT))
+        {
+            /* WKc is at least 290 byte (not including metadata):
+             *
+             * 16 bit len + 256 bit HMAC + 2048 bit Kc = 2320 bit
+             *
+             * This is increased by the normal length of client handshake +
+             * tls-crypt overhead (32)
+             *
+             * For metadata tls-crypt-v2.txt does not explicitly specify
+             * an upper limit but we also have TLS_CRYPT_V2_MAX_WKC_LEN
+             * as 1024 bytes. We err on the safe side with 255 extra overhead
+             *
+             * We don't do the 2 byte check for tls-crypt-v2 because it is very
+             * unrealistic to have only 2 bytes available.
+             */
+            return  (plen >= 336 && plen < (1024 + 255));
+        }
+        else
+        {
+            /* For non tls-crypt2 we assume the packet length to valid between
+             * 14 and 255 */
+            return plen >= 14 && plen <= 255
+                   && (p[2] == (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT));
+        }
     }
     else if (len >= 2)
     {
-        return p[0] == 0 && p[1] >= 14;
+        int plen = (p[0] << 8) | p[1];
+        return plen >= 14 && plen <= 255;
     }
     else
     {