From 5fcfec03154082a38915188f0ad9ef04473a7a80 Mon Sep 17 00:00:00 2001
From: Razvan Cojocaru <razvan.cojocaru@openvpn.com>
Date: Thu, 9 Jan 2025 18:28:18 +0200
Subject: [PATCH] Fix openvpn::is_openvpn_protocol()

The original version just assumes that the packet length will
always be less than 255 (i.e. that the most significant byte of len
will be 0), which is not always the case for tls-crypt-v2.

This patch uses the OpenVPN 2.x logic and especially the comments,
which are very useful to make sense of an otherwise more opaque
algorithm.

While at it, removed the openvpn/common/size.hpp dependency by
using cstddef / std::size_t.

Signed-off-by: Razvan Cojocaru <razvan.cojocaru@openvpn.com>
---
 openvpn/ssl/is_openvpn_protocol.hpp | 71 ++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 22 deletions(-)

diff --git a/openvpn/ssl/is_openvpn_protocol.hpp b/openvpn/ssl/is_openvpn_protocol.hpp
index 7305e99ba..c40605d35 100644
--- a/openvpn/ssl/is_openvpn_protocol.hpp
+++ b/openvpn/ssl/is_openvpn_protocol.hpp
@@ -13,35 +13,62 @@
 #ifndef OPENVPN_SSL_IS_OPENVPN_PROTOCOL_H
 #define OPENVPN_SSL_IS_OPENVPN_PROTOCOL_H
 
-#include <algorithm> // for std::min
-
-#include <openvpn/common/size.hpp> // for size_t
+#include <cstddef> // for std::size_t
 
 namespace openvpn {
 
-// Peek at the first few bytes of a session and
-// distinguishing between OpenVPN or SSL protocols.
-inline bool is_openvpn_protocol(const unsigned char *p, const size_t len)
+/**
+ * @brief  Given either the first 2 or 3 bytes of an initial client -> server
+ *         data payload, return true if the protocol is that of an OpenVPN
+ *         client attempting to connect with an OpenVPN server.
+ *
+ * @param  p   Buffer containing packet data.
+ * @param  len Packet (buffer) length.
+ *
+ * @return true if we're dealing with an OpenVPN client, false otherwise.
+ */
+inline bool is_openvpn_protocol(const unsigned char *p, std::size_t len)
 {
-    const int CONTROL_HARD_RESET_CLIENT_V2 = 7;
-    const int CONTROL_HARD_RESET_CLIENT_V3 = 10;
-    const int OPCODE_SHIFT = 3;
-    const int MIN_INITIAL_PKT_SIZE = 14;
+    static constexpr int P_CONTROL_HARD_RESET_CLIENT_V2 = 7;
+    static constexpr int P_CONTROL_HARD_RESET_CLIENT_V3 = 10;
+    static constexpr int P_OPCODE_SHIFT = 3;
 
-    switch (std::min(len, size_t(3)))
+    if (len >= 3)
     {
-    case 3:
-        return p[0] == 0
-               && p[1] >= MIN_INITIAL_PKT_SIZE
-               && (p[2] == (CONTROL_HARD_RESET_CLIENT_V2 << OPCODE_SHIFT)
-                   || p[2] == (CONTROL_HARD_RESET_CLIENT_V3 << OPCODE_SHIFT));
-    case 2:
-        return p[0] == 0 && p[1] >= MIN_INITIAL_PKT_SIZE;
-    case 1:
-        return p[0] == 0;
-    default:
-        return true;
+        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));
+        }
+
+        /* 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));
     }
+
+    if (len >= 2)
+    {
+        int plen = (p[0] << 8) | p[1];
+        return plen >= 14 && plen <= 255;
+    }
+
+    return true;
 }
 
 } // namespace openvpn