[Openvpn-devel] [PATCH] Fix OpenSSL 1.1.1 not using auto ecliptic curve selection

[Openvpn-devel] [PATCH] Fix OpenSSL 1.1.1 not using auto ecliptic curve selection

[Arne Schwabe]

Commit 8a01147ff attempted to avoid calling the deprecated/noop
operation SSL_CTX_set_ecdh_auto by surrounding it with #ifdef.
Unfortunately, that change also made the return; that would exit
the function no longer being compiled when using OpenSSL 1.1.0+.
As consequence OpenVPN with OpenSSL 1.1.0+ would always set
secp384r1 as ecdh curve unless otherwise specified by ecdh

This patch restores the correct/previous behaviour.
---
 src/openvpn/ssl_openssl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 3f0031ff..4b5ca214 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -678,8 +678,11 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
         /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
          * loading */
         SSL_CTX_set_ecdh_auto(ctx->ctx, 1);
-        return;
+
+        /* OpenSSL 1.1.0 and newer have always ecdh auto loading enabled,
+         * so do nothing */
 #endif
+        return;
 #else
         /* For older OpenSSL we have to extract the curve from key on our own */
         EC_KEY *eckey = NULL;
-- 
2.26.0

[Openvpn-devel] [PATCH 2/3] Refactor counting number of element in a : delimited list into function

[Arne Schwabe]

---
 src/openvpn/misc.c        | 18 ++++++++++++++++++
 src/openvpn/misc.h        | 13 +++++++++++++
 src/openvpn/ssl_mbedtls.c | 15 ++-------------
 3 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1931149b..b375451f 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -735,4 +735,22 @@ output_peer_info_env(struct env_set *es, const char *peer_info)
     }
 }
 
+int get_num_elements(const char* string, char delimiter)
+{
+  int string_len = strlen(string);
+
+  ASSERT(0 != string_len);
+
+  int element_count = 1;
+  /* Get number of ciphers */
+  for (int i = 0; i < string_len; i++)
+    {
+      if (string[i] == delimiter)
+        {
+          element_count++;
+        }
+    }
+
+  return element_count;
+}
 #endif /* P2MP_SERVER */
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index 991b7df2..0655b7fe 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -175,4 +175,17 @@ void output_peer_info_env(struct env_set *es, const char *peer_info);
 
 #endif /* P2MP_SERVER */
 
+/**
+ * Counts the number of delimiter in a string and returns
+ * their number +1. This is typically used to find out the
+ * number elements in a cipher string or similar that is separated by : like
+ *
+ *   X25519:secp256r1:X448:secp512r1:secp384r1:brainpoolP384r1
+ *
+ * @param string        the string to work on
+ * @param delimiter     the delimiter to count, typically ':'
+ * @return              number of delimiter found + 1
+ */
+int
+get_num_elements(const char* string, char delimiter);
 #endif /* ifndef MISC_H */
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 0f0b035b..0e17e734 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -289,33 +289,22 @@ void
 tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 {
     char *tmp_ciphers, *tmp_ciphers_orig, *token;
-    int i, cipher_count;
-    int ciphers_len;
 
     if (NULL == ciphers)
     {
         return; /* Nothing to do */
-
     }
-    ciphers_len = strlen(ciphers);
 
     ASSERT(NULL != ctx);
-    ASSERT(0 != ciphers_len);
 
     /* Get number of ciphers */
-    for (i = 0, cipher_count = 1; i < ciphers_len; i++)
-    {
-        if (ciphers[i] == ':')
-        {
-            cipher_count++;
-        }
-    }
+    int cipher_count = get_num_elements(ciphers, ':');
 
     /* Allocate an array for them */
     ALLOC_ARRAY_CLEAR(ctx->allowed_ciphers, int, cipher_count+1)
 
     /* Parse allowed ciphers, getting IDs */
-    i = 0;
+    int i = 0;
     tmp_ciphers_orig = tmp_ciphers = string_alloc(ciphers, NULL);
 
     token = strtok(tmp_ciphers, ":");
-- 
2.26.0

[Openvpn-devel] [PATCH 3/3] Implement tls-groups option to specify eliptic curves/groups

[Arne Schwabe]

OpenSSL 1.1+ by default only allows signatures and key exchange from the
default list of X25519:secp256r1:X448:secp521r1:secp384r1. Since in
TLS1.3 key exchange is independent from the signature/key of the
certificates, allowing all groups per default is not a sensible choice
anymore and the shorter lister is reasonable.

However, when using certificates with exotic curves the signatures of
this certificates will no longer be accepted. This option allows to
modify the list for these corner cases.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 README.ec                    |   7 +--
 doc/openvpn.8                |  37 +++++++++++--
 src/openvpn/openssl_compat.h |   4 ++
 src/openvpn/options.c        |  10 +++-
 src/openvpn/options.h        |   1 +
 src/openvpn/ssl.c            |   6 +++
 src/openvpn/ssl_backend.h    |  10 ++++
 src/openvpn/ssl_mbedtls.c    |  47 ++++++++++++++++
 src/openvpn/ssl_mbedtls.h    |   1 +
 src/openvpn/ssl_openssl.c    | 101 +++++++++++++++++++++++++++--------
 10 files changed, 194 insertions(+), 30 deletions(-)

diff --git a/README.ec b/README.ec
index 32938017..2f830972 100644
--- a/README.ec
+++ b/README.ec
@@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
 used for authentication, the curve used for the server certificate will be used
 for ECDH too. When autodetection fails (e.g. when using RSA certificates)
 OpenVPN lets the crypto library decide if possible, or falls back to the
-secp384r1 curve.
+secp384r1 curve. The list of groups/curves that the crypto library will choose
+from can be set with the --tls-groups <grouplist> configuration.
 
 An administrator can force an OpenVPN/OpenSSL server to use a specific curve
 using the --ecdh-curve <curvename> option with one of the curves listed as
-available by the --show-curves option. Clients will use the same curve as
+available by the --show-groups option. Clients will use the same curve as
 selected by the server.
 
-Note that not all curves listed by --show-curves are available for use with TLS;
+Note that not all curves listed by --show-groups are available for use with TLS;
 in that case connecting will fail with a 'no shared cipher' TLS error.
 
 Authentication (ECDSA)
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index c5b16981..4897fbdb 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -5084,11 +5084,11 @@ 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
+.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
+.B \-\-tls\-cipher
 setting is used.
 
 Use
@@ -5096,6 +5096,8 @@ Use
 to see a list of TLS ciphers supported by your crypto library.
 
 Warning!
+.B \-\-tls\-groups
+,
 .B \-\-tls\-cipher
 and
 .B \-\-tls\-ciphersuites
@@ -5111,6 +5113,33 @@ OpenSSL.
 The default for \-\-tls\-ciphersuites is to use the crypto library's default.
 .\"*********************************************************
 .TP
+.B \-\-tls\-groups l
+A list
+.B l
+of allowable groups/curves in order of preference.
+
+Set the allowed elictipic curves/groups for the TLS session.
+These groups are  allowed to be used in signatures and key exchange.
+
+mbed TLS currently allows all known curves per default.
+
+OpenSSL 1.1+ restricts the list per default to
+"X25519:secp256r1:X448:secp521r1:secp384r1".
+
+If you use certificates that use non-standard curves, you
+might need to add them here. If you do not force the ecdh curve
+by using
+.B \-\-ecdh\-curve
+, the groups for ecdh will also be picked from this list.
+
+OpenVPN maps the curve name secp256r1 to prime256v1 to allow
+specifying the tls-groups option for mbed TLS and OpenSSL.
+
+Warning: this option not only affects eliptic curve certificates
+but also the key exchange in TLS 1.3 and using this option improperly
+will disable TLS 1.3.
+.\"*********************************************************
+.TP
 .B \-\-tls\-cert\-profile profile
 Set the allowed cryptographic algorithms for certificates according to
 .B profile\fN.
@@ -5876,8 +5905,10 @@ engines supported by the OpenSSL library.
 .TP
 .B \-\-show\-curves
 (Standalone)
-Show all available elliptic curves to use with the
+Show all available elliptic groups/curves to use with the
 .B \-\-ecdh\-curve
+and
+.B \-\-tls\-groups
 option.
 .\"*********************************************************
 .SS Generating key material:
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 4ac8f24d..352e79f9 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -807,4 +807,8 @@ SSL_CTX_set_max_proto_version(SSL_CTX *ctx, long tls_ver_max)
 }
 #endif /* SSL_CTX_set_max_proto_version */
 
+#ifndef SSL_CTX_set1_groups
+#define SSL_CTX_set1_groups SSL_CTX_set1_curves
+#endif
+
 #endif /* OPENSSL_COMPAT_H_ */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 5127f653..88f4051e 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7894,7 +7894,7 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_tls_ciphers = true;
     }
-    else if (streq(p[0], "show-curves") && !p[1])
+    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_curves = true;
@@ -7902,6 +7902,9 @@ add_option(struct options *options,
     else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
+        msg(M_WARN, "Consider setting groups/curves in preference with "
+            "tls-groups instead of forcing a specific curve with "
+            "ecdh-curve.");
         options->ecdh_curve = p[1];
     }
     else if (streq(p[0], "tls-server") && !p[1])
@@ -8090,6 +8093,11 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->cipher_list_tls13 = p[1];
     }
+    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+        options->tls_groups = 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 2f1f6faf..3732a3a5 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -537,6 +537,7 @@ struct options
     const char *pkcs12_file;
     const char *cipher_list;
     const char *cipher_list_tls13;
+    const char *tls_groups;
     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 e21279f1..8bd1c86a 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
     tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
 
+    /* Set the allow groups/curves for TLS if we want to override them */
+    if (options->tls_groups)
+    {
+        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
+    }
+
     if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
     {
         goto err;
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index 1c244ece..d95e8320 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
  */
 void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
 
+/**
+ * Set the allowed (eliptic curve) group allowed for signatures and
+ * key exchange.
+ *
+ * @param ctx       TLS context to restrict, must be valid.
+ * @param groups    List of groups that will be allowed, in priority,
+ *                  separated by :
+ */
+void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
+
 /**
  * Check our certificate notBefore and notAfter fields, and warn if the cert is
  * either not yet valid or has expired.  Note that this is a non-fatal error,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 0e17e734..9ce68275 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
             free(ctx->allowed_ciphers);
         }
 
+        if (ctx->groups)
+        {
+            free(ctx->groups);
+        }
+
         CLEAR(*ctx);
 
         ctx->initialised = false;
@@ -342,6 +347,43 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
     }
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(NULL != ctx);
+
+    /* Get number of groups */
+    int groups_count = get_num_elements(groups, ':');
+
+    /* Allocate an array for them */
+    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
+
+    /* Parse allowed ciphers, getting IDs */
+    int i = 0;
+    char *tmp_groups_orig = string_alloc(groups, NULL);
+    char *tmp_groups = tmp_groups_orig;
+
+    const char *token = strsep(&tmp_groups, ":");
+    while (token)
+    {
+        const mbedtls_ecp_curve_info *ci =
+            mbedtls_ecp_curve_info_from_name(token);
+        if (ci == NULL)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            ctx->groups[i] = ci->grp_id;
+            i++;
+        }
+        token = strsep(&tmp_groups, ":");
+    }
+    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
+    free(tmp_groups_orig);
+}
+
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -1042,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
         mbedtls_ssl_conf_ciphersuites(&ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
     }
 
+    if (ssl_ctx->groups)
+    {
+        mbedtls_ssl_conf_curves(&ks_ssl->ssl_config, ssl_ctx->groups);
+    }
+
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
      * unfragmented, and changing that will require thorough review and
      * testing.  Since OpenVPN is not susceptible to BEAST, we can just
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index c1c676dc..d8c366e7 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -105,6 +105,7 @@ struct tls_root_ctx {
 #endif
     struct external_context external_key; /**< External key context */
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_ecp_group_id *groups;                /**< List of allowed groups for this connection */
     mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 4b5ca214..8c04986c 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -557,6 +557,59 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
 #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    /* This method could be as easy as
+     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
+     * but OpenSSL does not like the name secp256r1 for prime256v1
+     * and as this is one of the more important curve to have
+     * the same name for OpenSSL and mbedTLS, we do this dance
+     */
+
+    int groups_count = get_num_elements(groups, ':');
+
+    int *glist;
+    /* Allocate an array for them */
+    ALLOC_ARRAY_CLEAR(glist, int, groups_count);
+
+    /* Parse allowed ciphers, getting IDs */
+    int glistlen = 0;
+    char *tmp_groups_orig = string_alloc(groups, NULL);
+    char *tmp_groups = tmp_groups_orig;
+
+    const char *token = strsep(&tmp_groups, ":");
+    while (token)
+    {
+        if (streq(token, "secp256r1"))
+        {
+            token = "prime256v1";
+        }
+        int nid = OBJ_sn2nid(token);
+
+        if (nid == 0)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            glist[glistlen] = nid;
+            glistlen++;
+        }
+        token = strsep(&tmp_groups, ":");
+    }
+    free(tmp_groups_orig);
+
+    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
+    {
+        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
+                   groups);
+    }
+
+    free(glist);
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -577,7 +630,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 
     if (cert == NULL)
     {
-        goto cleanup; /* Nothing to check if there is no certificate */
+        goto cleanup;     /* Nothing to check if there is no certificate */
     }
 
     ret = X509_cmp_time(X509_get0_notBefore(cert), NULL);
@@ -890,7 +943,7 @@ tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO *bio)
     for (;; )
     {
         cert = NULL;
-        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL)) /* takes ownership of cert */
+        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL))     /* takes ownership of cert */
         {
             break;
         }
