Fixed out-of-bounds read in string_replace with nested list elements

libpromises/evalfunction.c

@@ -9164,8 +9164,25 @@ static char *StringReplaceWithLists(const char *input_string, JsonElement *match
     const size_t len = JsonLength(match_list);
     for (size_t i = 0; i < len; i++)
     {
-        const char *m = JsonPrimitiveGetAsString(JsonAt(match_list, i));
-        const char *s = JsonPrimitiveGetAsString(JsonAt(substitute_list, i));
+        JsonElement *match_el = JsonAt(match_list, i);
+        JsonElement *substitute_el = JsonAt(substitute_list, i);
+
+        /* The elements must be primitives. JsonPrimitiveGetAsString() returns
+         * the aliased container pointer for a nested array or object, which is
+         * then used as a C string. */
+        if (JsonGetElementType(match_el) != JSON_ELEMENT_TYPE_PRIMITIVE ||
+            JsonGetElementType(substitute_el) != JSON_ELEMENT_TYPE_PRIMITIVE)
+        {
+            Log(LOG_LEVEL_WARNING,
+                "Failed to replace with function '%s', string: '%s': "
+                "match and substitute lists must contain only strings",
+                fp->name, input_string);
+            free(ret);
+            return NULL;
+        }
+
+        const char *m = JsonPrimitiveGetAsString(match_el);
+        const char *s = JsonPrimitiveGetAsString(substitute_el);
 
         char *next = SearchAndReplace(ret, m, s);
         free(ret);

tests/unit/evalfunction_test.c

@@ -214,6 +214,49 @@ static void test_format_rejects_unpassed_arguments(void)
     }
 }
 
+static FnCallResult string_replace_lists_testcase(const char *string,
+                                                  const char *match,
+                                                  const char *substitute)
+{
+    EvalContext *ctx = EvalContextNew();
+    Rlist *args = NULL;
+    RlistAppendScalar(&args, string);
+    RlistAppendScalar(&args, match);
+    RlistAppendScalar(&args, substitute);
+    RlistAppendScalar(&args, "lists");
+    FnCall *call = FnCallNew("string_replace", args);
+
+    FnCallResult res = FnCallStringReplace(ctx, NULL, call, args);
+
+    FnCallDestroy(call);
+    EvalContextDestroy(ctx);
+    return res;
+}
+
+static void test_string_replace_rejects_non_primitive_list_elements(void)
+{
+    /* Valid flat lists still work. */
+    FnCallResult res =
+        string_replace_lists_testcase("abcb", "[\"b\"]", "[\"Z\"]");
+    assert_int_equal(res.status, FNCALL_SUCCESS);
+    assert_string_equal("aZcZ", (char *) res.rval.item);
+    RvalDestroy(res.rval);
+
+    /* A substitute element that is a nested array passes the outer array and
+     * equal-length checks. The unpatched code passed it to
+     * JsonPrimitiveGetAsString(), which returned the aliased container pointer
+     * as a char *, and used it as a string (heap over-read / leak). */
+    res = string_replace_lists_testcase("abcb", "[\"b\"]", "[[\"Z\"]]");
+    assert_int_equal(res.status, FNCALL_FAILURE);
+
+    /* Same on the match side, and for a nested object. */
+    res = string_replace_lists_testcase("abcb", "[[\"b\"]]", "[\"Z\"]");
+    assert_int_equal(res.status, FNCALL_FAILURE);
+
+    res = string_replace_lists_testcase("abcb", "[\"b\"]", "[{\"k\":\"v\"}]");
+    assert_int_equal(res.status, FNCALL_FAILURE);
+}
+
 int main()
 {
     PRINT_TEST_BANNER();
@@ -223,6 +266,7 @@ int main()
         unit_test(test_basename),
         unit_test(test_module_protocol_percent_no_delimiter),
         unit_test(test_format_rejects_unpassed_arguments),
+        unit_test(test_string_replace_rejects_non_primitive_list_elements),
     };
 
     return run_tests(tests);