From 7391f9d7394c0badb7ca605561e0af8d2ea13b4c Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgallagh@redhat.com>
Date: Wed, 1 May 2024 13:09:58 -0400
Subject: [PATCH] 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 <sgallagh@redhat.com>
---
 src/dhparams.c             | 33 +++++++++++++++++++++++++--------
 test/named_dhparams_test.c | 23 ++++++++++++-----------
 2 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/src/dhparams.c b/src/dhparams.c
index 5d03db0..1db5130 100644
--- 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
index e66dee6..af4492b 100644
--- a/test/named_dhparams_test.c
+++ b/test/named_dhparams_test.c
@@ -47,6 +47,7 @@ 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;
@@ -54,22 +55,22 @@ test_group_name_list (void)
     }
 
 #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;