@@ -1144,7 +1197,7 @@ openvpn_extkey_rsa_finish(RSA *rsa)
  * interface query
  */
 const char *
-get_rsa_padding_name (const int padding)
+get_rsa_padding_name(const int padding)
 {
     switch (padding)
     {
@@ -1161,14 +1214,14 @@ get_rsa_padding_name (const int padding)
 
 /**
  * Pass the input hash in 'dgst' to management and get the signature back.
-  *
- * @param dgst		hash to be signed
- * @param dgstlen	len of data in dgst
- * @param sig 		On successful return signature is in sig.
- * @param siglen	length of buffer sig
- * @param algorithm	padding/hashing algorithm for the signature
  *
- * @return 		signature length or -1 on error.
+ * @param dgst          hash to be signed
+ * @param dgstlen       len of data in dgst
+ * @param sig           On successful return signature is in sig.
+ * @param siglen        length of buffer sig
+ * @param algorithm     padding/hashing algorithm for the signature
+ *
+ * @return              signature length or -1 on error.
  */
 static int
 get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,
@@ -1210,7 +1263,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,
         return -1;
     }
 
-    ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name (padding));
+    ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name(padding));
 
     return (ret == len) ? ret : -1;
 }
@@ -1266,7 +1319,7 @@ tls_ctx_use_external_rsa_key(struct tls_root_ctx *ctx, EVP_PKEY *pkey)
         goto err;
     }
 
-    RSA_free(rsa); /* doesn't necessarily free, just decrements refcount */
+    RSA_free(rsa);     /* doesn't necessarily free, just decrements refcount */
     return 1;
 
 err:
@@ -1285,7 +1338,7 @@ err:
 }
 
 #if ((OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) \
-     || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
+    || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
     && !defined(OPENSSL_NO_EC)
 
 /* called when EC_KEY is destroyed */
@@ -1394,11 +1447,11 @@ tls_ctx_use_external_ec_key(struct tls_root_ctx *ctx, EVP_PKEY *pkey)
 
     if (!SSL_CTX_use_PrivateKey(ctx->ctx, privkey))
     {
-        ec = NULL; /* avoid double freeing it below */
+        ec = NULL;     /* avoid double freeing it below */
         goto err;
     }
 
-    EVP_PKEY_free(privkey); /* this will down ref privkey and ec */
+    EVP_PKEY_free(privkey);     /* this will down ref privkey and ec */
     return 1;
 
 err:
@@ -1436,7 +1489,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
 
     /* get the public key */
     EVP_PKEY *pkey = X509_get0_pubkey(cert);
-    ASSERT(pkey); /* NULL before SSL_CTX_use_certificate() is called */
+    ASSERT(pkey);     /* NULL before SSL_CTX_use_certificate() is called */
 
     if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA)
     {
@@ -1446,7 +1499,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
         }
     }
 #if ((OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) \
-     || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
+    || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
     && !defined(OPENSSL_NO_EC)
     else if (EVP_PKEY_id(pkey) == EVP_PKEY_EC)
     {
@@ -1690,7 +1743,7 @@ tls_ctx_load_extra_certs(struct tls_root_ctx *ctx, const char *extra_certs_file,
 static FILE *biofp;                            /* GLOBAL */
 static bool biofp_toggle;                      /* GLOBAL */
 static time_t biofp_last_open;                 /* GLOBAL */
-static const int biofp_reopen_interval = 600;  /* GLOBAL */
+static const int biofp_reopen_interval = 600;     /* GLOBAL */
 
 static void
 close_biofp(void)
@@ -1806,9 +1859,9 @@ bio_write(BIO *bio, const uint8_t *data, int size, const char *desc)
 static void
 bio_write_post(const int status, struct buffer *buf)
 {
-    if (status == 1) /* success status return from bio_write? */
+    if (status == 1)     /* success status return from bio_write? */
     {
-        memset(BPTR(buf), 0, BLEN(buf));  /* erase data just written */
+        memset(BPTR(buf), 0, BLEN(buf));     /* erase data just written */
         buf->len = 0;
     }
 }
@@ -2106,8 +2159,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
         crypto_msg(M_FATAL, "Cannot create SSL object");
     }
 
-#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) || \
-    (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <= 0x2090000fL)
+#if (OPENSSL_VERSION_NUMBER < 0x1010000fL)    \
+    || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <= 0x2090000fL)
     STACK_OF(SSL_CIPHER) *sk = SSL_get_ciphers(ssl);
 #else
     STACK_OF(SSL_CIPHER) *sk = SSL_get1_supported_ciphers(ssl);
@@ -2159,7 +2212,9 @@ show_available_curves(void)
     ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
     if (EC_get_builtin_curves(curves, crv_len))
     {
-        printf("Available Elliptic curves:\n");
+        printf("Consider using openssl ecparam -list_curves as\n"
+               "alternative to running this command to this command.");
+        printf("\nAvailable Elliptic curves/groups:\n");
         for (n = 0; n < crv_len; n++)
         {
             const char *sname;
-- 
2.26.0

Re: [Openvpn-devel] [PATCH] Fix OpenSSL 1.1.1 not using auto ecliptic curve selection

[Antonio Quartulli]

Hi,

On 28/03/2020 05:08, Arne Schwabe wrote:
> Commit 8a01147ff attempted to avoid calling the deprecated/noop
> operation SSL_CTX_set_ecdh_auto by surrounding it with #ifdef.
> Unfortunately, that change also made the return; that would exit
> the function no longer being compiled when using OpenSSL 1.1.0+.
> As consequence OpenVPN with OpenSSL 1.1.0+ would always set
> secp384r1 as ecdh curve unless otherwise specified by ecdh
> 
> This patch restores the correct/previous behaviour.
> ---
>  src/openvpn/ssl_openssl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 3f0031ff..4b5ca214 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -678,8 +678,11 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
>          /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
>           * loading */
>          SSL_CTX_set_ecdh_auto(ctx->ctx, 1);
> -        return;
> +
> +        /* OpenSSL 1.1.0 and newer have always ecdh auto loading enabled,
> +         * so do nothing */
>  #endif
> +        return;

my eyes want to fall when seeing this ifdef jungle...but that's another
topic.

The change makes sense, because for ossl >= 1.1.0 we only want to omit
the call to SSL_CTX_set_ecdh_auto() [no-op since 1.1.0], but the
codeflow should remain the same.

>  #else
>          /* For older OpenSSL we have to extract the curve from key on our own */
>          EC_KEY *eckey = NULL;
> 


Acked-by: Antonio Quartulli <antonio@...515...>

-- 
Antonio Quartulli

[Openvpn-devel] [PATCH applied] Re: Fix OpenSSL 1.1.1 not using auto ecliptic curve selection

[Gert Doering]

Your patch has been applied to the master and release/2.4 branch (bugfix).

I've changed the title from "ecliptic" to "elliptic" curves, though :)

Haven't tested, but have stared at the actual change and the surrounding
code a bit, and hope that we can remove 1.0.x support soon... :-)

commit d8ac887c6b1b57a1953ab62058b4aed5d8c11f65 (master)
commit 5ee76a8fab0411c7529c8da9f40ad386433d9a0c (release/2.4)
Author: Arne Schwabe
Date:   Sat Mar 28 05:08:58 2020 +0100

     Fix OpenSSL 1.1.1 not using auto ecliptic curve selection

     Acked-by: Antonio Quartulli <antonio@...515...>
     Message-Id: <20200328040858.16505-1-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19630.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 2/3] Refactor counting number of element in a : delimited list into function

[Antonio Quartulli]

Hi,

On 01/04/2020 12:21, Arne Schwabe wrote:
> ---
>  src/openvpn/misc.c        | 18 ++++++++++++++++++
>  src/openvpn/misc.h        | 13 +++++++++++++
>  src/openvpn/ssl_mbedtls.c | 15 ++-------------
>  3 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 1931149b..b375451f 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -735,4 +735,22 @@ output_peer_info_env(struct env_set *es, const char *peer_info)
>      }
>  }
>  
> +int get_num_elements(const char* string, char delimiter)
> +{
> +  int string_len = strlen(string);
> +
> +  ASSERT(0 != string_len);
> +
> +  int element_count = 1;
> +  /* Get number of ciphers */
> +  for (int i = 0; i < string_len; i++)
> +    {
> +      if (string[i] == delimiter)
> +        {
> +          element_count++;
> +        }
> +    }

while copying this code you are breaking the indentation. note that 2
blanks before the curly brackets. that's nt supposed to be there.


> +
> +  return element_count;
> +}
>  #endif /* P2MP_SERVER */
> diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
> index 991b7df2..0655b7fe 100644
> --- a/src/openvpn/misc.h
> +++ b/src/openvpn/misc.h
> @@ -175,4 +175,17 @@ void output_peer_info_env(struct env_set *es, const char *peer_info);
>  
>  #endif /* P2MP_SERVER */
>  
> +/**
> + * Counts the number of delimiter in a string and returns
> + * their number +1. 

I'd rephrase this sentence as:

Returns the occurrences of 'delimiter' in 'string' +1.

> This is typically used to find out the
> + * number elements in a cipher string or similar that is separated by : like
> + *
> + *   X25519:secp256r1:X448:secp512r1:secp384r1:brainpoolP384r1
> + *
> + * @param string        the string to work on
> + * @param delimiter     the delimiter to count, typically ':'
> + * @return              number of delimiter found + 1

      I'd change to "occurrences of 'delimiter' +1"

> + */
> +int
> +get_num_elements(const char* string, char delimiter);
>  #endif /* ifndef MISC_H */
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 0f0b035b..0e17e734 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -289,33 +289,22 @@ void
>  tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
>  {
>      char *tmp_ciphers, *tmp_ciphers_orig, *token;
> -    int i, cipher_count;
> -    int ciphers_len;
>  
>      if (NULL == ciphers)
>      {
>          return; /* Nothing to do */
> -
>      }
> -    ciphers_len = strlen(ciphers);
>  
>      ASSERT(NULL != ctx);
> -    ASSERT(0 != ciphers_len);
>  
>      /* Get number of ciphers */
> -    for (i = 0, cipher_count = 1; i < ciphers_len; i++)
> -    {
> -        if (ciphers[i] == ':')
> -        {
> -            cipher_count++;
> -        }
> -    }
> +    int cipher_count = get_num_elements(ciphers, ':');
>  
>      /* Allocate an array for them */
>      ALLOC_ARRAY_CLEAR(ctx->allowed_ciphers, int, cipher_count+1)
>  
>      /* Parse allowed ciphers, getting IDs */
> -    i = 0;
> +    int i = 0;
>      tmp_ciphers_orig = tmp_ciphers = string_alloc(ciphers, NULL);
>  
>      token = strtok(tmp_ciphers, ":");
> 

Other than my little nitpicks above, the patch looks good.
However, I have a question.

Since you are refactoring this code and this is going to master/2.5, why
not reimplementing the get_num_elements() function using strtok() ?


Regards,

-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH 3/3] Implement tls-groups option to specify eliptic curves/groups

[Antonio Quartulli]

Hi,

this patch looks pretty simple and easy to digest.

However, there are several style things which are odd. See below:

On 01/04/2020 12:21, Arne Schwabe wrote:
> OpenSSL 1.1+ by default only allows signatures and key exchange from the
> default list of X25519:secp256r1:X448:secp521r1:secp384r1. Since in
> TLS1.3 key exchange is independent from the signature/key of the
> certificates, allowing all groups per default is not a sensible choice
> anymore and the shorter lister is reasonable.
> 
> However, when using certificates with exotic curves the signatures of
> this certificates will no longer be accepted. This option allows to
> modify the list for these corner cases.

there are some errors in the commit message, but they can probably be
fixed at merge time.


> 
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  README.ec                    |   7 +--
>  doc/openvpn.8                |  37 +++++++++++--
>  src/openvpn/openssl_compat.h |   4 ++
>  src/openvpn/options.c        |  10 +++-
>  src/openvpn/options.h        |   1 +
>  src/openvpn/ssl.c            |   6 +++
>  src/openvpn/ssl_backend.h    |  10 ++++
>  src/openvpn/ssl_mbedtls.c    |  47 ++++++++++++++++
>  src/openvpn/ssl_mbedtls.h    |   1 +
>  src/openvpn/ssl_openssl.c    | 101 +++++++++++++++++++++++++++--------
>  10 files changed, 194 insertions(+), 30 deletions(-)
> 
> diff --git a/README.ec b/README.ec
> index 32938017..2f830972 100644
> --- a/README.ec
> +++ b/README.ec
> @@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
>  used for authentication, the curve used for the server certificate will be used
>  for ECDH too. When autodetection fails (e.g. when using RSA certificates)
>  OpenVPN lets the crypto library decide if possible, or falls back to the
> -secp384r1 curve.
> +secp384r1 curve. The list of groups/curves that the crypto library will choose
> +from can be set with the --tls-groups <grouplist> configuration.
>  
>  An administrator can force an OpenVPN/OpenSSL server to use a specific curve
>  using the --ecdh-curve <curvename> option with one of the curves listed as
> -available by the --show-curves option. Clients will use the same curve as
> +available by the --show-groups option. Clients will use the same curve as
>  selected by the server.
>  
> -Note that not all curves listed by --show-curves are available for use with TLS;
> +Note that not all curves listed by --show-groups are available for use with TLS;
>  in that case connecting will fail with a 'no shared cipher' TLS error.
>  
>  Authentication (ECDSA)
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index c5b16981..4897fbdb 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -5084,11 +5084,11 @@ 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
> +.B \-\-tls\-cipher

Why not fixing these issues in a separate patch? (my opinion)

>  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
> +.B \-\-tls\-cipher

same

