diff mbox series

[Openvpn-devel] Fix the "default" tls-version-min setting

Message ID 20211015043227.10679-1-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] Fix the "default" tls-version-min setting | expand

Commit Message

Selva Nair Oct. 14, 2021, 5:32 p.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

commit 968569f83b1561ea4dff5b8b1f0d7768e2a18e69
defined TLS 1.2 as the minimum version if not set
by user. But the patch introduced two errors:

(i) ssl_flags is overwritten without regard to other
    options set in the flags
(ii) Any tls-version-max set by the user is not taken into
     account.
Makes it impossible to set tls-version-max without also setting
tls-version-min along with loss of other bits set in ssl_flags.

Fix it.

The fix retains the original intent when possible, and tries to
use the maximum possible value when it cannot be set to TLS 1.2
without conflicting with user-specified tls-version-max, if any.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/options.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Arne Schwabe Oct. 15, 2021, 3:04 a.m. UTC | #1 
Am 15.10.21 um 06:32 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> commit 968569f83b1561ea4dff5b8b1f0d7768e2a18e69
> defined TLS 1.2 as the minimum version if not set
> by user. But the patch introduced two errors:
> 
> (i) ssl_flags is overwritten without regard to other
>     options set in the flags
> (ii) Any tls-version-max set by the user is not taken into
>      account.
> Makes it impossible to set tls-version-max without also setting
> tls-version-min along with loss of other bits set in ssl_flags.
> 
> Fix it.
> 
> The fix retains the original intent when possible, and tries to
> use the maximum possible value when it cannot be set to TLS 1.2
> without conflicting with user-specified tls-version-max, if any.

Makes sense and code looks correct.

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Oct. 15, 2021, 4:10 a.m. UTC | #2 
Arne beat me to it (I briefly looked at the patch yesterday when it
came in, noted "it makes sense" and was distracted...).  So, yeah,
thanks for fixing our oversights.

I have not tested (beyond "it compiles") but staring at the code 
looks very reasonable.   The "|=" without masking out the corresponding
bits is something I double-checked but it is safe, as we already know 
"they are all 0".

Your patch has been applied to the master branch.

commit 51be733ba236610dff6a1c361cf59172db97473a
Author: Selva Nair
Date:   Fri Oct 15 00:32:27 2021 -0400

     Fix the default tls-version-min setting

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20211015043227.10679-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22939.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 763dd330..7f14c1f3 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3168,15 +3168,22 @@  options_set_backwards_compatible_options(struct options *o)
     /* TLS min version is not set */
     if ((o->ssl_flags & SSLF_TLS_VERSION_MIN_MASK) == 0)
     {
+        int tls_ver_max = (o->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
+                          & SSLF_TLS_VERSION_MAX_MASK;
         if (need_compatibility_before(o, 20307))
         {
             /* 2.3.6 and earlier have TLS 1.0 only, set minimum to TLS 1.0 */
-            o->ssl_flags = (TLS_VER_1_0 << SSLF_TLS_VERSION_MIN_SHIFT);
+            o->ssl_flags |= (TLS_VER_1_0 << SSLF_TLS_VERSION_MIN_SHIFT);
         }
-        else
+        else if (tls_ver_max == 0 || tls_ver_max >= TLS_VER_1_2)
         {
             /* Use TLS 1.2 as proper default */
-            o->ssl_flags = (TLS_VER_1_2 << SSLF_TLS_VERSION_MIN_SHIFT);
+            o->ssl_flags |= (TLS_VER_1_2 << SSLF_TLS_VERSION_MIN_SHIFT);
+        }
+        else
+        {
+            /* Maximize the minimum version */
+            o->ssl_flags |= (tls_ver_max << SSLF_TLS_VERSION_MIN_SHIFT);
         }
     }