diff --git a/libpromises/evalfunction.c b/libpromises/evalfunction.c index d42df8c6fca..87aea35969c 100644 --- a/libpromises/evalfunction.c +++ b/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); diff --git a/tests/unit/evalfunction_test.c b/tests/unit/evalfunction_test.c index fcfd2f18a53..70d04f0dbf2 100644 --- a/tests/unit/evalfunction_test.c +++ b/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);