>  setting is used.
>  
>  Use
> @@ -5096,6 +5096,8 @@ Use
>  to see a list of TLS ciphers supported by your crypto library.
>  
>  Warning!
> +.B \-\-tls\-groups
> +,
>  .B \-\-tls\-cipher
>  and
>  .B \-\-tls\-ciphersuites
> @@ -5111,6 +5113,33 @@ OpenSSL.
>  The default for \-\-tls\-ciphersuites is to use the crypto library's default.
>  .\"*********************************************************
>  .TP
> +.B \-\-tls\-groups l
> +A list
> +.B l
> +of allowable groups/curves in order of preference.
> +
> +Set the allowed elictipic curves/groups for the TLS session.

                       ^^ typ0

> +These groups are  allowed to be used in signatures and key exchange.
> +
> +mbed TLS currently allows all known curves per default.
> +
> +OpenSSL 1.1+ restricts the list per default to
> +"X25519:secp256r1:X448:secp521r1:secp384r1".
> +
> +If you use certificates that use non-standard curves, you
> +might need to add them here. If you do not force the ecdh curve
> +by using
> +.B \-\-ecdh\-curve
> +, the groups for ecdh will also be picked from this list.
> +
> +OpenVPN maps the curve name secp256r1 to prime256v1 to allow
> +specifying the tls-groups option for mbed TLS and OpenSSL.
> +
> +Warning: this option not only affects eliptic curve certificates
> +but also the key exchange in TLS 1.3 and using this option improperly
> +will disable TLS 1.3.
> +.\"*********************************************************
> +.TP
>  .B \-\-tls\-cert\-profile profile
>  Set the allowed cryptographic algorithms for certificates according to
>  .B profile\fN.
> @@ -5876,8 +5905,10 @@ engines supported by the OpenSSL library.
>  .TP
>  .B \-\-show\-curves
>  (Standalone)
> -Show all available elliptic curves to use with the
> +Show all available elliptic groups/curves to use with the
>  .B \-\-ecdh\-curve
> +and
> +.B \-\-tls\-groups
>  option.
>  .\"*********************************************************
>  .SS Generating key material:
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 4ac8f24d..352e79f9 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -807,4 +807,8 @@ SSL_CTX_set_max_proto_version(SSL_CTX *ctx, long tls_ver_max)
>  }
>  #endif /* SSL_CTX_set_max_proto_version */
>  
> +#ifndef SSL_CTX_set1_groups
> +#define SSL_CTX_set1_groups SSL_CTX_set1_curves
> +#endif
> +
>  #endif /* OPENSSL_COMPAT_H_ */
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 5127f653..88f4051e 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -7894,7 +7894,7 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_GENERAL);
>          options->show_tls_ciphers = true;
>      }
> -    else if (streq(p[0], "show-curves") && !p[1])
> +    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL);
>          options->show_curves = true;
> @@ -7902,6 +7902,9 @@ add_option(struct options *options,
>      else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL);
> +        msg(M_WARN, "Consider setting groups/curves in preference with "
                                                       ^ I'd remove 'in'
> +            "tls-groups instead of forcing a specific curve with "
> +            "ecdh-curve.");
>          options->ecdh_curve = p[1];
>      }
>      else if (streq(p[0], "tls-server") && !p[1])
> @@ -8090,6 +8093,11 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_GENERAL);
>          options->cipher_list_tls13 = p[1];
>      }
> +    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
> +    {
> +        VERIFY_PERMISSION(OPT_P_GENERAL);
> +        options->tls_groups = 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 2f1f6faf..3732a3a5 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -537,6 +537,7 @@ struct options
>      const char *pkcs12_file;
>      const char *cipher_list;
>      const char *cipher_list_tls13;
> +    const char *tls_groups;
>      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 e21279f1..8bd1c86a 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
>      tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
>      tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
>  
> +    /* Set the allow groups/curves for TLS if we want to override them */
> +    if (options->tls_groups)
> +    {
> +        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
> +    }
> +
>      if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
>      {
>          goto err;
> diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
> index 1c244ece..d95e8320 100644
> --- a/src/openvpn/ssl_backend.h
> +++ b/src/openvpn/ssl_backend.h
> @@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
>   */
>  void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
>  
> +/**
> + * Set the allowed (eliptic curve) group allowed for signatures and
> + * key exchange.
> + *
> + * @param ctx       TLS context to restrict, must be valid.
> + * @param groups    List of groups that will be allowed, in priority,
> + *                  separated by :
> + */
> +void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
> +
>  /**
>   * Check our certificate notBefore and notAfter fields, and warn if the cert is
>   * either not yet valid or has expired.  Note that this is a non-fatal error,
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 0e17e734..9ce68275 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
>              free(ctx->allowed_ciphers);
>          }
>  
> +        if (ctx->groups)
> +        {
> +            free(ctx->groups);
> +        }
> +
>          CLEAR(*ctx);
>  
>          ctx->initialised = false;
> @@ -342,6 +347,43 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>      }
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(NULL != ctx);

elsewhere (i.e. later in this patch) you use the style ASSERT(cts) - I
suggest to do the same here.

> +
> +    /* Get number of groups */
> +    int groups_count = get_num_elements(groups, ':');
> +
> +    /* Allocate an array for them */
> +    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int i = 0;
> +    char *tmp_groups_orig = string_alloc(groups, NULL);

is it really important to clone "groups" ? why just not chopping it ?
It shouldn't be re-used any more I think.


If we really really need to alloc this memory, can't we get a gc from
outside? maybe from the option parser? We normally avoid explicit
alloc/free in the code.

> +    char *tmp_groups = tmp_groups_orig;
> +
> +    const char *token = strsep(&tmp_groups, ":");
> +    while (token)
> +    {
> +        const mbedtls_ecp_curve_info *ci =
> +            mbedtls_ecp_curve_info_from_name(token);
> +        if (ci == NULL)

we normally just use the style "if (a)" - I suggest to do the same here

> +        {
> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +        }
> +        else
> +        {
> +            ctx->groups[i] = ci->grp_id;
> +            i++;
> +        }
> +        token = strsep(&tmp_groups, ":");
> +    }
> +    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
> +    free(tmp_groups_orig);
> +}
> +
> +
>  void
>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  {
> @@ -1042,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>          mbedtls_ssl_conf_ciphersuites(&ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
>      }
>  
> +    if (ssl_ctx->groups)
> +    {
> +        mbedtls_ssl_conf_curves(&ks_ssl->ssl_config, ssl_ctx->groups);
> +    }
> +
>      /* Disable record splitting (for now).  OpenVPN assumes records are sent
>       * unfragmented, and changing that will require thorough review and
>       * testing.  Since OpenVPN is not susceptible to BEAST, we can just
> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
> index c1c676dc..d8c366e7 100644
> --- a/src/openvpn/ssl_mbedtls.h
> +++ b/src/openvpn/ssl_mbedtls.h
> @@ -105,6 +105,7 @@ struct tls_root_ctx {
>  #endif
>      struct external_context external_key; /**< External key context */
>      int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
> +    mbedtls_ecp_group_id *groups;                /**< List of allowed groups for this connection */
>      mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
>  };
>  
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 4b5ca214..8c04986c 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -557,6 +557,59 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>  #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(ctx);
> +    /* This method could be as easy as
> +     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
> +     * but OpenSSL does not like the name secp256r1 for prime256v1
> +     * and as this is one of the more important curve to have
                                    ^^^ most
> +     * the same name for OpenSSL and mbedTLS, we do this dance

maybe this last sentence could re-arranged a bit ?

> +     */
> +
> +    int groups_count = get_num_elements(groups, ':');
> +
> +    int *glist;
> +    /* Allocate an array for them */

this comment is a bit useless, no ? :D

> +    ALLOC_ARRAY_CLEAR(glist, int, groups_count);
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int glistlen = 0;
> +    char *tmp_groups_orig = string_alloc(groups, NULL);
> +    char *tmp_groups = tmp_groups_orig;
> +
> +    const char *token = strsep(&tmp_groups, ":");
> +    while (token)
> +    {
> +        if (streq(token, "secp256r1"))
> +        {
> +            token = "prime256v1";
> +        }
> +        int nid = OBJ_sn2nid(token);
> +
> +        if (nid == 0)
> +        {
> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +        }
> +        else
> +        {
> +            glist[glistlen] = nid;
> +            glistlen++;
> +        }
> +        token = strsep(&tmp_groups, ":");
> +    }
> +    free(tmp_groups_orig);
> +
> +    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
> +    {
> +        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
> +                   groups);
> +    }
> +
> +    free(glist);
> +}
> +
>  void
>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  {
> @@ -577,7 +630,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  
>      if (cert == NULL)
>      {
> -        goto cleanup; /* Nothing to check if there is no certificate */
> +        goto cleanup;     /* Nothing to check if there is no certificate */

how is this related to this patch?

>      }
>  
>      ret = X509_cmp_time(X509_get0_notBefore(cert), NULL);
> @@ -890,7 +943,7 @@ tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO *bio)
>      for (;; )
>      {
>          cert = NULL;
> -        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL)) /* takes ownership of cert */
> +        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL))     /* takes ownership of cert */

how is this related to this patch?

>          {
>              break;
>          }
> @@ -1144,7 +1197,7 @@ openvpn_extkey_rsa_finish(RSA *rsa)
>   * interface query
>   */
>  const char *
> -get_rsa_padding_name (const int padding)
> +get_rsa_padding_name(const int padding)

how is this related to this patch?

>  {
>      switch (padding)
>      {
> @@ -1161,14 +1214,14 @@ get_rsa_padding_name (const int padding)
>  
>  /**
>   * Pass the input hash in 'dgst' to management and get the signature back.
> -  *
> - * @param dgst		hash to be signed
> - * @param dgstlen	len of data in dgst
> - * @param sig 		On successful return signature is in sig.
> - * @param siglen	length of buffer sig
> - * @param algorithm	padding/hashing algorithm for the signature
>   *
> - * @return 		signature length or -1 on error.
> + * @param dgst          hash to be signed
> + * @param dgstlen       len of data in dgst
> + * @param sig           On successful return signature is in sig.
> + * @param siglen        length of buffer sig
> + * @param algorithm     padding/hashing algorithm for the signature
> + *
> + * @return              signature length or -1 on error.

how is this related to this patch?

>   */
>  static int
>  get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,
> @@ -1210,7 +1263,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,
>          return -1;
>      }
>  
> -    ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name (padding));
> +    ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name(padding));

how is this related to this patch?

>  
>      return (ret == len) ? ret : -1;
>  }
> @@ -1266,7 +1319,7 @@ tls_ctx_use_external_rsa_key(struct tls_root_ctx *ctx, EVP_PKEY *pkey)
>          goto err;
>      }
>  
> -    RSA_free(rsa); /* doesn't necessarily free, just decrements refcount */
> +    RSA_free(rsa);     /* doesn't necessarily free, just decrements refcount */

how is this related to this patch?

>      return 1;
>  
>  err:
> @@ -1285,7 +1338,7 @@ err:
>  }
>  
>  #if ((OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) \
> -     || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
> +    || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \

how is this related to this patch?

>      && !defined(OPENSSL_NO_EC)
>  
>  /* called when EC_KEY is destroyed */
> @@ -1394,11 +1447,11 @@ tls_ctx_use_external_ec_key(struct tls_root_ctx *ctx, EVP_PKEY *pkey)
>  
>      if (!SSL_CTX_use_PrivateKey(ctx->ctx, privkey))
>      {
> -        ec = NULL; /* avoid double freeing it below */
> +        ec = NULL;     /* avoid double freeing it below */

how is this related to this patch?

>          goto err;
>      }
>  
> -    EVP_PKEY_free(privkey); /* this will down ref privkey and ec */
> +    EVP_PKEY_free(privkey);     /* this will down ref privkey and ec */

how is this related to this patch?

>      return 1;
>  
>  err:
> @@ -1436,7 +1489,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
>  
>      /* get the public key */
>      EVP_PKEY *pkey = X509_get0_pubkey(cert);
> -    ASSERT(pkey); /* NULL before SSL_CTX_use_certificate() is called */
> +    ASSERT(pkey);     /* NULL before SSL_CTX_use_certificate() is called */

how is this related to this patch?

>  
>      if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA)
>      {
> @@ -1446,7 +1499,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
>          }
>      }
>  #if ((OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) \
> -     || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
> +    || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \

how is this related to this patch?

>      && !defined(OPENSSL_NO_EC)
>      else if (EVP_PKEY_id(pkey) == EVP_PKEY_EC)
>      {
> @@ -1690,7 +1743,7 @@ tls_ctx_load_extra_certs(struct tls_root_ctx *ctx, const char *extra_certs_file,
>  static FILE *biofp;                            /* GLOBAL */
>  static bool biofp_toggle;                      /* GLOBAL */
>  static time_t biofp_last_open;                 /* GLOBAL */
> -static const int biofp_reopen_interval = 600;  /* GLOBAL */
> +static const int biofp_reopen_interval = 600;     /* GLOBAL */

how is this related to this patch?

>  
>  static void
>  close_biofp(void)
> @@ -1806,9 +1859,9 @@ bio_write(BIO *bio, const uint8_t *data, int size, const char *desc)
>  static void
>  bio_write_post(const int status, struct buffer *buf)
>  {
> -    if (status == 1) /* success status return from bio_write? */
> +    if (status == 1)     /* success status return from bio_write? */

how is this related to this patch?

>      {
> -        memset(BPTR(buf), 0, BLEN(buf));  /* erase data just written */
> +        memset(BPTR(buf), 0, BLEN(buf));     /* erase data just written */

how is this related to this patch?

>          buf->len = 0;
>      }
>  }
> @@ -2106,8 +2159,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
>          crypto_msg(M_FATAL, "Cannot create SSL object");
>      }
>  
> -#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) || \
> -    (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <= 0x2090000fL)
> +#if (OPENSSL_VERSION_NUMBER < 0x1010000fL)    \
> +    || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <= 0x2090000fL)

