[Openvpn-devel,v4] Add support for tls-ciphersuites for TLS 1.3

OpenSSL 1.1.1 introduces a separate list for TLS 1.3 ciphers. As these
interfaces are meant to be user facing or not exposed at all and we
expose the tls-cipher interface, we should also expose tls-cipherlist.

Combining both settings into tls-cipher would add a lot of glue logic
that needs to be maintained and is error prone. On top of that, users
should not set either settings unless absolutely required.

OpenSSL's own s_client/s_server also expose both settings and I believe
most other software will too:

 -cipher val         Specify TLSv1.2 and below cipher list to be used
 -ciphersuites val   Specify TLSv1.3 ciphersuites to be used

For mbed TLS only the future can tell if we will see a combined or also
two separate lists.
---
 doc/openvpn.8             | 20 +++++++++++---
 src/openvpn/options.c     |  7 +++++
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  3 ++-
 src/openvpn/ssl_backend.h | 13 ++++++++-
 src/openvpn/ssl_mbedtls.c | 13 +++++++++
 src/openvpn/ssl_openssl.c | 56 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 108 insertions(+), 5 deletions(-)

Hi,

This seems to be the exact same patch as v3.

For future reference: this is v4 of 2/2 of the patch set.

On 07-10-18 23:55, Arne Schwabe wrote:
> OpenSSL 1.1.1 introduces a separate list for TLS 1.3 ciphers. As these
> interfaces are meant to be user facing or not exposed at all and we
> expose the tls-cipher interface, we should also expose tls-cipherlist.
> 
> Combining both settings into tls-cipher would add a lot of glue logic
> that needs to be maintained and is error prone. On top of that, users
> should not set either settings unless absolutely required.
> 
> OpenSSL's own s_client/s_server also expose both settings and I believe
> most other software will too:
> 
>  -cipher val         Specify TLSv1.2 and below cipher list to be used
>  -ciphersuites val   Specify TLSv1.3 ciphersuites to be used
> 
> For mbed TLS only the future can tell if we will see a combined or also
> two separate lists.
> ---
>  doc/openvpn.8             | 20 +++++++++++---
>  src/openvpn/options.c     |  7 +++++
>  src/openvpn/options.h     |  1 +
>  src/openvpn/ssl.c         |  3 ++-
>  src/openvpn/ssl_backend.h | 13 ++++++++-
>  src/openvpn/ssl_mbedtls.c | 13 +++++++++
>  src/openvpn/ssl_openssl.c | 56 +++++++++++++++++++++++++++++++++++++++
>  7 files changed, 108 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index feec0b6e..226d9a98 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -5001,11 +5001,13 @@ determines the derivation of the tunnel session keys.
>  .\"*********************************************************
>  .TP
>  .B \-\-tls\-cipher l
> +.TQ
> +.B \-\-tls\-ciphersuites l
>  A list
>  .B l
>  of allowable TLS ciphers delimited by a colon (":").
>  
> -This setting can be used to ensure that certain cipher suites are used (or
> +These setting can be used to ensure that certain cipher suites are used (or
>  not used) for the TLS connection.  OpenVPN uses TLS to secure the control
>  channel, over which the keys that are used to protect the actual VPN traffic
>  are exchanged.
> @@ -5014,20 +5016,32 @@ The supplied list of ciphers is (after potential OpenSSL/IANA name translation)
>  simply supplied to the crypto library.  Please see the OpenSSL and/or mbed TLS
>  documentation for details on the cipher list interpretation.
>  
> +For OpenSSL, the
> +.B \-\-tls-cipher
> +is used for TLS 1.2 and below. For TLS 1.3 and up, the
> +.B \-\-tls\-ciphersuites
> +setting is used. mbed TLS has no TLS 1.3 support yet and only the
> +.B \-\-tls-cipher
> +setting is used.
> +
>  Use
>  .B \-\-show\-tls
>  to see a list of TLS ciphers supported by your crypto library.
>  
>  Warning!
>  .B \-\-tls\-cipher
> -is an expert feature, which \- if used correcly \- can improve the security of
> -your VPN connection.  But it is also easy to unwittingly use it to carefully
> +and
> +.B \-\-tls\-ciphersuites
> +are expert features, which \- if used correcly \- can improve the security of
> +your VPN connection.  But it is also easy to unwittingly use them to carefully
>  align a gun with your foot, or just break your connection.  Use with care!
>  
>  The default for \-\-tls\-cipher is to use mbed TLS's default cipher list
>  when using mbed TLS or
>  "DEFAULT:!EXP:!LOW:!MEDIUM:!kDH:!kECDH:!DSS:!PSK:!SRP:!kRSA" when using
>  OpenSSL.
> +
> +The default for \-\-tls\-ciphersuites is to use the crypto library's default.
>  .\"*********************************************************
>  .TP
>  .B \-\-tls\-cert\-profile profile
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 95ebe3b3..891468bd 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -1763,6 +1763,7 @@ show_settings(const struct options *o)
>      SHOW_STR(cryptoapi_cert);
>  #endif
>      SHOW_STR(cipher_list);
> +    SHOW_STR(cipher_list_tls13);
>      SHOW_STR(tls_cert_profile);
>      SHOW_STR(tls_verify);
>      SHOW_STR(tls_export_cert);
> @@ -2757,6 +2758,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
>          MUST_BE_UNDEF(pkcs12_file);
>  #endif
>          MUST_BE_UNDEF(cipher_list);
> +        MUST_BE_UNDEF(cipher_list_tls13);
>          MUST_BE_UNDEF(tls_cert_profile);
>          MUST_BE_UNDEF(tls_verify);
>          MUST_BE_UNDEF(tls_export_cert);
> @@ -7954,6 +7956,11 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_GENERAL);
>          options->tls_cert_profile = p[1];
>      }
> +    else if (streq(p[0], "tls-ciphersuites") && p[1] && !p[2])
> +    {
> +        VERIFY_PERMISSION(OPT_P_GENERAL);
> +        options->cipher_list_tls13 = p[1];
> +    }
>      else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
>                                                     || (p[2] && streq(p[1], INLINE_FILE_TAG) ) || !p[2]) && !p[3])
>      {
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 4c3bc4fb..3e7ef4f8 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -508,6 +508,7 @@ struct options
>      const char *priv_key_file;
>      const char *pkcs12_file;
>      const char *cipher_list;
> +    const char *cipher_list_tls13;
>      const char *tls_cert_profile;
>      const char *ecdh_curve;
>      const char *tls_verify;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index fd12af05..adb0f3c8 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -626,9 +626,10 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
>      tls_ctx_set_cert_profile(new_ctx, options->tls_cert_profile);
>  
>      /* Allowable ciphers */
> -    /* Since @SECLEVEL also influces loading of certificates, set the
> +    /* Since @SECLEVEL also influences loading of certificates, set the
>       * cipher restrictions before loading certificates */
>      tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
> +    tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
>  
>      if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
>      {
> diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
> index 5023c02a..0995bb4c 100644
> --- a/src/openvpn/ssl_backend.h
> +++ b/src/openvpn/ssl_backend.h
> @@ -169,7 +169,8 @@ bool tls_ctx_initialised(struct tls_root_ctx *ctx);
>  bool tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags);
>  
>  /**
> - * Restrict the list of ciphers that can be used within the TLS context.
> + * Restrict the list of ciphers that can be used within the TLS context for TLS 1.2
> + * and below
>   *
>   * @param ctx           TLS context to restrict, must be valid.
>   * @param ciphers       String containing : delimited cipher names, or NULL to use
> @@ -177,6 +178,16 @@ bool tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags);
>   */
>  void tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers);
>  
> +/**
> + * Restrict the list of ciphers that can be used within the TLS context for TLS 1.3
> + * and higher
> + *
> + * @param ctx           TLS context to restrict, must be valid.
> + * @param ciphers       String containing : delimited cipher names, or NULL to use
> + *                                      sane defaults.
> + */
> +void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *ciphers);
> +
>  /**
>   * Set the TLS certificate profile.  The profile defines which crypto
>   * algorithms may be used in the supplied certificate.
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index e4850cb6..f23cd30a 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -222,6 +222,19 @@ tls_translate_cipher_name(const char *cipher_name)
>      return pair->iana_name;
>  }
>  
> +void
> +tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *ciphers)
> +{
> +    if (ciphers == NULL)
> +    {
> +        /* Nothing to do, return without warning message */
> +        return;
> +    }
> +
> +    msg(M_WARN, "mbed TLS does not support setting tls-ciphersuites. "
> +                "Ignoring TLS 1.3 cipher list: %s", ciphers);
> +}
> +
>  void
>  tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
>  {
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 1a66d178..f08d7cae 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -391,6 +391,7 @@ convert_tls_list_to_openssl(char* openssl_ciphers, size_t len,const char *cipher
>          openssl_ciphers[openssl_ciphers_len-1] = '\0';
>      }
>  }
> +
>  void
>  tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
>  {
> @@ -426,6 +427,61 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
>      }
>  }
>  
> +void
> +convert_tls13_list_to_openssl(char* openssl_ciphers, size_t len, const char *ciphers)

