No valid groups is not an error

Of course TLS-1.3 won't be usable with such configuration.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/26801)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 3f5abaa48c43e4f64bb3ba1c581959794f9ff56c..0311dadb7ecebb402f6b43d2376dd612d85ecf7b 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -829,32 +829,43 @@ SSL *ossl_ssl_connection_new_int(SSL_CTX *ctx, SSL *user_ssl,
         goto err;
 
     s->session_ctx = ctx;
-    if (ctx->ext.ecpointformats) {
+    if (ctx->ext.ecpointformats != NULL) {
         s->ext.ecpointformats =
             OPENSSL_memdup(ctx->ext.ecpointformats,
                            ctx->ext.ecpointformats_len);
-        if (!s->ext.ecpointformats) {
+        if (s->ext.ecpointformats == NULL) {
             s->ext.ecpointformats_len = 0;
             goto err;
         }
         s->ext.ecpointformats_len =
             ctx->ext.ecpointformats_len;
     }
-    if (ctx->ext.supportedgroups) {
+    if (ctx->ext.supportedgroups != NULL) {
+        size_t add = 0;
+
+        if (ctx->ext.supportedgroups_len == 0)
+            /* Add 1 so allocation won't fail */
+            add = 1;
         s->ext.supportedgroups =
             OPENSSL_memdup(ctx->ext.supportedgroups,
-                           ctx->ext.supportedgroups_len
-                                * sizeof(*ctx->ext.supportedgroups));
-        if (!s->ext.supportedgroups) {
+                           (ctx->ext.supportedgroups_len + add)
+                           * sizeof(*ctx->ext.supportedgroups));
+        if (s->ext.supportedgroups == NULL) {
             s->ext.supportedgroups_len = 0;
             goto err;
         }
         s->ext.supportedgroups_len = ctx->ext.supportedgroups_len;
     }
     if (ctx->ext.keyshares != NULL) {
+        size_t add = 0;
+
+        if (ctx->ext.keyshares_len == 0)
+            /* Add 1 so allocation won't fail */
+            add = 1;
         s->ext.keyshares =
             OPENSSL_memdup(ctx->ext.keyshares,
-                           ctx->ext.keyshares_len * sizeof(*ctx->ext.keyshares));
+                           (ctx->ext.keyshares_len + add)
+                           * sizeof(*ctx->ext.keyshares));
         if (s->ext.keyshares == NULL) {
             s->ext.keyshares_len = 0;
             goto err;
@@ -862,9 +873,15 @@ SSL *ossl_ssl_connection_new_int(SSL_CTX *ctx, SSL *user_ssl,
         s->ext.keyshares_len = ctx->ext.keyshares_len;
     }
     if (ctx->ext.tuples != NULL) {
+        size_t add = 0;
+
+        if (ctx->ext.tuples_len == 0)
+            /* Add 1 so allocation won't fail */
+            add = 1;
         s->ext.tuples =
             OPENSSL_memdup(ctx->ext.tuples,
-                           ctx->ext.tuples_len * sizeof(*ctx->ext.tuples));
+                           (ctx->ext.tuples_len + add)
+                           * sizeof(*ctx->ext.tuples));
         if (s->ext.tuples == NULL) {
             s->ext.tuples_len = 0;
             goto err;

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 1ec5596550ec5be9b39b8849ff50d6059ee7733c..7c18dadbda3c2227c3e44d00c62145fbf19d1651 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -9,6 +9,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <ctype.h>
 #include <openssl/objects.h>
 #include <openssl/evp.h>
 #include <openssl/hmac.h>
@@ -1565,13 +1566,13 @@ int tls1_set_groups_list(SSL_CTX *ctx,
                          size_t **tplext, size_t *tplextlen,
                          const char *str)
 {
-    size_t i, j;
+    size_t i = 0, j;
     int ret = 0, parse_ret = 0;
     gid_cb_st gcb;
 
     /* Sanity check */
     if (ctx == NULL) {
-        ERR_raise(ERR_LIB_CONF, CONF_R_VARIABLE_HAS_NO_VALUE);
+        ERR_raise(ERR_LIB_CONF, ERR_R_INTERNAL_ERROR);
         return 0;
     }
 
@@ -1595,6 +1596,11 @@ int tls1_set_groups_list(SSL_CTX *ctx,
     if (gcb.ksid_arr == NULL)
         goto end;
 
+    while (str[0] != '\0' && isspace((unsigned char)*str))
+        str++;
+    if (str[0] == '\0')
+        goto empty_list;
+
     /*
      * Start the (potentially recursive) tuple processing by calling CONF_parse_list
      * with the TUPLE_DELIMITER_CHARACTER (which will call tuple_cb after cleaning spaces)
@@ -1624,12 +1630,6 @@ int tls1_set_groups_list(SSL_CTX *ctx,
     }
     gcb.tplcnt = i;
 
-    /* Some more checks (at least one remaining group, not more that nominally 4 key shares */
-    if (gcb.gidcnt == 0) {
-        ERR_raise_data(ERR_LIB_SSL, ERR_R_PASSED_INVALID_ARGUMENT,
-                       "No valid groups in '%s'", str);
-        goto end;
-    }
     if (gcb.ksidcnt > OPENSSL_CLIENT_MAX_KEY_SHARES) {
         ERR_raise_data(ERR_LIB_SSL, ERR_R_PASSED_INVALID_ARGUMENT,
                        "To many keyshares requested in '%s' (max = %d)",
@@ -1641,7 +1641,7 @@ int tls1_set_groups_list(SSL_CTX *ctx,
      * For backward compatibility we let the rest of the code know that a key share
      * for the first valid group should be added if no "*" prefix was used anywhere
      */
-    if (gcb.ksidcnt == 0) {
+    if (gcb.gidcnt > 0 && gcb.ksidcnt == 0) {
         /*
          * No key share group prefix character was used, hence we indicate that a single
          * key share should be sent and flag that it should come from the supported_groups list
@@ -1650,6 +1650,7 @@ int tls1_set_groups_list(SSL_CTX *ctx,
         gcb.ksid_arr[0] = 0;
     }
 
+ empty_list:
     /*
      * A call to tls1_set_groups_list with any of the args (other than ctx) set
      * to NULL only does a syntax check, hence we're done here and report success

diff --git a/test/sslapitest.c b/test/sslapitest.c
index 8c1b2073b872a0422f7e7b5f5fc29e118166311c..f3395997c5dd9d442394a0b5567c1df88be9d0e3 100644 (file)
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -9846,7 +9846,7 @@ static int test_unknown_sigalgs_groups(void)
                                               0))
         goto end;
 
-    if (!TEST_int_le(SSL_CTX_set1_groups_list(ctx,
+    if (!TEST_int_gt(SSL_CTX_set1_groups_list(ctx,
                                               "?nonexistent1:?nonexistent2:?nonexistent3"),
                                               0))
         goto end;

diff --git a/test/tls13groupselection_test.c b/test/tls13groupselection_test.c
index fcebc1e43e658a389237c8884c649452adef7427..01d1eded5f87ca85f6b9f036a9caa540bb10232b 100644 (file)
--- a/test/tls13groupselection_test.c
+++ b/test/tls13groupselection_test.c
@@ -278,33 +278,27 @@ static const struct tls13groupselection_test_st tls13groupselection_tests[] =
           CLIENT_PREFERENCE,
           SYNTAX_FAILURE
         },
-        /* test 33 remove all groups */
-        { "X25519:secp256r1:X448:secp521r1:-X448:-secp256r1:-X25519:-secp521r1",
-          "",
-          CLIENT_PREFERENCE,
-          SYNTAX_FAILURE
-        },
-        { "X25519:??secp256r1:X448", /* test 34 */
+        { "X25519:??secp256r1:X448", /* test 33 */
           "",
           CLIENT_PREFERENCE,
           SYNTAX_FAILURE
         },
-        { "X25519:secp256r1:**X448", /* test 35 */
+        { "X25519:secp256r1:**X448", /* test 34 */
           "",
           CLIENT_PREFERENCE,
           SYNTAX_FAILURE
         },
-        { "--X25519:secp256r1:X448", /* test 36 */
+        { "--X25519:secp256r1:X448", /* test 35 */
           "",
           CLIENT_PREFERENCE,
           SYNTAX_FAILURE
         },
-        { "-DEFAULT",  /* test 37 */
+        { "-DEFAULT",  /* test 36 */
           "",
           CLIENT_PREFERENCE,
           SYNTAX_FAILURE
         },
-        { "?DEFAULT",  /* test 38 */
+        { "?DEFAULT",  /* test 37 */
           "",
           CLIENT_PREFERENCE,
           SYNTAX_FAILURE
@@ -313,6 +307,12 @@ static const struct tls13groupselection_test_st tls13groupselection_tests[] =
          * Negotiation Failures
          * No overlapping groups between client and server
          */
+        /* test 38 remove all groups */
+        { "X25519:secp256r1:X448:secp521r1:-X448:-secp256r1:-X25519:-secp521r1",
+          "",
+          CLIENT_PREFERENCE,
+          NEGOTIATION_FAILURE
+        },
         { "secp384r1:secp521r1:X25519", /* test 39 */
           "prime256v1:X448",
           CLIENT_PREFERENCE,