how is this related to this patch?

>      STACK_OF(SSL_CIPHER) *sk = SSL_get_ciphers(ssl);
>  #else
>      STACK_OF(SSL_CIPHER) *sk = SSL_get1_supported_ciphers(ssl);
> @@ -2159,7 +2212,9 @@ show_available_curves(void)
>      ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
>      if (EC_get_builtin_curves(curves, crv_len))
>      {
> -        printf("Available Elliptic curves:\n");
> +        printf("Consider using openssl ecparam -list_curves as\n"
> +               "alternative to running this command to this command.");
                                                       ^^^^ typ0

> +        printf("\nAvailable Elliptic curves/groups:\n");
>          for (n = 0; n < crv_len; n++)
>          {
>              const char *sname;
> 


it seems like the second half of your patch is made up by changes
created by uncrustify that are unrelated to the actual patch.

how about creating a second patxing all these style things instead of
mixing everything up?


This said, the logic looks good and it does what it claims without
inroducing too much complexity, except for allocating memory that may be
avoided (as asked inline).

my 2 cents.


Regards,


-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH 2/3] Refactor counting number of element in a : delimited list into function

[Arne Schwabe]

[-- Attachment #1.1: Type: text/plain, Size: 776 bytes --]


>>
> 
> Other than my little nitpicks above, the patch looks good.
> However, I have a question.
> 
> Since you are refactoring this code and this is going to master/2.5, why
> not reimplementing the get_num_elements() function using strtok() ?
>

strsep/strok have the disadvantage of modifying the passed string,
requring copying the string. strok in addition breaks on recursive calls.

Also the for loop is very to understand and from playing with godbolt
(https://godbolt.org/z/RVtjVL) it looks that the compilers will
optimise/vectorise this function so using C string function will
probably be even slower. So the simple and easy to understand function
is the best choice here IMHO.

Will send a V2 with the formatting/description fixed.

Arne


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Openvpn-devel] [PATCH v2 2/3] Refactor counting number of element in a : delimited list into function

[Arne Schwabe]

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 src/openvpn/misc.c        | 19 +++++++++++++++++++
 src/openvpn/misc.h        | 14 ++++++++++++++
 src/openvpn/ssl_mbedtls.c | 15 ++-------------
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1c17948c..a768f88d 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -765,4 +765,23 @@ output_peer_info_env(struct env_set *es, const char *peer_info)
     }
 }
 
+int
+get_num_elements(const char *string, char delimiter)
+{
+    int string_len = strlen(string);
+
+    ASSERT(0 != string_len);
+
+    int element_count = 1;
+    /* Get number of ciphers */
+    for (int i = 0; i < string_len; i++)
+    {
+        if (string[i] == delimiter)
+        {
+            element_count++;
+        }
+    }
+
+    return element_count;
+}
 #endif /* P2MP_SERVER */
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index 991b7df2..2605c6d2 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -175,4 +175,18 @@ void output_peer_info_env(struct env_set *es, const char *peer_info);
 
 #endif /* P2MP_SERVER */
 
+/**
+ * Returns the occurrences of 'delimiter' in a string +1
+ * This is typically used to find out the number elements in a
+ * cipher string or similar that is separated by : like
+ *
+ *   X25519:secp256r1:X448:secp512r1:secp384r1:brainpoolP384r1
+ *
+ * @param string        the string to work on
+ * @param delimiter     the delimiter to count, typically ':'
+ * @return              occrrences of delimiter + 1
+ */
+int
+get_num_elements(const char *string, char delimiter);
+
 #endif /* ifndef MISC_H */
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 4f194ad7..51669278 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -289,33 +289,22 @@ void
 tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 {
     char *tmp_ciphers, *tmp_ciphers_orig, *token;
-    int i, cipher_count;
-    int ciphers_len;
 
     if (NULL == ciphers)
     {
         return; /* Nothing to do */
-
     }
-    ciphers_len = strlen(ciphers);
 
     ASSERT(NULL != ctx);
-    ASSERT(0 != ciphers_len);
 
     /* Get number of ciphers */
-    for (i = 0, cipher_count = 1; i < ciphers_len; i++)
-    {
-        if (ciphers[i] == ':')
-        {
-            cipher_count++;
-        }
-    }
+    int cipher_count = get_num_elements(ciphers, ':');
 
     /* Allocate an array for them */
     ALLOC_ARRAY_CLEAR(ctx->allowed_ciphers, int, cipher_count+1)
 
     /* Parse allowed ciphers, getting IDs */
-    i = 0;
+    int i = 0;
     tmp_ciphers_orig = tmp_ciphers = string_alloc(ciphers, NULL);
 
     token = strtok(tmp_ciphers, ":");
-- 
2.26.0

[Openvpn-devel] [PATCH v2 3/3] Implement tls-groups option to specify eliptic curves/groups

[Arne Schwabe]

By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
TLS1.3 key exchange is independent from the signature/key of the
certificates, so allowing all groups per default is not a sensible
choice anymore and instead a shorter list is reasonable.

However, when using certificates with exotic curves that are not on
the group list, the signatures of these certificates will no longer
be accepted.

The tls-groups  option allows to modify the group list to account
for these corner cases.

Patch V2: Uses local gc_arena instead of malloc/free, reword commit
          message. Fix other typos/clarify messages

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 README.ec                 |  7 ++---
 doc/openvpn.8             | 33 ++++++++++++++++++++++-
 src/openvpn/options.c     | 10 ++++++-
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  6 +++++
 src/openvpn/ssl_backend.h | 10 +++++++
 src/openvpn/ssl_mbedtls.c | 46 ++++++++++++++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h |  1 +
 src/openvpn/ssl_openssl.c | 56 ++++++++++++++++++++++++++++++++++++++-
 9 files changed, 164 insertions(+), 6 deletions(-)

diff --git a/README.ec b/README.ec
index 32938017..2f830972 100644
--- a/README.ec
+++ b/README.ec
@@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
 used for authentication, the curve used for the server certificate will be used
 for ECDH too. When autodetection fails (e.g. when using RSA certificates)
 OpenVPN lets the crypto library decide if possible, or falls back to the
-secp384r1 curve.
+secp384r1 curve. The list of groups/curves that the crypto library will choose
+from can be set with the --tls-groups <grouplist> configuration.
 
 An administrator can force an OpenVPN/OpenSSL server to use a specific curve
 using the --ecdh-curve <curvename> option with one of the curves listed as
-available by the --show-curves option. Clients will use the same curve as
+available by the --show-groups option. Clients will use the same curve as
 selected by the server.
 
-Note that not all curves listed by --show-curves are available for use with TLS;
+Note that not all curves listed by --show-groups are available for use with TLS;
 in that case connecting will fail with a 'no shared cipher' TLS error.
 
 Authentication (ECDSA)
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index f0796e52..76633900 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -5098,6 +5098,8 @@ Use
 to see a list of TLS ciphers supported by your crypto library.
 
 Warning!
+.B \-\-tls\-groups
+,
 .B \-\-tls\-cipher
 and
 .B \-\-tls\-ciphersuites
@@ -5113,6 +5115,33 @@ OpenSSL.
 The default for \-\-tls\-ciphersuites is to use the crypto library's default.
 .\"*********************************************************
 .TP
+.B \-\-tls\-groups l
+A list
+.B l
+of allowable groups/curves in order of preference.
+
+Set the allowed elictipic curves/groups for the TLS session.
+These groups are  allowed to be used in signatures and key exchange.
+
+mbed TLS currently allows all known curves per default.
+
+OpenSSL 1.1+ restricts the list per default to
+"X25519:secp256r1:X448:secp521r1:secp384r1".
+
+If you use certificates that use non-standard curves, you
+might need to add them here. If you do not force the ecdh curve
+by using
+.B \-\-ecdh\-curve
+, the groups for ecdh will also be picked from this list.
+
+OpenVPN maps the curve name secp256r1 to prime256v1 to allow
+specifying the tls-groups option for mbed TLS and OpenSSL.
+
+Warning: this option not only affects eliptic curve certificates
+but also the key exchange in TLS 1.3 and using this option improperly
+will disable TLS 1.3.
+.\"*********************************************************
+.TP
 .B \-\-tls\-cert\-profile profile
 Set the allowed cryptographic algorithms for certificates according to
 .B profile\fN.
@@ -5878,8 +5907,10 @@ engines supported by the OpenSSL library.
 .TP
 .B \-\-show\-curves
 (Standalone)
-Show all available elliptic curves to use with the
+Show all available elliptic groups/curves to use with the
 .B \-\-ecdh\-curve
+and
+.B \-\-tls\-groups
 option.
 .\"*********************************************************
 .SS Generating key material:
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 49df8df1..57bc0abb 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7895,7 +7895,7 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_tls_ciphers = true;
     }
-    else if (streq(p[0], "show-curves") && !p[1])
+    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_curves = true;
@@ -7903,6 +7903,9 @@ add_option(struct options *options,
     else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
+        msg(M_WARN, "Consider setting groups/curves preference with "
+            "tls-groups instead of forcing a specific curve with "
+            "ecdh-curve.");
         options->ecdh_curve = p[1];
     }
     else if (streq(p[0], "tls-server") && !p[1])
@@ -8091,6 +8094,11 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->cipher_list_tls13 = p[1];
     }
+    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+        options->tls_groups = 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 2f1f6faf..3732a3a5 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -537,6 +537,7 @@ struct options
     const char *pkcs12_file;
     const char *cipher_list;
     const char *cipher_list_tls13;
+    const char *tls_groups;
     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 56d0576a..ef153d37 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
     tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
 
+    /* Set the allow groups/curves for TLS if we want to override them */
+    if (options->tls_groups)
+    {
+        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
+    }
+
     if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
     {
         goto err;
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index 1c244ece..d95e8320 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
  */
 void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
 
+/**
+ * Set the allowed (eliptic curve) group allowed for signatures and
+ * key exchange.
+ *
+ * @param ctx       TLS context to restrict, must be valid.
+ * @param groups    List of groups that will be allowed, in priority,
+ *                  separated by :
+ */
+void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
+
 /**
  * Check our certificate notBefore and notAfter fields, and warn if the cert is
  * either not yet valid or has expired.  Note that this is a non-fatal error,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 51669278..f133ae39 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
             free(ctx->allowed_ciphers);
         }
 
+        if (ctx->groups)
+        {
+            free(ctx->groups);
+        }
+
         CLEAR(*ctx);
 
         ctx->initialised = false;
@@ -342,6 +347,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
     }
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+
+    /* Get number of groups and allocate an array in ctx */
+    int groups_count = get_num_elements(groups, ':');
+    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
+
+    /* Parse allowed ciphers, getting IDs */
+    int i = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token = strsep(&tmp_groups, ":");
+    while (token)
+    {
+        const mbedtls_ecp_curve_info *ci =
+            mbedtls_ecp_curve_info_from_name(token);
+        if (!ci)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            ctx->groups[i] = ci->grp_id;
+            i++;
+        }
+        token = strsep(&tmp_groups, ":");
+    }
+    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
+
+    gc_free(&gc);
+}
+
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
         mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
     }
 
