SSL_set1_groups_list(): Fix memory corruption with 40 groups and more

Fixes #23624

The calculation of the size for gid_arr reallocation was wrong.
A multiplication by gid_arr array item size was missing.

Testcase is added.

Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/23625)

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index de82d2b33a1e621874822311d380fe7229ccf969..ed40f1091b51d881fd95006714883d07b7727307 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1049,7 +1049,8 @@ static int gid_cb(const char *elem, int len, void *arg)
         return 0;
     if (garg->gidcnt == garg->gidmax) {
         uint16_t *tmp =
-            OPENSSL_realloc(garg->gid_arr, garg->gidmax + GROUPLIST_INCREMENT);
+            OPENSSL_realloc(garg->gid_arr,
+                            (garg->gidmax + GROUPLIST_INCREMENT) * sizeof(*garg->gid_arr));
         if (tmp == NULL)
             return 0;
         garg->gidmax += GROUPLIST_INCREMENT;

diff --git a/test/sslapitest.c b/test/sslapitest.c
index 243488020e62239cfcce1ac2c1cf9c54d0d8816d..3b0b21aa56e61397f8cf2db9dd8db199a1afc0a5 100644 (file)
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -9652,20 +9652,11 @@ static int test_pluggable_group(int idx)
     OSSL_PROVIDER *tlsprov = OSSL_PROVIDER_load(libctx, "tls-provider");
     /* Check that we are not impacted by a provider without any groups */
     OSSL_PROVIDER *legacyprov = OSSL_PROVIDER_load(libctx, "legacy");
-    const char *group_name = idx == 0 ? "xorgroup" : "xorkemgroup";
+    const char *group_name = idx == 0 ? "xorkemgroup" : "xorgroup";
 
     if (!TEST_ptr(tlsprov))
         goto end;
 
-    if (legacyprov == NULL) {
-        /*
-         * In this case we assume we've been built with "no-legacy" and skip
-         * this test (there is no OPENSSL_NO_LEGACY)
-         */
-        testresult = 1;
-        goto end;
-    }
-
     if (!TEST_true(create_ssl_ctx_pair(libctx, TLS_server_method(),
                                        TLS_client_method(),
                                        TLS1_3_VERSION,
@@ -9675,7 +9666,9 @@ static int test_pluggable_group(int idx)
                                              NULL, NULL)))
         goto end;
 
-    if (!TEST_true(SSL_set1_groups_list(serverssl, group_name))
+    /* ensure GROUPLIST_INCREMENT (=40) logic triggers: */
+    if (!TEST_true(SSL_set1_groups_list(serverssl, "xorgroup:xorkemgroup:dummy1:dummy2:dummy3:dummy4:dummy5:dummy6:dummy7:dummy8:dummy9:dummy10:dummy11:dummy12:dummy13:dummy14:dummy15:dummy16:dummy17:dummy18:dummy19:dummy20:dummy21:dummy22:dummy23:dummy24:dummy25:dummy26:dummy27:dummy28:dummy29:dummy30:dummy31:dummy32:dummy33:dummy34:dummy35:dummy36:dummy37:dummy38:dummy39:dummy40:dummy41:dummy42:dummy43"))
+    /* removing a single algorithm from the list makes the test pass */
             || !TEST_true(SSL_set1_groups_list(clientssl, group_name)))
         goto end;
 

diff --git a/test/tls-provider.c b/test/tls-provider.c
index 5f1479435f78abff6fd1221c33e45e8b1c8f22c7..e5737dc5df0e55888d2407e186905a93fd4c8718 100644 (file)
--- a/test/tls-provider.c
+++ b/test/tls-provider.c
@@ -410,6 +410,8 @@ static int tls_prov_get_capabilities(void *provctx, const char *capability,
             }
             dummygroup[0].data = dummy_group_names[i];
             dummygroup[0].data_size = strlen(dummy_group_names[i]) + 1;
+            /* assign unique group IDs also to dummy groups for registration */
+            *((int *)(dummygroup[3].data)) = 65279 - NUM_DUMMY_GROUPS + i;
             ret &= cb(dummygroup, arg);
         }
     }
@@ -3185,9 +3187,10 @@ unsigned int randomize_tls_alg_id(OSSL_LIB_CTX *libctx)
         return 0;
     /*
      * Ensure id is within the IANA Reserved for private use range
-     * (65024-65279)
+     * (65024-65279).
+     * Carve out NUM_DUMMY_GROUPS ids for properly registering those.
      */
-    id %= 65279 - 65024;
+    id %= 65279 - NUM_DUMMY_GROUPS - 65024;
     id += 65024;
 
     /* Ensure we did not already issue this id */