Make valid_dh_group_names() more robust

Avoids the possibility of a NULL-dereference if the DH groups are ever empty. Also cleans up the test to make it easier to spot when the expected value differs from the received value. Signed-off-by: Stephen Gallagher <[email protected]>
sgallagher · May 1, 2024 · 7391f9d · 7391f9d
1 parent dea921d
commit 7391f9d
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 19 deletions.
diff --git a/src/dhparams.c b/src/dhparams.c
@@ -31,6 +31,7 @@
 */
 
 #include <assert.h>
+#include <stdbool.h>
 #include <string.h>
 
 #include <openssl/evp.h>
@@ -84,13 +85,23 @@ char *
 valid_dh_group_names (TALLOC_CTX *mem_ctx)
 {
   size_t i;
-  char *names = NULL;
   TALLOC_CTX *tmp_ctx = talloc_new (NULL);
+  char *names = NULL;
+  bool first = true;
 
   i = 0;
   while (dh_fips_groups[i])
     {
-      names = talloc_asprintf_append (names, "%s, ", dh_fips_groups[i]);
+      if (first)
+        {
+          names = talloc_strdup (tmp_ctx, dh_fips_groups[i]);
+          first = false;
+        }
+      else
+        {
+          names = talloc_asprintf_append (names, ", %s", dh_fips_groups[i]);
+        }
+
       if (!names)
         goto done;
 
@@ -100,18 +111,24 @@ valid_dh_group_names (TALLOC_CTX *mem_ctx)
   i = 0;
   while (dh_nonfips_groups[i])
     {
-      names = talloc_asprintf_append (names, "%s, ", dh_nonfips_groups[i]);
+      if (first)
+        {
+          /* This should never be reached, since dh_fips_groups should always
+           * have at least one entry, but for safety we will include it.
+          */
+          names = talloc_strdup (tmp_ctx, dh_nonfips_groups[i]);
+          first = false;
+        }
+      else
+        {
+          names = talloc_asprintf_append (names, ", %s", dh_nonfips_groups[i]);
+        }
       if (!names)
         goto done;
 
       i++;
     }
 
-  /* Truncate the last ", " */
-  names = talloc_strndup (names, names, strlen (names) - 2);
-  if (!names)
-    goto done;
-
   talloc_steal (mem_ctx, names);
 
 done:

diff --git a/test/named_dhparams_test.c b/test/named_dhparams_test.c
@@ -47,29 +47,30 @@ test_group_name_list (void)
   int ret;
   TALLOC_CTX *tmp_ctx = talloc_new (NULL);
   char *names = valid_dh_group_names (tmp_ctx);
+  char *expected = NULL;
   if (!names)
     {
       ret = EINVAL;
       goto done;
     }
 
 #if OPENSSL_VERSION_NUMBER < 0x30000000L
-  if (strcmp (names,
-              "ffdhe2048, ffdhe3072, ffdhe4096, ffdhe6144, ffdhe8192") != 0)
-    {
-      ret = EINVAL;
-      goto done;
-    }
+  expected = talloc_strdup (
+    tmp_ctx, "ffdhe2048, ffdhe3072, ffdhe4096, ffdhe6144, ffdhe8192");
 #else
-  if (strcmp (names,
-              "ffdhe2048, ffdhe3072, ffdhe4096, ffdhe6144, ffdhe8192, "
-              "modp_2048, modp_3072, modp_4096, modp_6144, modp_8192, "
-              "modp_1536, dh_1024_160, dh_2048_224, dh_2048_256") != 0)
+  expected =
+    talloc_strdup (tmp_ctx,
+                   "ffdhe2048, ffdhe3072, ffdhe4096, ffdhe6144, ffdhe8192, "
+                   "modp_2048, modp_3072, modp_4096, modp_6144, modp_8192, "
+                   "modp_1536, dh_1024_160, dh_2048_224, dh_2048_256");
+#endif
+  if (strcmp (names, expected) != 0)
     {
+      fprintf (stderr, "Expected: [%s]\n", expected);
+      fprintf (stderr, "Recieved: [%s]\n", names);
       ret = EINVAL;
       goto done;
     }
-#endif
 
 
   ret = EOK;