+    if (ssl_ctx->groups)
+    {
+        mbedtls_ssl_conf_curves(&ks_ssl->ssl_config, ssl_ctx->groups);
+    }
+
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
      * unfragmented, and changing that will require thorough review and
      * testing.  Since OpenVPN is not susceptible to BEAST, we can just
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index 92381f1a..1dc28313 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -105,6 +105,7 @@ struct tls_root_ctx {
 #endif
     struct external_context external_key; /**< External key context */
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_ecp_group_id *groups;                /**< List of allowed groups for this connection */
     mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index d7bd6aa2..06971d63 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -557,6 +557,58 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
 #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+    /* This method could be as easy as
+     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
+     * but OpenSSL does not like the name secp256r1 for prime256v1
+     * This is one of the important curves.
+     * To support the same name for OpenSSL and mbedTLS, we do
+     * this dance.
+     */
+
+    int groups_count = get_num_elements(groups, ':');
+
+    int *glist;
+    /* Allocate an array for them */
+    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
+
+    /* Parse allowed ciphers, getting IDs */
+    int glistlen = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token = strsep(&tmp_groups, ":");
+    while (token)
+    {
+        if (streq(token, "secp256r1"))
+        {
+            token = "prime256v1";
+        }
+        int nid = OBJ_sn2nid(token);
+
+        if (nid == 0)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            glist[glistlen] = nid;
+            glistlen++;
+        }
+        token = strsep(&tmp_groups, ":");
+    }
+
+    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
+    {
+        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
+                   groups);
+    }
+    gc_free(&gc);
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -2179,6 +2231,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
 void
 show_available_curves(void)
 {
+    printf("Consider using openssl ecparam -list_curves as\n"
+           "alternative to running this command.");
 #ifndef OPENSSL_NO_EC
     EC_builtin_curve *curves = NULL;
     size_t crv_len = 0;
@@ -2188,7 +2242,7 @@ show_available_curves(void)
     ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
     if (EC_get_builtin_curves(curves, crv_len))
     {
-        printf("Available Elliptic curves:\n");
+        printf("\nAvailable Elliptic curves/groups:\n");
         for (n = 0; n < crv_len; n++)
         {
             const char *sname;
-- 
2.26.0

Re: [Openvpn-devel] [PATCH 3/3] Implement tls-groups option to specify eliptic curves/groups

[Arne Schwabe]

[-- Attachment #1.1: Type: text/plain, Size: 758 bytes --]

Am 16.04.20 um 00:31 schrieb Antonio Quartulli:
> is it really important to clone "groups" ? why just not chopping it ?
> It shouldn't be re-used any more I think.

Maybe but modifying a parameter as side effect does not feel right. I
rather have a clean API here instead of this unintended side stuff that
can lead to kind of interesting bugs.

> If we really really need to alloc this memory, can't we get a gc from
> outside? maybe from the option parser? We normally avoid explicit
> alloc/free in the code.

The problem with a gc from the outside is that we don't really want to
tie up the memory in options->gc as that would just be extra memory that
is allocated the whole session I will update the patch to use a local gc.


Arne


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Openvpn-devel] [PATCH 3/3] Implement tls-groups option to specify eliptic curves/groups

[Antonio Quartulli]

Hi,

On 17/04/2020 09:47, Arne Schwabe wrote:
> Am 16.04.20 um 00:31 schrieb Antonio Quartulli:
>> is it really important to clone "groups" ? why just not chopping it ?
>> It shouldn't be re-used any more I think.
> 
> Maybe but modifying a parameter as side effect does not feel right. I
> rather have a clean API here instead of this unintended side stuff that
> can lead to kind of interesting bugs.

yeah, makes sense.

> 
>> If we really really need to alloc this memory, can't we get a gc from
>> outside? maybe from the option parser? We normally avoid explicit
>> alloc/free in the code.
> 
> The problem with a gc from the outside is that we don't really want to
> tie up the memory in options->gc as that would just be extra memory that
> is allocated the whole session I will update the patch to use a local gc.
> 

yeah, that would fit our style I think.

Thanks!

-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH v2 2/3] Refactor counting number of element in a : delimited list into function

[Antonio Quartulli]

Hi,

On 16/04/2020 17:26, Arne Schwabe wrote:
> Signed-off-by: Arne Schwabe <arne@...1227...>

Patch looks good and does what it says.

I tested the parser to make sure it still parses the ciphers as
expected, and it does:

./openvpn --tls-cipher
TLS-RSA-PSK-WITH-CHACHA20-POLY1305-SHA256:TLS-RSA-PSK-WITH-AES-256-GCM-SHA384:TLS-RSA-PSK-WITH-AES-256-CBC-SHA384
--show-tls
Available TLS Ciphers, listed in order of preference:

For TLS 1.2 and older (--tls-cipher):

TLS-RSA-PSK-WITH-CHACHA20-POLY1305-SHA256
TLS-RSA-PSK-WITH-AES-256-GCM-SHA384
TLS-RSA-PSK-WITH-AES-256-CBC-SHA384

So all good imho.

Acked-by: Antonio Quartulli <antonio@...515...>

Regards,


> ---
>  src/openvpn/misc.c        | 19 +++++++++++++++++++
>  src/openvpn/misc.h        | 14 ++++++++++++++
>  src/openvpn/ssl_mbedtls.c | 15 ++-------------
>  3 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 1c17948c..a768f88d 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -765,4 +765,23 @@ output_peer_info_env(struct env_set *es, const char *peer_info)
>      }
>  }
>  
> +int
> +get_num_elements(const char *string, char delimiter)
> +{
> +    int string_len = strlen(string);
> +
> +    ASSERT(0 != string_len);
> +
> +    int element_count = 1;
> +    /* Get number of ciphers */
> +    for (int i = 0; i < string_len; i++)
> +    {
> +        if (string[i] == delimiter)
> +        {
> +            element_count++;
> +        }
> +    }
> +
> +    return element_count;
> +}
>  #endif /* P2MP_SERVER */
> diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
> index 991b7df2..2605c6d2 100644
> --- a/src/openvpn/misc.h
> +++ b/src/openvpn/misc.h
> @@ -175,4 +175,18 @@ void output_peer_info_env(struct env_set *es, const char *peer_info);
>  
>  #endif /* P2MP_SERVER */
>  
> +/**
> + * Returns the occurrences of 'delimiter' in a string +1
> + * This is typically used to find out the number elements in a
> + * cipher string or similar that is separated by : like
> + *
> + *   X25519:secp256r1:X448:secp512r1:secp384r1:brainpoolP384r1
> + *
> + * @param string        the string to work on
> + * @param delimiter     the delimiter to count, typically ':'
> + * @return              occrrences of delimiter + 1
> + */
> +int
> +get_num_elements(const char *string, char delimiter);
> +
>  #endif /* ifndef MISC_H */
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 4f194ad7..51669278 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -289,33 +289,22 @@ void
>  tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
>  {
>      char *tmp_ciphers, *tmp_ciphers_orig, *token;
> -    int i, cipher_count;
> -    int ciphers_len;
>  
>      if (NULL == ciphers)
>      {
>          return; /* Nothing to do */
> -
>      }
> -    ciphers_len = strlen(ciphers);
>  
>      ASSERT(NULL != ctx);
> -    ASSERT(0 != ciphers_len);
>  
>      /* Get number of ciphers */
> -    for (i = 0, cipher_count = 1; i < ciphers_len; i++)
> -    {
> -        if (ciphers[i] == ':')
> -        {
> -            cipher_count++;
> -        }
> -    }
> +    int cipher_count = get_num_elements(ciphers, ':');
>  
>      /* Allocate an array for them */
>      ALLOC_ARRAY_CLEAR(ctx->allowed_ciphers, int, cipher_count+1)
>  
>      /* Parse allowed ciphers, getting IDs */
> -    i = 0;
> +    int i = 0;
>      tmp_ciphers_orig = tmp_ciphers = string_alloc(ciphers, NULL);
>  
>      token = strtok(tmp_ciphers, ":");
> 

-- 
Antonio Quartulli

[Openvpn-devel] [PATCH applied] Re: Refactor counting number of element in a : delimited list into function

[Gert Doering]

Your patch has been applied to the master branch.

Nothing to add to Antonio's review :)

commit c577facffb09046da90c52f3ed1af5bdf7b25888
Author: Arne Schwabe
Date:   Thu Apr 16 17:26:18 2020 +0200

     Refactor counting number of element in a : delimited list into function

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Acked-by: Antonio Quartulli <antonio@...515...>
     Message-Id: <20200416152619.5465-1-arne@...1227...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19757.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH v2 3/3] Implement tls-groups option to specify eliptic curves/groups

[Antonio Quartulli]

Hi,

On 16/04/2020 17:26, Arne Schwabe wrote:
> By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
> default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
> TLS1.3 key exchange is independent from the signature/key of the
> certificates, so allowing all groups per default is not a sensible
> choice anymore and instead a shorter list is reasonable.
> 
> However, when using certificates with exotic curves that are not on
> the group list, the signatures of these certificates will no longer
> be accepted.
> 
> The tls-groups  option allows to modify the group list to account
> for these corner cases.
> 
> Patch V2: Uses local gc_arena instead of malloc/free, reword commit
>           message. Fix other typos/clarify messages
> 
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
>  README.ec                 |  7 ++---
>  doc/openvpn.8             | 33 ++++++++++++++++++++++-
>  src/openvpn/options.c     | 10 ++++++-
>  src/openvpn/options.h     |  1 +
>  src/openvpn/ssl.c         |  6 +++++
>  src/openvpn/ssl_backend.h | 10 +++++++
>  src/openvpn/ssl_mbedtls.c | 46 ++++++++++++++++++++++++++++++++
>  src/openvpn/ssl_mbedtls.h |  1 +
>  src/openvpn/ssl_openssl.c | 56 ++++++++++++++++++++++++++++++++++++++-
>  9 files changed, 164 insertions(+), 6 deletions(-)
> 
> diff --git a/README.ec b/README.ec
> index 32938017..2f830972 100644
> --- a/README.ec
> +++ b/README.ec
> @@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
>  used for authentication, the curve used for the server certificate will be used
>  for ECDH too. When autodetection fails (e.g. when using RSA certificates)
>  OpenVPN lets the crypto library decide if possible, or falls back to the
> -secp384r1 curve.
> +secp384r1 curve. The list of groups/curves that the crypto library will choose
> +from can be set with the --tls-groups <grouplist> configuration.
                                  I'd use "config option" ^^^^^
>  
>  An administrator can force an OpenVPN/OpenSSL server to use a specific curve
>  using the --ecdh-curve <curvename> option with one of the curves listed as
> -available by the --show-curves option. Clients will use the same curve as
> +available by the --show-groups option. Clients will use the same curve as
>  selected by the server.
>  
> -Note that not all curves listed by --show-curves are available for use with TLS;
> +Note that not all curves listed by --show-groups are available for use with TLS;
>  in that case connecting will fail with a 'no shared cipher' TLS error.
>  
>  Authentication (ECDSA)
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index f0796e52..76633900 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -5098,6 +5098,8 @@ Use
>  to see a list of TLS ciphers supported by your crypto library.
>  
>  Warning!
> +.B \-\-tls\-groups
> +,
>  .B \-\-tls\-cipher
>  and
>  .B \-\-tls\-ciphersuites
> @@ -5113,6 +5115,33 @@ OpenSSL.
>  The default for \-\-tls\-ciphersuites is to use the crypto library's default.
>  .\"*********************************************************
>  .TP
> +.B \-\-tls\-groups l
> +A list
> +.B l
> +of allowable groups/curves in order of preference.
      ^^^ shouldn't this be "allowed" ?
> +
> +Set the allowed elictipic curves/groups for the TLS session.
> +These groups are  allowed to be used in signatures and key exchange.
> +
> +mbed TLS currently allows all known curves per default.
> +
> +OpenSSL 1.1+ restricts the list per default to
> +"X25519:secp256r1:X448:secp521r1:secp384r1".
> +
> +If you use certificates that use non-standard curves, you
> +might need to add them here. If you do not force the ecdh curve
> +by using
> +.B \-\-ecdh\-curve
> +, the groups for ecdh will also be picked from this list.
> +
> +OpenVPN maps the curve name secp256r1 to prime256v1 to allow
> +specifying the tls-groups option for mbed TLS and OpenSSL.
                 ^ add "same" here to make the sentence more clear

> +
> +Warning: this option not only affects eliptic curve certificates
> +but also the key exchange in TLS 1.3 and using this option improperly
> +will disable TLS 1.3.
> +.\"*********************************************************
> +.TP
>  .B \-\-tls\-cert\-profile profile
>  Set the allowed cryptographic algorithms for certificates according to
>  .B profile\fN.
> @@ -5878,8 +5907,10 @@ engines supported by the OpenSSL library.
>  .TP
>  .B \-\-show\-curves
>  (Standalone)
> -Show all available elliptic curves to use with the
> +Show all available elliptic groups/curves to use with the
>  .B \-\-ecdh\-curve
> +and
> +.B \-\-tls\-groups
>  option.
>  .\"*********************************************************
>  .SS Generating key material:
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 49df8df1..57bc0abb 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -7895,7 +7895,7 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_GENERAL);
>          options->show_tls_ciphers = true;
>      }
> -    else if (streq(p[0], "show-curves") && !p[1])
> +    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
shouldn't we deprecate --show-curves at this point?
what's the point of keeping both options?

