Message ID | 20211015043227.10679-1-selva.nair@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [Openvpn-devel] Fix the "default" tls-version-min setting | expand |
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>
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 --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); } }