Browse code

Implement tls-groups option to specify eliptic curves/groups

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 mbedTLS session
fix

Patch V5: Fix compilation with OpenSSL 1.0.2

Patch V6: Redo the 'while((token = strsep(&tmp_groups, ":"))' change
which accidentally got lost.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20200721154922.17144-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20521.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Arne Schwabe authored on 2020/07/22 00:49:22
Showing 13 changed files
... ...
@@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
12 12
 used for authentication, the curve used for the server certificate will be used
13 13
 for ECDH too. When autodetection fails (e.g. when using RSA certificates)
14 14
 OpenVPN lets the crypto library decide if possible, or falls back to the
15
-secp384r1 curve.
15
+secp384r1 curve. The list of groups/curves that the crypto library will choose
16
+from can be set with the --tls-groups <grouplist> option.
16 17
 
17 18
 An administrator can force an OpenVPN/OpenSSL server to use a specific curve
18 19
 using the --ecdh-curve <curvename> option with one of the curves listed as
19
-available by the --show-curves option. Clients will use the same curve as
20
+available by the --show-groups option. Clients will use the same curve as
20 21
 selected by the server.
21 22
 
22
-Note that not all curves listed by --show-curves are available for use with TLS;
23
+Note that not all curves listed by --show-groups are available for use with TLS;
23 24
 in that case connecting will fail with a 'no shared cipher' TLS error.
24 25
 
25 26
 Authentication (ECDSA)
... ...
@@ -929,6 +929,7 @@ if test "${with_crypto_library}" = "openssl"; then
929 929
 			OpenSSL_version \
930 930
 			SSL_CTX_get_default_passwd_cb \
931 931
 			SSL_CTX_get_default_passwd_cb_userdata \
932
+			SSL_CTX_set1_groups \
932 933
 			SSL_CTX_set_security_level \
933 934
 			X509_get0_notBefore \
934 935
 			X509_get0_notAfter \
... ...
@@ -27,9 +27,9 @@ SSL Library information
27 27
   (Standalone) Show currently available hardware-based crypto acceleration
28 28
   engines supported by the OpenSSL library.
29 29
 
30
-  (Standalone) Show all available elliptic curves to use with the
31
-  ``--ecdh-curve`` option.
30
+--show-groups
31
+  (Standalone) Show all available elliptic curves/groups to use with the
32
+  ``--ecdh-curve`` and ``tls-groups`` options.
32 33
 
33 34
 Generating key material
34 35
 -----------------------
... ...
@@ -338,6 +338,31 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
338 338
   Use ``--tls-crypt`` instead if you want to use the key file to not only
339 339
   authenticate, but also encrypt the TLS control channel.
340 340
 
341
+--tls-groups list
342
+    A list of allowable groups/curves in order of preference.
343
+
344
+    Set the allowed elliptic curves/groups for the TLS session.
345
+    These groups are allowed to be used in signatures and key exchange.
346
+
347
+    mbedTLS currently allows all known curves per default.
348
+
349
+    OpenSSL 1.1+ restricts the list per default to
350
+    ::
351
+
352
+      "X25519:secp256r1:X448:secp521r1:secp384r1".
353
+
354
+    If you use certificates that use non-standard curves, you
355
+    might need to add them here. If you do not force the ecdh curve
356
+    by using ``--ecdh-curve``, the groups for ecdh will also be picked
357
+    from this list.
358
+
359
+    OpenVPN maps the curve name `secp256r1` to `prime256v1` to allow
360
+    specifying the same tls-groups option for mbedTLS and OpenSSL.
361
+
362
+    Warning: this option not only affects elliptic curve certificates
363
+    but also the key exchange in TLS 1.3 and using this option improperly
364
+    will disable TLS 1.3.
365
+
341 366
 --tls-cert-profile profile
342 367
   Set the allowed cryptographic algorithms for certificates according to
343 368
   ``profile``.
... ...
@@ -368,7 +393,7 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
368 368
   OpenVPN will migrate to 'preferred' as default in the future. Please
369 369
   ensure that your keys already comply.
370 370
 
371
-*WARNING:* ``--tls-ciphers`` and ``--tls-ciphersuites``
371
+*WARNING:* ``--tls-ciphers``, ``--tls-ciphersuites`` and ``tls-groups``
372 372
     These options are expert features, which - if used correctly - can
373 373
     improve the security of your VPN connection. But it is also easy to
374 374
     unwittingly use them to carefully align a gun with your foot, or just