>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL);
>          options->show_curves = true;
> @@ -7903,6 +7903,9 @@ add_option(struct options *options,
>      else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL);
> +        msg(M_WARN, "Consider setting groups/curves preference with "
> +            "tls-groups instead of forcing a specific curve with "
> +            "ecdh-curve.");
>          options->ecdh_curve = p[1];
>      }
>      else if (streq(p[0], "tls-server") && !p[1])
> @@ -8091,6 +8094,11 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_GENERAL);
>          options->cipher_list_tls13 = p[1];
>      }
> +    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
> +    {
> +        VERIFY_PERMISSION(OPT_P_GENERAL);
> +        options->tls_groups = 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 2f1f6faf..3732a3a5 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -537,6 +537,7 @@ struct options
>      const char *pkcs12_file;
>      const char *cipher_list;
>      const char *cipher_list_tls13;
> +    const char *tls_groups;
>      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 56d0576a..ef153d37 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
>      tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
>      tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
>  
> +    /* Set the allow groups/curves for TLS if we want to override them */
> +    if (options->tls_groups)
> +    {
> +        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
> +    }
> +
>      if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
>      {
>          goto err;
> diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
> index 1c244ece..d95e8320 100644
> --- a/src/openvpn/ssl_backend.h
> +++ b/src/openvpn/ssl_backend.h
> @@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
>   */
>  void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
>  
> +/**
> + * Set the allowed (eliptic curve) group allowed for signatures and
> + * key exchange.
> + *
> + * @param ctx       TLS context to restrict, must be valid.
> + * @param groups    List of groups that will be allowed, in priority,
> + *                  separated by :
> + */
> +void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
> +
>  /**
>   * Check our certificate notBefore and notAfter fields, and warn if the cert is
>   * either not yet valid or has expired.  Note that this is a non-fatal error,
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 51669278..f133ae39 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
>              free(ctx->allowed_ciphers);
>          }
>  
> +        if (ctx->groups)
> +        {
> +            free(ctx->groups);
> +        }
> +
>          CLEAR(*ctx);
>  
>          ctx->initialised = false;
> @@ -342,6 +347,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>      }
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(ctx);
> +    struct gc_arena gc = gc_new();
> +
> +    /* Get number of groups and allocate an array in ctx */
> +    int groups_count = get_num_elements(groups, ':');
> +    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int i = 0;
> +    char *tmp_groups = string_alloc(groups, &gc);
> +
> +    const char *token = strsep(&tmp_groups, ":");
> +    while (token)

Couldn't we avoid having the assignment here and at the end of the loop
by doing:

const char *token;
while ((token = strsep(&tmp_groups, ":"))

?

> +    {
> +        const mbedtls_ecp_curve_info *ci =
> +            mbedtls_ecp_curve_info_from_name(token);
> +        if (!ci)
> +        {
> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +        }
> +        else
> +        {
> +            ctx->groups[i] = ci->grp_id;
> +            i++;
> +        }
> +        token = strsep(&tmp_groups, ":");
> +    }
> +    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
> +
> +    gc_free(&gc);
> +}
> +
> +
>  void
>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  {
> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>          mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
>      }
>  
> +    if (ssl_ctx->groups)
> +    {
> +        mbedtls_ssl_conf_curves(&ks_ssl->ssl_config, ssl_ctx->groups);

the '&' should not be there I guess.

/usr/include/mbedtls/ssl.h:2925:51: note: expected ‘mbedtls_ssl_config
*’ but argument is of type ‘mbedtls_ssl_config **’


> +    }
> +
>      /* Disable record splitting (for now).  OpenVPN assumes records are sent
>       * unfragmented, and changing that will require thorough review and
>       * testing.  Since OpenVPN is not susceptible to BEAST, we can just
> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
> index 92381f1a..1dc28313 100644
> --- a/src/openvpn/ssl_mbedtls.h
> +++ b/src/openvpn/ssl_mbedtls.h
> @@ -105,6 +105,7 @@ struct tls_root_ctx {
>  #endif
>      struct external_context external_key; /**< External key context */
>      int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
> +    mbedtls_ecp_group_id *groups;                /**< List of allowed groups for this connection */

Maybe you can reduce the space before the comment? :-D

>      mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
>  };
>  
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index d7bd6aa2..06971d63 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -557,6 +557,58 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>  #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(ctx);
> +    struct gc_arena gc = gc_new();
> +    /* This method could be as easy as
> +     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
> +     * but OpenSSL does not like the name secp256r1 for prime256v1
> +     * This is one of the important curves.
> +     * To support the same name for OpenSSL and mbedTLS, we do
> +     * this dance.
> +     */
> +
> +    int groups_count = get_num_elements(groups, ':');
> +
> +    int *glist;
> +    /* Allocate an array for them */
> +    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int glistlen = 0;
> +    char *tmp_groups = string_alloc(groups, &gc);
> +
> +    const char *token = strsep(&tmp_groups, ":");
> +    while (token)

same suggestion as above

> +    {
> +        if (streq(token, "secp256r1"))
> +        {
> +            token = "prime256v1";
> +        }
> +        int nid = OBJ_sn2nid(token);
> +
> +        if (nid == 0)
> +        {
> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +        }
> +        else
> +        {
> +            glist[glistlen] = nid;
> +            glistlen++;
> +        }
> +        token = strsep(&tmp_groups, ":");
> +    }
> +
> +    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
> +    {
> +        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
> +                   groups);
> +    }
> +    gc_free(&gc);
> +}
> +
>  void
>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  {
> @@ -2179,6 +2231,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
>  void
>  show_available_curves(void)
>  {
> +    printf("Consider using openssl ecparam -list_curves as\n"
> +           "alternative to running this command.");

How about putting single quotes around the command? At first I thought
that "as" was also part of the openssl options :-D


>  #ifndef OPENSSL_NO_EC
>      EC_builtin_curve *curves = NULL;
>      size_t crv_len = 0;
> @@ -2188,7 +2242,7 @@ show_available_curves(void)
>      ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
>      if (EC_get_builtin_curves(curves, crv_len))
>      {
> -        printf("Available Elliptic curves:\n");
> +        printf("\nAvailable Elliptic curves/groups:\n");
>          for (n = 0; n < crv_len; n++)
>          {
>              const char *sname;
> 


The rest looks good.

I applied this patch on top of master with git am -3 because there are
some trivial conflicts to fix.


Cheers,

-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH v2 3/3] Implement tls-groups option to specify eliptic curves/groups

[Arne Schwabe]

[-- Attachment #1.1: Type: text/plain, Size: 3785 bytes --]

>>  The default for \-\-tls\-ciphersuites is to use the crypto library's default.
>>  .\"*********************************************************
>>  .TP
>> +.B \-\-tls\-groups l
>> +A list
>> +.B l
>> +of allowable groups/curves in order of preference.
>       ^^^ shouldn't this be "allowed" ?

It is a very tiny semantic difference and both forms are correct. We use
the same wording for tls-cipher and considering that we ignore unknown
curves allowable is IMO the better fit. I prefer allowable but will not
fight for it.


>>          options->show_tls_ciphers = true;
>>      }
>> -    else if (streq(p[0], "show-curves") && !p[1])
>> +    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
> shouldn't we deprecate --show-curves at this point?
> what's the point of keeping both options?

It is a nice thing to and does not extra complexity. So still accepting
is a thing gesture for users.


>> +void
>> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
>> +{
>> +    ASSERT(ctx);
>> +    struct gc_arena gc = gc_new();
>> +
>> +    /* Get number of groups and allocate an array in ctx */
>> +    int groups_count = get_num_elements(groups, ':');
>> +    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
>> +
>> +    /* Parse allowed ciphers, getting IDs */
>> +    int i = 0;
>> +    char *tmp_groups = string_alloc(groups, &gc);
>> +
>> +    const char *token = strsep(&tmp_groups, ":");
>> +    while (token)
> 
> Couldn't we avoid having the assignment here and at the end of the loop
> by doing:
> 
> const char *token;
> while ((token = strsep(&tmp_groups, ":"))
> 

Yes, this is a left over from an earlier version where the first call
had to be made differently iirc.

> 
>> +    {
>> +        const mbedtls_ecp_curve_info *ci =
>> +            mbedtls_ecp_curve_info_from_name(token);
>> +        if (!ci)
>> +        {
>> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
>> +        }
>> +        else
>> +        {
>> +            ctx->groups[i] = ci->grp_id;
>> +            i++;
>> +        }
>> +        token = strsep(&tmp_groups, ":");
>> +    }
>> +    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
>> +
>> +    gc_free(&gc);
>> +}
>> +
>> +
>>  void
>>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>>  {
>> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>>          mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
>>      }
>>  
>> +    if (ssl_ctx->groups)
>> +    {
>> +        mbedtls_ssl_conf_curves(&ks_ssl->ssl_config, ssl_ctx->groups);
> 
> the '&' should not be there I guess.
> 
> /usr/include/mbedtls/ssl.h:2925:51: note: expected ‘mbedtls_ssl_config
> *’ but argument is of type ‘mbedtls_ssl_config **’

The commit mbedTLS: Make sure TLS session survives was commited in the
meantime that change the argument from * to **

https://github.com/openvpn/openvpn/commit/a59e0754afd37a606d96cf24cea771ace3467289


>>  #ifndef OPENSSL_NO_EC
>>      EC_builtin_curve *curves = NULL;
>>      size_t crv_len = 0;
>> @@ -2188,7 +2242,7 @@ show_available_curves(void)
>>      ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
>>      if (EC_get_builtin_curves(curves, crv_len))
>>      {
>> -        printf("Available Elliptic curves:\n");
>> +        printf("\nAvailable Elliptic curves/groups:\n");
>>          for (n = 0; n < crv_len; n++)
>>          {
>>              const char *sname;
>>
> 
> 
> The rest looks good.
> 
> I applied this patch on top of master with git am -3 because there are
> some trivial conflicts to fix.


Will send a V3 with the changes.

Arne


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Openvpn-devel] [PATCH v3 3/3] Implement tls-groups option to specify eliptic curves/groups

[Arne Schwabe]

By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
TLS1.3 key exchange is independent from the signature/key of the
certificates, so allowing all groups per default is not a sensible
choice anymore and instead a shorter list is reasonable.

However, when using certificates with exotic curves that are not on
the group list, the signatures of these certificates will no longer
be accepted.

The tls-groups  option allows to modify the group list to account
for these corner cases.

Patch V2: Uses local gc_arena instead of malloc/free, reword commit
          message. Fix other typos/clarify messages

Patch V3: Style fixes, adjust code to changes from mbed tls session
          fix

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 README.ec                 |  7 ++---
 doc/openvpn.8             | 33 ++++++++++++++++++++++-
 src/openvpn/options.c     | 10 ++++++-
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  6 +++++
 src/openvpn/ssl_backend.h | 10 +++++++
 src/openvpn/ssl_mbedtls.c | 46 ++++++++++++++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h |  1 +
 src/openvpn/ssl_openssl.c | 55 ++++++++++++++++++++++++++++++++++++++-
 9 files changed, 163 insertions(+), 6 deletions(-)

diff --git a/README.ec b/README.ec
index 32938017..61f23b2e 100644
--- a/README.ec
+++ b/README.ec
@@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
 used for authentication, the curve used for the server certificate will be used
 for ECDH too. When autodetection fails (e.g. when using RSA certificates)
 OpenVPN lets the crypto library decide if possible, or falls back to the
-secp384r1 curve.
+secp384r1 curve. The list of groups/curves that the crypto library will choose
+from can be set with the --tls-groups <grouplist> option.
 
 An administrator can force an OpenVPN/OpenSSL server to use a specific curve
 using the --ecdh-curve <curvename> option with one of the curves listed as
-available by the --show-curves option. Clients will use the same curve as
+available by the --show-groups option. Clients will use the same curve as
 selected by the server.
 
-Note that not all curves listed by --show-curves are available for use with TLS;
+Note that not all curves listed by --show-groups are available for use with TLS;
 in that case connecting will fail with a 'no shared cipher' TLS error.
 
 Authentication (ECDSA)
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index dcc72abe..f35a4f64 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -5107,6 +5107,8 @@ Use
 to see a list of TLS ciphers supported by your crypto library.
 
 Warning!
+.B \-\-tls\-groups
+,
 .B \-\-tls\-cipher
 and
 .B \-\-tls\-ciphersuites
@@ -5122,6 +5124,33 @@ OpenSSL.
 The default for \-\-tls\-ciphersuites is to use the crypto library's default.
 .\"*********************************************************
 .TP
+.B \-\-tls\-groups l
+A list
+.B l
+of allowable groups/curves in order of preference.
+
+Set the allowed elictipic curves/groups for the TLS session.
+These groups are  allowed to be used in signatures and key exchange.
+
+mbed TLS currently allows all known curves per default.
+
+OpenSSL 1.1+ restricts the list per default to
+"X25519:secp256r1:X448:secp521r1:secp384r1".
+
+If you use certificates that use non-standard curves, you
+might need to add them here. If you do not force the ecdh curve
+by using
+.B \-\-ecdh\-curve
+, the groups for ecdh will also be picked from this list.
+
+OpenVPN maps the curve name secp256r1 to prime256v1 to allow
+specifying the same tls-groups option for mbed TLS and OpenSSL.
+
+Warning: this option not only affects eliptic curve certificates
+but also the key exchange in TLS 1.3 and using this option improperly
+will disable TLS 1.3.
+.\"*********************************************************
+.TP
 .B \-\-tls\-cert\-profile profile
 Set the allowed cryptographic algorithms for certificates according to
 .B profile\fN.
@@ -5887,8 +5916,10 @@ engines supported by the OpenSSL library.
 .TP
 .B \-\-show\-curves
 (Standalone)
-Show all available elliptic curves to use with the
+Show all available elliptic groups/curves to use with the
 .B \-\-ecdh\-curve
+and
+.B \-\-tls\-groups
 option.
 .\"*********************************************************
 .SS Generating key material:
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 6c0fc0ed..d1e68a51 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7929,7 +7929,7 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_tls_ciphers = true;
     }
-    else if (streq(p[0], "show-curves") && !p[1])
+    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_curves = true;
@@ -7937,6 +7937,9 @@ add_option(struct options *options,
     else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
+        msg(M_WARN, "Consider setting groups/curves preference with "
+            "tls-groups instead of forcing a specific curve with "
+            "ecdh-curve.");
         options->ecdh_curve = p[1];
     }
     else if (streq(p[0], "tls-server") && !p[1])
@@ -8107,6 +8110,11 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->cipher_list_tls13 = p[1];
     }
+    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+        options->tls_groups = p[1];
+    }
     else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
                                                    || !p[2]))
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 375a4fc9..a6a5ba1b 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -541,6 +541,7 @@ struct options
     bool pkcs12_file_inline;
     const char *cipher_list;
     const char *cipher_list_tls13;
+    const char *tls_groups;
     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 c8b3a959..a7bd392a 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
     tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
 
+    /* Set the allow groups/curves for TLS if we want to override them */
+    if (options->tls_groups)
+    {
+        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
+    }
+
     if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
     {
         goto err;
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index a1770bd4..75692797 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
  */
 void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
 
+/**
+ * Set the allowed (eliptic curve) group allowed for signatures and
+ * key exchange.
+ *
+ * @param ctx       TLS context to restrict, must be valid.
+ * @param groups    List of groups that will be allowed, in priority,
+ *                  separated by :
+ */
+void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
+
 /**
  * Check our certificate notBefore and notAfter fields, and warn if the cert is
  * either not yet valid or has expired.  Note that this is a non-fatal error,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index f518f593..d74318f9 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
             free(ctx->allowed_ciphers);
         }
 
+        if (ctx->groups)
+        {
+            free(ctx->groups);
+        }
+
         CLEAR(*ctx);
 
         ctx->initialised = false;
@@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
     }
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+
+    /* Get number of groups and allocate an array in ctx */
+    int groups_count = get_num_elements(groups, ':');
+    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
+
+    /* Parse allowed ciphers, getting IDs */
+    int i = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token;
+    while ((token = strsep(&tmp_groups, ":")))
+    {
+        const mbedtls_ecp_curve_info *ci =
+            mbedtls_ecp_curve_info_from_name(token);
+        if (!ci)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            ctx->groups[i] = ci->grp_id;
+            i++;
+        }
+        token = strsep(&tmp_groups, ":");
+    }
+    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
+
+    gc_free(&gc);
+}
+
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
         mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
     }
 
