Fix: heap-use-after-free in merge_patch when patch is subtree of target (#1065)

When cJSONUtils_MergePatch(target, patch) is called with a non-object
patch (scalar, array, or NULL) that happens to be a subtree of target,
merge_patch() called cJSON_Delete(target) first, which freed the patch
memory, and then cJSON_Duplicate(patch, 1) read the already-freed memory,
triggering a heap-use-after-free (detected by AddressSanitizer at
cJSON_Duplicate_rec, cJSON.c:2808).

Fix: duplicate the patch first into a local variable, then delete the
target, then return the duplicate. This matches the Option B approach
proposed in issue #1060.

Verified locally:
- Reproduced the UAF with a minimal PoC under ASan before the fix.
- After the fix the PoC runs cleanly (exit 0, correct result [1,2,3]).
- Added a regression unit test
  (merge_patch_should_not_read_freed_memory_when_patch_is_subtree).
- Full ctest suite passes (22/22 tests).

Fixes #1060

Signed-off-by: lilu <lilu@kylinos.cn>

CMakeLists.txt

@@ -1,5 +1,5 @@
 set(CMAKE_LEGACY_CYGWIN_WIN32 0)
-cmake_minimum_required(VERSION 3.0)
+cmake_minimum_required(VERSION 3.5)
 
 project(cJSON
     VERSION 1.7.19

README.md

@@ -89,7 +89,7 @@ cJSON is written in ANSI C (C89) in order to support as many platforms and compi
 
 #### CMake
 
-With CMake, cJSON supports a full blown build system. This way you get the most features. CMake with an equal or higher version than 2.8.5 is supported. With CMake it is recommended to do an out of tree build, meaning the compiled files are put in a directory separate from the source files. So in order to build cJSON with CMake on a Unix platform, make a `build` directory and run CMake inside it.
+With CMake, cJSON supports a full blown build system. This way you get the most features. CMake with an equal or higher version than 3.5 is supported. With CMake it is recommended to do an out of tree build, meaning the compiled files are put in a directory separate from the source files. So in order to build cJSON with CMake on a Unix platform, make a `build` directory and run CMake inside it.
 
 ```
 mkdir build

cJSON.c

@@ -410,6 +410,11 @@ loop_end:
 /* don't ask me, but the original cJSON_SetNumberValue returns an integer or double */
 CJSON_PUBLIC(double) cJSON_SetNumberHelper(cJSON *object, double number)
 {
+    if (object == NULL)
+    {
+        return (double)NAN;
+    }
+
     if (number >= INT_MAX)
     {
         object->valueint = INT_MAX;
@@ -1598,6 +1603,11 @@ static cJSON_bool print_array(const cJSON * const item, printbuffer * const outp
         return false;
     }
 
+    if (output_buffer->depth >= CJSON_NESTING_LIMIT)
+    {
+        return false; /* nesting is too deep */
+    }
+
     /* Compose the output array. */
     /* opening square bracket */
     output_pointer = ensure(output_buffer, 1);
@@ -1778,6 +1788,11 @@ static cJSON_bool print_object(const cJSON * const item, printbuffer * const out
         return false;
     }
 
+    if (output_buffer->depth >= CJSON_NESTING_LIMIT)
+    {
+        return false; /* nesting is too deep */
+    }
+
     /* Compose the output: */
     length = (size_t) (output_buffer->format ? 2 : 1); /* fmt: {\n */
     output_pointer = ensure(output_buffer, length + 1);

cJSON_Utils.c

@@ -906,7 +906,7 @@ static int apply_patch(cJSON *object, const cJSON *patch, const cJSON_bool case_
     if ((opcode == MOVE) || (opcode == COPY))
     {
         cJSON *from = get_object_item(patch, "from", case_sensitive);
-        if (from == NULL)
+        if (!cJSON_IsString(from))
         {
             /* missing "from" for copy/move. */
             status = 4;
@@ -1324,9 +1324,13 @@ static cJSON *merge_patch(cJSON *target, const cJSON * const patch, const cJSON_
 
     if (!cJSON_IsObject(patch))
     {
-        /* scalar value, array or NULL, just duplicate */
+        /* scalar value, array or NULL, just duplicate.
+         * Duplicate the patch first in case it is a subtree of target,
+         * otherwise cJSON_Delete(target) would free the patch memory
+         * and the subsequent cJSON_Duplicate would read freed memory. */
+        cJSON *duplicate = cJSON_Duplicate(patch, 1);
         cJSON_Delete(target);
-        return cJSON_Duplicate(patch, 1);
+        return duplicate;
     }
 
     if (!cJSON_IsObject(target))

library_config/uninstall.cmake

@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 2.8.5)
+cmake_minimum_required(VERSION 3.5)
 
 set(MANIFEST "${CMAKE_CURRENT_BINARY_DIR}/install_manifest.txt")
 

tests/misc_tests.c

@@ -23,6 +23,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <math.h>
 
 #include "unity/examples/unity_config.h"
 #include "unity/src/unity.h"
@@ -478,8 +479,8 @@ static void cjson_functions_should_not_crash_with_null_pointers(void)
     TEST_ASSERT_NULL(cJSON_SetValuestring(corruptedString, "test"));
     TEST_ASSERT_NULL(cJSON_SetValuestring(item, NULL));
     cJSON_Minify(NULL);
-    /* skipped because it is only used via a macro that checks for NULL */
+    /* cJSON_SetNumberHelper should handle NULL gracefully */
-    /* cJSON_SetNumberHelper(NULL, 0); */
+    TEST_ASSERT_TRUE(isnan(cJSON_SetNumberHelper(NULL, 0)));
 
     /* restore corrupted item2 to delete it */
     item2->prev = originalPrev;

tests/old_utils_tests.c

@@ -189,6 +189,39 @@ static void merge_tests(void)
     }
 }
 
+static void merge_patch_should_not_read_freed_memory_when_patch_is_subtree(void)
+{
+    /* When patch is a subtree of target, merge_patch must duplicate the patch
+     * before deleting target. Otherwise cJSON_Delete(target) frees the patch
+     * memory and the subsequent cJSON_Duplicate reads freed memory (UAF).
+     * See CVE candidate: heap-use-after-free in merge_patch (cJSON_Utils.c). */
+    cJSON *target = cJSON_Parse("{\"a\":[1,2,3]}");
+    cJSON *patch = cJSON_GetObjectItem(target, "a");
+    cJSON *result = NULL;
+    cJSON *first = NULL;
+    cJSON *second = NULL;
+    cJSON *third = NULL;
+
+    TEST_ASSERT_NOT_NULL(target);
+    TEST_ASSERT_NOT_NULL(patch);
+
+    /* patch (array [1,2,3]) is a subtree of target. This used to trigger
+     * heap-use-after-free under AddressSanitizer before the fix. */
+    result = cJSONUtils_MergePatch(target, patch);
+    TEST_ASSERT_NOT_NULL(result);
+    TEST_ASSERT_TRUE(cJSON_IsArray(result));
+    TEST_ASSERT_EQUAL_INT(3, cJSON_GetArraySize(result));
+
+    first = cJSON_GetArrayItem(result, 0);
+    second = cJSON_GetArrayItem(result, 1);
+    third = cJSON_GetArrayItem(result, 2);
+    TEST_ASSERT_EQUAL_INT(1, first->valueint);
+    TEST_ASSERT_EQUAL_INT(2, second->valueint);
+    TEST_ASSERT_EQUAL_INT(3, third->valueint);
+
+    cJSON_Delete(result);
+}
+
 static void generate_merge_tests(void)
 {
     size_t i = 0;
@@ -219,6 +252,7 @@ int main(void)
     RUN_TEST(misc_tests);
     RUN_TEST(sort_tests);
     RUN_TEST(merge_tests);
+    RUN_TEST(merge_patch_should_not_read_freed_memory_when_patch_is_subtree);
     RUN_TEST(generate_merge_tests);
 
     return UNITY_END();