... ...
@@ -183,6 +183,12 @@ SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
183 183
 }
184 184
 #endif
185 185
 
186
+/* This function is implemented as macro, so the configure check for the
187
+ * function may fail, so we check for both variants here */
188
+#if !defined(HAVE_SSL_CTX_SET1_GROUPS) && !defined(SSL_CTX_set1_groups)
189
+#define SSL_CTX_set1_groups SSL_CTX_set1_curves
190
+#endif
191
+
186 192
 #if !defined(HAVE_X509_GET0_PUBKEY)
187 193
 /**
188 194
  * Get the public key from a X509 certificate
... ...
@@ -7998,7 +7998,7 @@ add_option(struct options *options,
7998 7998
         VERIFY_PERMISSION(OPT_P_GENERAL);
7999 7999
         options->show_tls_ciphers = true;
8000 8000
     }
8001
-    else if (streq(p[0], "show-curves") && !p[1])
8001
+    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
8002 8002
     {
8003 8003
         VERIFY_PERMISSION(OPT_P_GENERAL);
8004 8004
         options->show_curves = true;
... ...
@@ -8006,6 +8006,9 @@ add_option(struct options *options,
8006 8006
     else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
8007 8007
     {
8008 8008
         VERIFY_PERMISSION(OPT_P_GENERAL);
8009
+        msg(M_WARN, "Consider setting groups/curves preference with "
8010
+            "tls-groups instead of forcing a specific curve with "
8011
+            "ecdh-curve.");
8009 8012
         options->ecdh_curve = p[1];
8010 8013
     }
8011 8014
     else if (streq(p[0], "tls-server") && !p[1])
... ...
@@ -8176,6 +8179,11 @@ add_option(struct options *options,
8176 8176
         VERIFY_PERMISSION(OPT_P_GENERAL);
8177 8177
         options->cipher_list_tls13 = p[1];
8178 8178
     }
8179
+    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
8180
+    {
8181
+        VERIFY_PERMISSION(OPT_P_GENERAL);
8182
+        options->tls_groups = p[1];
8183
+    }
8179 8184
     else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
8180 8185
                                                    || !p[2]))
8181 8186
     {
... ...
@@ -538,6 +538,7 @@ struct options
538 538
     bool pkcs12_file_inline;
539 539
     const char *cipher_list;
540 540
     const char *cipher_list_tls13;
541
+    const char *tls_groups;
541 542
     const char *tls_cert_profile;
542 543
     const char *ecdh_curve;
543 544
     const char *tls_verify;
... ...
@@ -615,6 +615,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
615 615
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
616 616
     tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
617 617
 
618
+    /* Set the allow groups/curves for TLS if we want to override them */
619
+    if (options->tls_groups)
620
+    {
621
+        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
622
+    }
623
+
618 624
     if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
619 625
     {
620 626
         goto err;
... ...
@@ -199,6 +199,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
199 199
 void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
200 200
 
201 201
 /**
202
+ * Set the (elliptic curve) group allowed for signatures and
203
+ * key exchange.
204
+ *
205
+ * @param ctx       TLS context to restrict, must be valid.
206
+ * @param groups    List of groups that will be allowed, in priority,
207
+ *                  separated by :
208
+ */
209
+void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
210
+
211
+/**
202 212
  * Check our certificate notBefore and notAfter fields, and warn if the cert is
203 213
  * either not yet valid or has expired.  Note that this is a non-fatal error,
204 214
  * since we compare against the system time, which might be incorrect.
... ...
@@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
176 176
             free(ctx->allowed_ciphers);
177 177
         }
178 178
 
179
+        if (ctx->groups)
180
+        {
181
+            free(ctx->groups);
182
+        }
183
+
179 184
         CLEAR(*ctx);
180 185
 
181 186
         ctx->initialised = false;
... ...
@@ -344,6 +349,41 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
344 344
 }
345 345
 
346 346
 void
347
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
348
+{
349
+    ASSERT(ctx);
350
+    struct gc_arena gc = gc_new();
351
+
352
+    /* Get number of groups and allocate an array in ctx */
353
+    int groups_count = get_num_elements(groups, ':');
354
+    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
355
+
356
+    /* Parse allowed ciphers, getting IDs */
357
+    int i = 0;
358
+    char *tmp_groups = string_alloc(groups, &gc);
359
+
360
+    const char *token;
361
+    while ((token = strsep(&tmp_groups, ":")))
362
+    {
363
+        const mbedtls_ecp_curve_info *ci =
364
+            mbedtls_ecp_curve_info_from_name(token);
365
+        if (!ci)
366
+        {
367
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
368
+        }
369
+        else
370
+        {
371
+            ctx->groups[i] = ci->grp_id;
372
+            i++;
373
+        }
374
+    }
375
+    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
376
+
377
+    gc_free(&gc);
378
+}
379
+
380
+
381
+void
347 382
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
348 383
 {
349 384
     ASSERT(ctx);
... ...
@@ -1043,6 +1083,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
1043 1043
         mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
1044 1044
     }