+    if (ssl_ctx->groups)
+    {
+        mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
+    }
+
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
      * unfragmented, and changing that will require thorough review and
      * testing.  Since OpenVPN is not susceptible to BEAST, we can just
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index 92381f1a..0525134f 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -105,6 +105,7 @@ struct tls_root_ctx {
 #endif
     struct external_context external_key; /**< External key context */
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_ecp_group_id *groups;     /**< List of allowed groups for this connection */
     mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 06c836da..abdf3bf1 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -565,6 +565,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
 #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+    /* This method could be as easy as
+     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
+     * but OpenSSL does not like the name secp256r1 for prime256v1
+     * This is one of the important curves.
+     * To support the same name for OpenSSL and mbedTLS, we do
+     * this dance.
+     */
+
+    int groups_count = get_num_elements(groups, ':');
+
+    int *glist;
+    /* Allocate an array for them */
+    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
+
+    /* Parse allowed ciphers, getting IDs */
+    int glistlen = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token;
+    while ((token = strsep(&tmp_groups, ":"))
+    {
+        if (streq(token, "secp256r1"))
+        {
+            token = "prime256v1";
+        }
+        int nid = OBJ_sn2nid(token);
+
+        if (nid == 0)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            glist[glistlen] = nid;
+            glistlen++;
+        }
+    }
+
+    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
+    {
+        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
+                   groups);
+    }
+    gc_free(&gc);
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -2186,6 +2237,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
 void
 show_available_curves(void)
 {
+    printf("Consider using openssl 'ecparam -list_curves' as\n"
+           "alternative to running this command.\n");
 #ifndef OPENSSL_NO_EC
     EC_builtin_curve *curves = NULL;
     size_t crv_len = 0;
@@ -2195,7 +2248,7 @@ show_available_curves(void)
     ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
     if (EC_get_builtin_curves(curves, crv_len))
     {
-        printf("Available Elliptic curves:\n");
+        printf("\nAvailable Elliptic curves/groups:\n");
         for (n = 0; n < crv_len; n++)
         {
             const char *sname;
-- 
2.26.0

[Openvpn-devel] [PATCH v3 3/3] Implement tls-groups option to specify eliptic curves/groups

[Arne Schwabe]

By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
TLS1.3 key exchange is independent from the signature/key of the
certificates, so allowing all groups per default is not a sensible
choice anymore and instead a shorter list is reasonable.

However, when using certificates with exotic curves that are not on
the group list, the signatures of these certificates will no longer
be accepted.

The tls-groups  option allows to modify the group list to account
for these corner cases.

Patch V2: Uses local gc_arena instead of malloc/free, reword commit
          message. Fix other typos/clarify messages

Patch V3: Style fixes, adjust code to changes from mbed tls session
          fix

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 README.ec                 |  7 ++---
 doc/openvpn.8             | 33 ++++++++++++++++++++++-
 src/openvpn/options.c     | 10 ++++++-
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  6 +++++
 src/openvpn/ssl_backend.h | 10 +++++++
 src/openvpn/ssl_mbedtls.c | 46 ++++++++++++++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h |  1 +
 src/openvpn/ssl_openssl.c | 55 ++++++++++++++++++++++++++++++++++++++-
 9 files changed, 163 insertions(+), 6 deletions(-)

diff --git a/README.ec b/README.ec
index 32938017..61f23b2e 100644
--- a/README.ec
+++ b/README.ec
@@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
 used for authentication, the curve used for the server certificate will be used
 for ECDH too. When autodetection fails (e.g. when using RSA certificates)
 OpenVPN lets the crypto library decide if possible, or falls back to the
-secp384r1 curve.
+secp384r1 curve. The list of groups/curves that the crypto library will choose
+from can be set with the --tls-groups <grouplist> option.
 
 An administrator can force an OpenVPN/OpenSSL server to use a specific curve
 using the --ecdh-curve <curvename> option with one of the curves listed as
-available by the --show-curves option. Clients will use the same curve as
+available by the --show-groups option. Clients will use the same curve as
 selected by the server.
 
-Note that not all curves listed by --show-curves are available for use with TLS;
+Note that not all curves listed by --show-groups are available for use with TLS;
 in that case connecting will fail with a 'no shared cipher' TLS error.
 
 Authentication (ECDSA)
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index dcc72abe..f35a4f64 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -5107,6 +5107,8 @@ Use
 to see a list of TLS ciphers supported by your crypto library.
 
 Warning!
+.B \-\-tls\-groups
+,
 .B \-\-tls\-cipher
 and
 .B \-\-tls\-ciphersuites
@@ -5122,6 +5124,33 @@ OpenSSL.
 The default for \-\-tls\-ciphersuites is to use the crypto library's default.
 .\"*********************************************************
 .TP
+.B \-\-tls\-groups l
+A list
+.B l
+of allowable groups/curves in order of preference.
+
+Set the allowed elictipic curves/groups for the TLS session.
+These groups are  allowed to be used in signatures and key exchange.
+
+mbed TLS currently allows all known curves per default.
+
+OpenSSL 1.1+ restricts the list per default to
+"X25519:secp256r1:X448:secp521r1:secp384r1".
+
+If you use certificates that use non-standard curves, you
+might need to add them here. If you do not force the ecdh curve
+by using
+.B \-\-ecdh\-curve
+, the groups for ecdh will also be picked from this list.
+
+OpenVPN maps the curve name secp256r1 to prime256v1 to allow
+specifying the same tls-groups option for mbed TLS and OpenSSL.
+
+Warning: this option not only affects eliptic curve certificates
+but also the key exchange in TLS 1.3 and using this option improperly
+will disable TLS 1.3.
+.\"*********************************************************
+.TP
 .B \-\-tls\-cert\-profile profile
 Set the allowed cryptographic algorithms for certificates according to
 .B profile\fN.
@@ -5887,8 +5916,10 @@ engines supported by the OpenSSL library.
 .TP
 .B \-\-show\-curves
 (Standalone)
-Show all available elliptic curves to use with the
+Show all available elliptic groups/curves to use with the
 .B \-\-ecdh\-curve
+and
+.B \-\-tls\-groups
 option.
 .\"*********************************************************
 .SS Generating key material:
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 6c0fc0ed..d1e68a51 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7929,7 +7929,7 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_tls_ciphers = true;
     }
-    else if (streq(p[0], "show-curves") && !p[1])
+    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_curves = true;
@@ -7937,6 +7937,9 @@ add_option(struct options *options,
     else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
+        msg(M_WARN, "Consider setting groups/curves preference with "
+            "tls-groups instead of forcing a specific curve with "
+            "ecdh-curve.");
         options->ecdh_curve = p[1];
     }
     else if (streq(p[0], "tls-server") && !p[1])
@@ -8107,6 +8110,11 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->cipher_list_tls13 = p[1];
     }
+    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+        options->tls_groups = p[1];
+    }
     else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
                                                    || !p[2]))
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 375a4fc9..a6a5ba1b 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -541,6 +541,7 @@ struct options
     bool pkcs12_file_inline;
     const char *cipher_list;
     const char *cipher_list_tls13;
+    const char *tls_groups;
     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 c8b3a959..a7bd392a 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
     tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
 
+    /* Set the allow groups/curves for TLS if we want to override them */
+    if (options->tls_groups)
+    {
+        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
+    }
+
     if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
     {
         goto err;
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index a1770bd4..75692797 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
  */
 void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
 
+/**
+ * Set the allowed (eliptic curve) group allowed for signatures and
+ * key exchange.
+ *
+ * @param ctx       TLS context to restrict, must be valid.
+ * @param groups    List of groups that will be allowed, in priority,
+ *                  separated by :
+ */
+void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
+
 /**
  * Check our certificate notBefore and notAfter fields, and warn if the cert is
  * either not yet valid or has expired.  Note that this is a non-fatal error,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index f518f593..d74318f9 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
             free(ctx->allowed_ciphers);
         }
 
+        if (ctx->groups)
+        {
+            free(ctx->groups);
+        }
+
         CLEAR(*ctx);
 
         ctx->initialised = false;
@@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
     }
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+
+    /* Get number of groups and allocate an array in ctx */
+    int groups_count = get_num_elements(groups, ':');
+    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
+
+    /* Parse allowed ciphers, getting IDs */
+    int i = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token;
+    while ((token = strsep(&tmp_groups, ":")))
+    {
+        const mbedtls_ecp_curve_info *ci =
+            mbedtls_ecp_curve_info_from_name(token);
+        if (!ci)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            ctx->groups[i] = ci->grp_id;
+            i++;
+        }
+    }
+    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
+
+    gc_free(&gc);
+}
+
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
         mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
     }
 
+    if (ssl_ctx->groups)
+    {
+        mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
+    }
+
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
      * unfragmented, and changing that will require thorough review and
      * testing.  Since OpenVPN is not susceptible to BEAST, we can just
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index 92381f1a..0525134f 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -105,6 +105,7 @@ struct tls_root_ctx {
 #endif
     struct external_context external_key; /**< External key context */
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_ecp_group_id *groups;     /**< List of allowed groups for this connection */
     mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 06c836da..abdf3bf1 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -565,6 +565,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
 #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+    /* This method could be as easy as
+     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
+     * but OpenSSL does not like the name secp256r1 for prime256v1
+     * This is one of the important curves.
+     * To support the same name for OpenSSL and mbedTLS, we do
+     * this dance.
+     */
+
+    int groups_count = get_num_elements(groups, ':');
+
+    int *glist;
+    /* Allocate an array for them */
+    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
+
+    /* Parse allowed ciphers, getting IDs */
+    int glistlen = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token;
+    while ((token = strsep(&tmp_groups, ":")))
+    {
+        if (streq(token, "secp256r1"))
+        {
+            token = "prime256v1";
+        }
+        int nid = OBJ_sn2nid(token);
+
+        if (nid == 0)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            glist[glistlen] = nid;
+            glistlen++;
+        }
+    }
+
+    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
+    {
+        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
+                   groups);
+    }
+    gc_free(&gc);
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -2186,6 +2237,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
 void
 show_available_curves(void)
 {
+    printf("Consider using openssl 'ecparam -list_curves' as\n"
+           "alternative to running this command.\n");
 #ifndef OPENSSL_NO_EC
     EC_builtin_curve *curves = NULL;
     size_t crv_len = 0;
@@ -2195,7 +2248,7 @@ show_available_curves(void)
     ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
     if (EC_get_builtin_curves(curves, crv_len))
     {
-        printf("Available Elliptic curves:\n");
+        printf("\nAvailable Elliptic curves/groups:\n");
         for (n = 0; n < crv_len; n++)
         {
             const char *sname;
-- 
2.26.0

Re: [Openvpn-devel] [PATCH v3 3/3] Implement tls-groups option to specify eliptic curves/groups

[Arne Schwabe]

[-- Attachment #1.1: Type: text/plain, Size: 179 bytes --]

Am 05.06.20 um 00:16 schrieb Arne Schwabe:


Ignore this one, I made the patch, tested it, fixed it and forgot to
copy it before sending it to the mailling list.

Arne



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Openvpn-devel] [PATCH v4 3/3] Implement tls-groups option to specify eliptic curves/groups

[Arne Schwabe]

By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
TLS1.3 key exchange is independent from the signature/key of the
certificates, so allowing all groups per default is not a sensible
choice anymore and instead a shorter list is reasonable.

However, when using certificates with exotic curves that are not on
the group list, the signatures of these certificates will no longer
be accepted.

The tls-groups  option allows to modify the group list to account
for these corner cases.

Patch V2: Uses local gc_arena instead of malloc/free, reword commit
          message. Fix other typos/clarify messages

Patch V3: Style fixes, adjust code to changes from mbed tls session
          fix

Patch V4: Resend V3 as V4 as the invalid V3 confused patchwork and Gert.

Signed-off-by: Arne Schwabe <arne@...1227...>
---
 README.ec                 |  7 ++---
 doc/openvpn.8             | 33 ++++++++++++++++++++++-
 src/openvpn/options.c     | 10 ++++++-
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  6 +++++
 src/openvpn/ssl_backend.h | 10 +++++++
 src/openvpn/ssl_mbedtls.c | 46 ++++++++++++++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h |  1 +
 src/openvpn/ssl_openssl.c | 55 ++++++++++++++++++++++++++++++++++++++-
 9 files changed, 163 insertions(+), 6 deletions(-)

diff --git a/README.ec b/README.ec
index 32938017..61f23b2e 100644
--- a/README.ec
+++ b/README.ec
@@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
 used for authentication, the curve used for the server certificate will be used
 for ECDH too. When autodetection fails (e.g. when using RSA certificates)
 OpenVPN lets the crypto library decide if possible, or falls back to the
-secp384r1 curve.
+secp384r1 curve. The list of groups/curves that the crypto library will choose
+from can be set with the --tls-groups <grouplist> option.
 
 An administrator can force an OpenVPN/OpenSSL server to use a specific curve
 using the --ecdh-curve <curvename> option with one of the curves listed as
-available by the --show-curves option. Clients will use the same curve as
+available by the --show-groups option. Clients will use the same curve as
 selected by the server.
 
-Note that not all curves listed by --show-curves are available for use with TLS;
+Note that not all curves listed by --show-groups are available for use with TLS;
 in that case connecting will fail with a 'no shared cipher' TLS error.
 
 Authentication (ECDSA)
diff --git a/doc/openvpn.8 b/doc/openvpn.8
index dcc72abe..f35a4f64 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -5107,6 +5107,8 @@ Use
 to see a list of TLS ciphers supported by your crypto library.
 
 Warning!
+.B \-\-tls\-groups
+,
 .B \-\-tls\-cipher
 and
 .B \-\-tls\-ciphersuites
@@ -5122,6 +5124,33 @@ OpenSSL.
 The default for \-\-tls\-ciphersuites is to use the crypto library's default.
 .\"*********************************************************
 .TP
+.B \-\-tls\-groups l
+A list
+.B l
+of allowable groups/curves in order of preference.
+
+Set the allowed elictipic curves/groups for the TLS session.
+These groups are  allowed to be used in signatures and key exchange.
+
+mbed TLS currently allows all known curves per default.
+
+OpenSSL 1.1+ restricts the list per default to
+"X25519:secp256r1:X448:secp521r1:secp384r1".
+
+If you use certificates that use non-standard curves, you
+might need to add them here. If you do not force the ecdh curve
+by using
+.B \-\-ecdh\-curve
+, the groups for ecdh will also be picked from this list.
+
+OpenVPN maps the curve name secp256r1 to prime256v1 to allow
+specifying the same tls-groups option for mbed TLS and OpenSSL.
+
+Warning: this option not only affects eliptic curve certificates
+but also the key exchange in TLS 1.3 and using this option improperly
+will disable TLS 1.3.
+.\"*********************************************************
+.TP
 .B \-\-tls\-cert\-profile profile
 Set the allowed cryptographic algorithms for certificates according to
 .B profile\fN.
@@ -5887,8 +5916,10 @@ engines supported by the OpenSSL library.
 .TP
 .B \-\-show\-curves
 (Standalone)
-Show all available elliptic curves to use with the
+Show all available elliptic groups/curves to use with the
 .B \-\-ecdh\-curve
+and
+.B \-\-tls\-groups
 option.
 .\"*********************************************************
 .SS Generating key material:
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 16f9da6a..0c1d590c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7947,7 +7947,7 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_tls_ciphers = true;
     }
-    else if (streq(p[0], "show-curves") && !p[1])
+    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_curves = true;
@@ -7955,6 +7955,9 @@ add_option(struct options *options,
     else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
+        msg(M_WARN, "Consider setting groups/curves preference with "
+            "tls-groups instead of forcing a specific curve with "
+            "ecdh-curve.");
         options->ecdh_curve = p[1];
     }
     else if (streq(p[0], "tls-server") && !p[1])
@@ -8125,6 +8128,11 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->cipher_list_tls13 = p[1];
     }
+    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+        options->tls_groups = p[1];
+    }
     else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
                                                    || !p[2]))
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 375a4fc9..a6a5ba1b 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -541,6 +541,7 @@ struct options
     bool pkcs12_file_inline;
     const char *cipher_list;
     const char *cipher_list_tls13;
