[Openvpn-devel,6/7] set TLS 1.2 as minimum by default

Message ID 20210904095629.6273-7-a@unstable.cc
State Superseded
Headers show
Series
  • change defaults and introduce compat-mode
Related show

Commit Message

Antonio Quartulli Sept. 4, 2021, 9:56 a.m.
Do not accept handshakes with peers trying to negotiate TLS lower than 1.2.
TLS 1.1 and 1.0 are not recommended and therefore will, by default,
allow TLS 1.2 as minimum version.

The minimum allowed version can still be controlled via
'--tls-version-min'.

At the same time automatically set '--tls-version-min' to 1.0 if the
user requires compatibility with versions onlder than 2.3.7, as that was
the only version supported back then.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 Changes.rst                          |  5 +++++
 doc/man-sections/generic-options.rst |  2 ++
 src/openvpn/options.c                | 16 ++++++++++++++++
 3 files changed, 23 insertions(+)

Comments

Arne Schwabe Sept. 6, 2021, 1:24 p.m. | #1
Am 04.09.21 um 11:56 schrieb Antonio Quartulli:
> Do not accept handshakes with peers trying to negotiate TLS lower than 1.2.
> TLS 1.1 and 1.0 are not recommended and therefore will, by default,
> allow TLS 1.2 as minimum version.
> 
> The minimum allowed version can still be controlled via
> '--tls-version-min'.
> 
> At the same time automatically set '--tls-version-min' to 1.0 if the
> user requires compatibility with versions onlder than 2.3.7, as that was
> the only version supported back then.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>  Changes.rst                          |  5 +++++
>  doc/man-sections/generic-options.rst |  2 ++
>  src/openvpn/options.c                | 16 ++++++++++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/Changes.rst b/Changes.rst
> index f803b760..472421c8 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -71,6 +71,11 @@ Deprecated features
>      This option mainly served a role as debug option when NCP was first
>      introduced. It should now no longer be necessary.
>  
> +TLS 1.0 and 1.1 are deprecated
> +    ``tls-version-min`` is set to 1.2 by default.  OpenVPN 2.6.0 defaults
> +    to a minimum TLS version of 1.2 as TLS 1.0 and 1.1 should be generally
> +    avoided. Note that OpenVPN versions older than 2.3.7 use TLS 1.0 only.
> +
>  ``--cipher`` argument is no longer included in ``--data-ciphers`` by default
>      Data cipher negotiation has been introduced in 2.4.0 and been significantly
>      improved in 2.5.0. The implicit fallback to the cipher specified in
> diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst
> index 3e099e12..e6c1fe45 100644
> --- a/doc/man-sections/generic-options.rst
> +++ b/doc/man-sections/generic-options.rst
> @@ -70,6 +70,8 @@ which mode OpenVPN is configured as.
>      ``--data-ciphers``
>    - 2.3.x or lower: ``--data-cipher-fallback`` is automatically added with
>      the same cipher as ``--cipher``
> +  - 2.3.6 or lower: ``--tls-version-min 1.0`` is added to the configuration
> +    when ``--tls-version-min`` is not explicitly set.
>  
>  --config file
>    Load additional config options from ``file`` where each line corresponds
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index f2fb6d64..6f6eb73d 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -850,6 +850,7 @@ init_options(struct options *o, const bool init_gc)
>      o->use_prediction_resistance = false;
>  #endif
>      o->tls_timeout = 2;
> +    o->ssl_flags = (TLS_VER_1_2 << SSLF_TLS_VERSION_MIN_SHIFT);
>      o->renegotiate_bytes = -1;
>      o->renegotiate_seconds = 3600;
>      o->renegotiate_seconds_min = -1;
> @@ -3140,6 +3141,21 @@ need_compatibility_before(const struct options *o, int version)
>  static void
>  options_set_backwards_compatible_options(struct options *o)
>  {
> +    /* TLS min version is not set */
> +    if ((o->ssl_flags & SSLF_TLS_VERSION_MIN_MASK) == 0)
> +    {
> +        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);
> +        }
> +        else
> +        {
> +            /* Use TLS 1.2 as proper default */
> +            o->ssl_flags = (TLS_VER_1_2 << SSLF_TLS_VERSION_MIN_SHIFT);
> +        }
> +    }
> +
>      /* Versions < 2.5.0 do need --cipher in the list of accepted ciphers.
>       * Version 2.4 might probably does not need it but NCP was not so
>       * good with 2.4 and ncp-disable might be more common on 2.4 peers.
> 

Acked-By: Arne Schwabe <arne@rfc2549.org>

Note that on newer Ubuntu versions, which their strangely patched
OpenSSL, you are already getting this behaviour by default.

Arne
Gert Doering Sept. 13, 2021, 12:39 p.m. | #2
Hi,

On Sat, Sep 04, 2021 at 11:56:28AM +0200, Antonio Quartulli wrote:
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index f2fb6d64..6f6eb73d 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -850,6 +850,7 @@ init_options(struct options *o, const bool init_gc)
>      o->use_prediction_resistance = false;
>  #endif
>      o->tls_timeout = 2;
> +    o->ssl_flags = (TLS_VER_1_2 << SSLF_TLS_VERSION_MIN_SHIFT);
>      o->renegotiate_bytes = -1;
>      o->renegotiate_seconds = 3600;
>      o->renegotiate_seconds_min = -1;

This code confuses me.  We now unconditionally initialize ssl_flags to
"TLS_VER_1_2"...

> @@ -3140,6 +3141,21 @@ need_compatibility_before(const struct options *o, int version)
>  static void
>  options_set_backwards_compatible_options(struct options *o)
>  {
> +    /* TLS min version is not set */
> +    if ((o->ssl_flags & SSLF_TLS_VERSION_MIN_MASK) == 0)
> +    {

... and then we check if that happens to be "0", so we can invoke the
compat handler...

The first hunk looks wrong.

gert
Antonio Quartulli Sept. 13, 2021, 7:21 p.m. | #3
Hi,

On 13/09/2021 14:39, Gert Doering wrote:
> Hi,
> 
> On Sat, Sep 04, 2021 at 11:56:28AM +0200, Antonio Quartulli wrote:
>> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
>> index f2fb6d64..6f6eb73d 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -850,6 +850,7 @@ init_options(struct options *o, const bool init_gc)
>>      o->use_prediction_resistance = false;
>>  #endif
>>      o->tls_timeout = 2;
>> +    o->ssl_flags = (TLS_VER_1_2 << SSLF_TLS_VERSION_MIN_SHIFT);
>>      o->renegotiate_bytes = -1;
>>      o->renegotiate_seconds = 3600;
>>      o->renegotiate_seconds_min = -1;
> 
> This code confuses me.  We now unconditionally initialize ssl_flags to
> "TLS_VER_1_2"...

Good catch!
I think this hunk should just go.

> 
>> @@ -3140,6 +3141,21 @@ need_compatibility_before(const struct options *o, int version)
>>  static void
>>  options_set_backwards_compatible_options(struct options *o)
>>  {
>> +    /* TLS min version is not set */
>> +    if ((o->ssl_flags & SSLF_TLS_VERSION_MIN_MASK) == 0)
>> +    {
> 
> ... and then we check if that happens to be "0", so we can invoke the
> compat handler...
> 
> The first hunk looks wrong.

It does. The "new default" is set in the else branch of this hunk.
Therefore there is no need to "preset" a default value in init_options().

Cheers,

Patch

diff --git a/Changes.rst b/Changes.rst
index f803b760..472421c8 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -71,6 +71,11 @@  Deprecated features
     This option mainly served a role as debug option when NCP was first
     introduced. It should now no longer be necessary.
 
+TLS 1.0 and 1.1 are deprecated
+    ``tls-version-min`` is set to 1.2 by default.  OpenVPN 2.6.0 defaults
+    to a minimum TLS version of 1.2 as TLS 1.0 and 1.1 should be generally
+    avoided. Note that OpenVPN versions older than 2.3.7 use TLS 1.0 only.
+
 ``--cipher`` argument is no longer included in ``--data-ciphers`` by default
     Data cipher negotiation has been introduced in 2.4.0 and been significantly
     improved in 2.5.0. The implicit fallback to the cipher specified in
diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst
index 3e099e12..e6c1fe45 100644
--- a/doc/man-sections/generic-options.rst
+++ b/doc/man-sections/generic-options.rst
@@ -70,6 +70,8 @@  which mode OpenVPN is configured as.
     ``--data-ciphers``
   - 2.3.x or lower: ``--data-cipher-fallback`` is automatically added with
     the same cipher as ``--cipher``
+  - 2.3.6 or lower: ``--tls-version-min 1.0`` is added to the configuration
+    when ``--tls-version-min`` is not explicitly set.
 
 --config file
   Load additional config options from ``file`` where each line corresponds
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index f2fb6d64..6f6eb73d 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -850,6 +850,7 @@  init_options(struct options *o, const bool init_gc)
     o->use_prediction_resistance = false;
 #endif
     o->tls_timeout = 2;
+    o->ssl_flags = (TLS_VER_1_2 << SSLF_TLS_VERSION_MIN_SHIFT);
     o->renegotiate_bytes = -1;
     o->renegotiate_seconds = 3600;
     o->renegotiate_seconds_min = -1;
@@ -3140,6 +3141,21 @@  need_compatibility_before(const struct options *o, int version)
 static void
 options_set_backwards_compatible_options(struct options *o)
 {
+    /* TLS min version is not set */
+    if ((o->ssl_flags & SSLF_TLS_VERSION_MIN_MASK) == 0)
+    {
+        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);
+        }
+        else
+        {
+            /* Use TLS 1.2 as proper default */
+            o->ssl_flags = (TLS_VER_1_2 << SSLF_TLS_VERSION_MIN_SHIFT);
+        }
+    }
+
     /* Versions < 2.5.0 do need --cipher in the list of accepted ciphers.
      * Version 2.4 might probably does not need it but NCP was not so
      * good with 2.4 and ncp-disable might be more common on 2.4 peers.