1045 1045
 
1046
+    if (ssl_ctx->groups)
1047
+    {
1048
+        mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
1049
+    }
1050
+
1046 1051
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
1047 1052
      * unfragmented, and changing that will require thorough review and
1048 1053
      * testing.  Since OpenVPN is not susceptible to BEAST, we can just
... ...
@@ -105,6 +105,7 @@ struct tls_root_ctx {
105 105
 #endif
106 106
     struct external_context external_key; /**< External key context */
107 107
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
108
+    mbedtls_ecp_group_id *groups;     /**< List of allowed groups for this connection */
108 109
     mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
109 110
 };
110 111
 
... ...
@@ -233,15 +233,14 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
233 233
 
234 234
     char *tmp_ciphers = string_alloc(server_list, &gc_tmp);
235 235
 
236
-    const char *token = strsep(&tmp_ciphers, ":");
237
-    while (token)
236
+    const char *token;
237
+    while ((token = strsep(&tmp_ciphers, ":")))
238 238
     {
239 239
         if (tls_item_in_cipher_list(token, peer_ncp_list)
240 240
             || streq(token, remote_cipher))
241 241
         {
242 242
             break;
243 243
         }
244
-        token = strsep(&tmp_ciphers, ":");
245 244
     }
246 245
     /* We have not found a common cipher, as a last resort check if the
247 246
      * server cipher can be used
... ...
@@ -564,6 +564,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
564 564
 }
565 565
 
566 566
 void
567
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
568
+{
569
+    ASSERT(ctx);
570
+    struct gc_arena gc = gc_new();
571
+    /* This method could be as easy as
572
+     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
573
+     * but OpenSSL does not like the name secp256r1 for prime256v1
574
+     * This is one of the important curves.
575
+     * To support the same name for OpenSSL and mbedTLS, we do
576
+     * this dance.
577
+     */
578
+
579
+    int groups_count = get_num_elements(groups, ':');
580
+
581
+    int *glist;
582
+    /* Allocate an array for them */
583
+    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
584
+
585
+    /* Parse allowed ciphers, getting IDs */
586
+    int glistlen = 0;
587
+    char *tmp_groups = string_alloc(groups, &gc);
588
+
589
+    const char *token;
590
+    while ((token = strsep(&tmp_groups, ":")))
591
+    {
592
+        if (streq(token, "secp256r1"))
593
+        {
594
+            token = "prime256v1";
595
+        }
596
+        int nid = OBJ_sn2nid(token);
597
+
598
+        if (nid == 0)
599
+        {
600
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
601
+        }
602
+        else
603
+        {
604
+            glist[glistlen] = nid;
605
+            glistlen++;
606
+        }
607
+    }
608
+
609
+    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
610
+    {
611
+        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
612
+                   groups);
613
+    }
614
+    gc_free(&gc);
615
+}
616
+
617
+void
567 618
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
568 619
 {
569 620
     int ret;
... ...
@@ -2135,6 +2186,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
2135 2135
 void
2136 2136
 show_available_curves(void)
2137 2137
 {
2138
+    printf("Consider using openssl 'ecparam -list_curves' as\n"
2139
+           "alternative to running this command.\n");
2138 2140
 #ifndef OPENSSL_NO_EC
2139 2141
     EC_builtin_curve *curves = NULL;
2140 2142
     size_t crv_len = 0;
... ...
@@ -2144,7 +2197,7 @@ show_available_curves(void)
2144 2144
     ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
2145 2145
     if (EC_get_builtin_curves(curves, crv_len))
2146 2146
     {
2147
-        printf("Available Elliptic curves:\n");
2147
+        printf("\nAvailable Elliptic curves/groups:\n");
2148 2148
         for (n = 0; n < crv_len; n++)
2149 2149
         {
2150 2150
             const char *sname;