To be explicit, this still need two changes:

s/char* openssl_cipher/char *openssl_ciphers/ and wrap line please.

> +{
> +    /*
> +     * OpenSSL (and official IANA) cipher names have _ in them. We
> +     * historically used names with - in them. Silently convert names
> +     * with - to names with _ to support both
> +     */
> +    if (strlen(ciphers) >= (len - 1))
> +    {
> +        msg(M_FATAL,
> +            "Failed to set restricted TLS 1.3 cipher list, too long (>%d).",
> +            (int) (len - 1));
> +    }
> +
> +    strncpy(openssl_ciphers, ciphers, len);
> +
> +    for (size_t i = 0; i < strlen(openssl_ciphers); i++)
> +    {
> +        if (openssl_ciphers[i] == '-')
> +        {
> +            openssl_ciphers[i] = '_';
> +        }
> +    }
> +}
> +
> +void
> +tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *ciphers)
> +{
> +    if (ciphers == NULL)
> +    {
> +        /* default cipher list of OpenSSL 1.1.1 is sane, do not set own
> +         * default as we do with tls-cipher */
> +        return;
> +    }
> +
> +#if (OPENSSL_VERSION_NUMBER < 0x1010100fL)
> +        crypto_msg(M_WARN, "Not compiled with OpenSSL 1.1.1 or higher. "
> +                       "Ignoring TLS 1.3 only tls-ciphersuites '%s' setting.",
> +                        ciphers);
> +#else
> +    ASSERT(NULL != ctx);
> +
> +    char openssl_ciphers[4096];
> +    convert_tls13_list_to_openssl(openssl_ciphers, sizeof(openssl_ciphers),
> +                                  ciphers);
> +
> +    if (!SSL_CTX_set_ciphersuites(ctx->ctx, openssl_ciphers))
> +    {
> +        crypto_msg(M_FATAL, "Failed to set restricted TLS 1.3 cipher list: %s",
> +                   openssl_ciphers);
> +    }
> +#endif
> +}
> +
>  void
>  tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>  {
> 

I'll let you and Gert again decide who will tackle these final minor
nits ;-)

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

-Steffan

Your patch has been applied to the master branch and release/2.4 branch
(long-term compatibility, as agreed last weekend - and it's nicely 
isolated, so if you are not using the new option, nothing changes).

WRT Steffan's formatting nits on v3 - these are unchanged here in v4,
but the *next* TLS 1.3 patch will also pick *these* nits, so I decided
to pass through both patches "as is", instead of changing this one and
then the next one too just to move a whitespace change to a different
commit...  so yes, code formatting ghosts, come haunt me.

Test built on FreeBSD with OpenSSL 1.0.2 and on Linux with OpenSSL 1.1.1
and mbedTLS.  Some cursory tests but no full TLS 1.3 ciphersuites tests.

commit ea4ee31333a0cddb5c8dd4185f9426df13c76947 (master)
commit 9de7fe0a7bf1111ebea9ca9e28b2def9ae3e53ff (release/2.4)
Author: Arne Schwabe
Date:   Sun Oct 7 23:55:39 2018 +0200

     Add support for tls-ciphersuites for TLS 1.3

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


--
kind regards,

Gert Doering