+    const char *tls_groups;
     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 68d2b5c1..7244c43d 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
     tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
 
+    /* Set the allow groups/curves for TLS if we want to override them */
+    if (options->tls_groups)
+    {
+        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
+    }
+
     if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
     {
         goto err;
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index a1770bd4..75692797 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
  */
 void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
 
+/**
+ * Set the allowed (eliptic curve) group allowed for signatures and
+ * key exchange.
+ *
+ * @param ctx       TLS context to restrict, must be valid.
+ * @param groups    List of groups that will be allowed, in priority,
+ *                  separated by :
+ */
+void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
+
 /**
  * Check our certificate notBefore and notAfter fields, and warn if the cert is
  * either not yet valid or has expired.  Note that this is a non-fatal error,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index f518f593..d74318f9 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
             free(ctx->allowed_ciphers);
         }
 
+        if (ctx->groups)
+        {
+            free(ctx->groups);
+        }
+
         CLEAR(*ctx);
 
         ctx->initialised = false;
@@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
     }
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+
+    /* Get number of groups and allocate an array in ctx */
+    int groups_count = get_num_elements(groups, ':');
+    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
+
+    /* Parse allowed ciphers, getting IDs */
+    int i = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token;
+    while ((token = strsep(&tmp_groups, ":")))
+    {
+        const mbedtls_ecp_curve_info *ci =
+            mbedtls_ecp_curve_info_from_name(token);
+        if (!ci)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            ctx->groups[i] = ci->grp_id;
+            i++;
+        }
+        token = strsep(&tmp_groups, ":");
+    }
+    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
+
+    gc_free(&gc);
+}
+
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
         mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
     }
 
+    if (ssl_ctx->groups)
+    {
+        mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
+    }
+
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
      * unfragmented, and changing that will require thorough review and
      * testing.  Since OpenVPN is not susceptible to BEAST, we can just
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index 92381f1a..0525134f 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -105,6 +105,7 @@ struct tls_root_ctx {
 #endif
     struct external_context external_key; /**< External key context */
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_ecp_group_id *groups;     /**< List of allowed groups for this connection */
     mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index a489053b..da7d252a 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -565,6 +565,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
 #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+    /* This method could be as easy as
+     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
+     * but OpenSSL does not like the name secp256r1 for prime256v1
+     * This is one of the important curves.
+     * To support the same name for OpenSSL and mbedTLS, we do
+     * this dance.
+     */
+
+    int groups_count = get_num_elements(groups, ':');
+
+    int *glist;
+    /* Allocate an array for them */
+    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
+
+    /* Parse allowed ciphers, getting IDs */
+    int glistlen = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token;
+    while ((token = strsep(&tmp_groups, ":")))
+    {
+        if (streq(token, "secp256r1"))
+        {
+            token = "prime256v1";
+        }
+        int nid = OBJ_sn2nid(token);
+
+        if (nid == 0)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            glist[glistlen] = nid;
+            glistlen++;
+        }
+    }
+
+    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
+    {
+        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
+                   groups);
+    }
+    gc_free(&gc);
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -2191,6 +2242,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
 void
 show_available_curves(void)
 {
+    printf("Consider using openssl 'ecparam -list_curves' as\n"
+           "alternative to running this command.\n");
 #ifndef OPENSSL_NO_EC
     EC_builtin_curve *curves = NULL;
     size_t crv_len = 0;
@@ -2200,7 +2253,7 @@ show_available_curves(void)
     ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
     if (EC_get_builtin_curves(curves, crv_len))
     {
-        printf("Available Elliptic curves:\n");
+        printf("\nAvailable Elliptic curves/groups:\n");
         for (n = 0; n < crv_len; n++)
         {
             const char *sname;
-- 
2.26.0

Re: [Openvpn-devel] [PATCH v4 3/3] Implement tls-groups option to specify eliptic curves/groups

[Antonio Quartulli]

Hi,

On 22/06/2020 16:02, Arne Schwabe wrote:

[CUT]

> @@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>      }
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(ctx);
> +    struct gc_arena gc = gc_new();
> +
> +    /* Get number of groups and allocate an array in ctx */
> +    int groups_count = get_num_elements(groups, ':');
> +    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int i = 0;
> +    char *tmp_groups = string_alloc(groups, &gc);
> +
> +    const char *token;
> +    while ((token = strsep(&tmp_groups, ":")))
> +    {
> +        const mbedtls_ecp_curve_info *ci =
> +            mbedtls_ecp_curve_info_from_name(token);
> +        if (!ci)
> +        {
> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +        }
> +        else
> +        {
> +            ctx->groups[i] = ci->grp_id;
> +            i++;
> +        }
> +        token = strsep(&tmp_groups, ":");

The line above should be removed, otherwise end up doing strsep() twice
in a row.


> +    }
> +    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
> +
> +    gc_free(&gc);
> +}
> +
> +
>  void
>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  {
> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>          mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
>      }
>  
> +    if (ssl_ctx->groups)
> +    {
> +        mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
> +    }
> +
>      /* Disable record splitting (for now).  OpenVPN assumes records are sent
>       * unfragmented, and changing that will require thorough review and
>       * testing.  Since OpenVPN is not susceptible to BEAST, we can just
> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
> index 92381f1a..0525134f 100644
> --- a/src/openvpn/ssl_mbedtls.h
> +++ b/src/openvpn/ssl_mbedtls.h
> @@ -105,6 +105,7 @@ struct tls_root_ctx {
>  #endif
>      struct external_context external_key; /**< External key context */
>      int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
> +    mbedtls_ecp_group_id *groups;     /**< List of allowed groups for this connection */
>      mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
>  };
>  
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index a489053b..da7d252a 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -565,6 +565,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>  #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(ctx);
> +    struct gc_arena gc = gc_new();
> +    /* This method could be as easy as
> +     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
> +     * but OpenSSL does not like the name secp256r1 for prime256v1
> +     * This is one of the important curves.
> +     * To support the same name for OpenSSL and mbedTLS, we do
> +     * this dance.
> +     */
> +
> +    int groups_count = get_num_elements(groups, ':');
> +
> +    int *glist;
> +    /* Allocate an array for them */
> +    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int glistlen = 0;
> +    char *tmp_groups = string_alloc(groups, &gc);
> +
> +    const char *token;
> +    while ((token = strsep(&tmp_groups, ":")))
> +    {
> +        if (streq(token, "secp256r1"))
> +        {
> +            token = "prime256v1";
> +        }
> +        int nid = OBJ_sn2nid(token);
> +
> +        if (nid == 0)

From a style perspective, I think it looks better to add an empty line
*before* "int nid =..." and remove the one after.

This way we also follow the same pattern:

x = a();
if (x ..)
{
}

as other parts of the code.




The rest looks good to me.

Regards,

-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH v4 3/3] Implement tls-groups option to specify eliptic curves/groups

[Antonio Quartulli]

Hi,

on my GitLab CI build test, the compilation failed with the following
message, while compiling against openssl-1.1:

 /usr/bin/ld: ssl_openssl.o: in function `tls_ctx_set_tls_groups':
/builds/ordex986/openvpn/src/openvpn/ssl_openssl.c:611: undefined
reference to `SSL_CTX_set1_groups'
collect2: error: ld returned 1 exit status


Any clue?

On 23/06/2020 11:21, Antonio Quartulli wrote:
> Hi,
> 
> On 22/06/2020 16:02, Arne Schwabe wrote:
> 
> [CUT]
> 
>> @@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>>      }
>>  }
>>  
>> +void
>> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
>> +{
>> +    ASSERT(ctx);
>> +    struct gc_arena gc = gc_new();
>> +
>> +    /* Get number of groups and allocate an array in ctx */
>> +    int groups_count = get_num_elements(groups, ':');
>> +    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
>> +
>> +    /* Parse allowed ciphers, getting IDs */
>> +    int i = 0;
>> +    char *tmp_groups = string_alloc(groups, &gc);
>> +
>> +    const char *token;
>> +    while ((token = strsep(&tmp_groups, ":")))
>> +    {
>> +        const mbedtls_ecp_curve_info *ci =
>> +            mbedtls_ecp_curve_info_from_name(token);
>> +        if (!ci)
>> +        {
>> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
>> +        }
>> +        else
>> +        {
>> +            ctx->groups[i] = ci->grp_id;
>> +            i++;
>> +        }
>> +        token = strsep(&tmp_groups, ":");
> 
> The line above should be removed, otherwise end up doing strsep() twice
> in a row.
> 
> 
>> +    }
>> +    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
>> +
>> +    gc_free(&gc);
>> +}
>> +
>> +
>>  void
>>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>>  {
>> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>>          mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
>>      }
>>  
>> +    if (ssl_ctx->groups)
>> +    {
>> +        mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
>> +    }
>> +
>>      /* Disable record splitting (for now).  OpenVPN assumes records are sent
>>       * unfragmented, and changing that will require thorough review and
>>       * testing.  Since OpenVPN is not susceptible to BEAST, we can just
>> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
>> index 92381f1a..0525134f 100644
>> --- a/src/openvpn/ssl_mbedtls.h
>> +++ b/src/openvpn/ssl_mbedtls.h
>> @@ -105,6 +105,7 @@ struct tls_root_ctx {
>>  #endif
>>      struct external_context external_key; /**< External key context */
>>      int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
>> +    mbedtls_ecp_group_id *groups;     /**< List of allowed groups for this connection */
>>      mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
>>  };
>>  
>> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
>> index a489053b..da7d252a 100644
>> --- a/src/openvpn/ssl_openssl.c
>> +++ b/src/openvpn/ssl_openssl.c
>> @@ -565,6 +565,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>>  #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
>>  }
>>  
>> +void
>> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
>> +{
>> +    ASSERT(ctx);
>> +    struct gc_arena gc = gc_new();
>> +    /* This method could be as easy as
>> +     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
>> +     * but OpenSSL does not like the name secp256r1 for prime256v1
>> +     * This is one of the important curves.
>> +     * To support the same name for OpenSSL and mbedTLS, we do
>> +     * this dance.
>> +     */
>> +
>> +    int groups_count = get_num_elements(groups, ':');
>> +
>> +    int *glist;
>> +    /* Allocate an array for them */
>> +    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
>> +
>> +    /* Parse allowed ciphers, getting IDs */
>> +    int glistlen = 0;
>> +    char *tmp_groups = string_alloc(groups, &gc);
>> +
>> +    const char *token;
>> +    while ((token = strsep(&tmp_groups, ":")))
>> +    {
>> +        if (streq(token, "secp256r1"))
>> +        {
>> +            token = "prime256v1";
>> +        }
>> +        int nid = OBJ_sn2nid(token);
>> +
>> +        if (nid == 0)
> 
> From a style perspective, I think it looks better to add an empty line
> *before* "int nid =..." and remove the one after.
> 
> This way we also follow the same pattern:
> 
> x = a();
> if (x ..)
> {
> }
> 
> as other parts of the code.
> 
> 
> 
> 
> The rest looks good to me.
> 
> Regards,
> 

-- 
Antonio Quartulli