[Openvpn-devel,v3] 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,

Looks mostly good now. One final nit:

On 07-10-18 22:59, 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 e5e4aac2..616c2696 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 ac68f147..0854e106 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -393,6 +393,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)
>  {
> @@ -428,6 +429,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)

The comment below is nicely wrapped not, but the * of the first argument
should be moved to *openssl_ciphers (I though I already complained about
that but can't find it in my previous review), and this line is too long.

> +{
> +    /*
> +     * 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)
>  {
> 

But the remaining points are few and (very) minor, so I'll leave it up
to you and Gert to decide who gets to fix that ;-)

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

-Steffan