Use-after-free in cJSONUtils_ApplyPatches when a patch removes the patch array

Start in cJSON_Utils.c at cJSONUtils_ApplyPatches() and cJSONUtils_ApplyPatchesCaseSensitive(), which both walk the patch list while applying changes. Run the supplied self-referential example under AddressSanitizer, then run ctest; done means both variants complete without use-after-free and the existing 22 tests pass.

cJSONUtils_ApplyPatches() walks the patch array through current_patch->next, while apply_patch() can delete any part of object. If the patch array is itself part of the document being patched and one of the operations removes it, the loop reads next from a freed node:

#include <stdio.h>
#include "cJSON.h"
#include "cJSON_Utils.h"

int main(void)
{
    cJSON *doc = cJSON_Parse("{\"patches\":[{\"op\":\"remove\",\"path\":\"/patches\"}]}");
    cJSON *patches = cJSON_GetObjectItemCaseSensitive(doc, "patches");

    int rc = cJSONUtils_ApplyPatches(doc, patches);
    printf("rc=%d\n", rc);
    cJSON_Delete(doc);
    return 0;
}

With -fsanitize=address:

==304425==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000000e0 at pc 0x555555644eea bp 0x7fffffffd290 sp 0x7fffffffd288
READ of size 8 at 0x6060000000e0 thread T0
    #0 in cJSONUtils_ApplyPatches cJSON_Utils.c:1061:40
    #1 in main apply_patches_self.c:10:14

0x6060000000e0 is located 0 bytes inside of 64-byte region [0x6060000000e0,0x606000000120)
freed by thread T0 here:
    #0 in free <null>
    #1 in cJSON_Delete cJSON.c:273:9
    #2 in cJSON_Delete cJSON.c:261:13
    #3 in apply_patch cJSON_Utils.c:896:9
    #4 in cJSONUtils_ApplyPatches cJSON_Utils.c:1056:18
    #5 in main apply_patches_self.c:10:14

A normal build segfaults. cJSONUtils_ApplyPatchesCaseSensitive() has the same loop and fails the same way (cJSON_Utils.c:1090).

Passing a patch list that lives inside the target is unusual, but nothing in the API rules it out. #1065 recently fixed the same situation in cJSONUtils_MergePatch(), where the patch is a subtree of the target.

Reproduced on v1.7.19 and on current master (6d9f244), x86_64 Linux, clang 15.

One way to fix it is to iterate over a copy of the patch list. apply_patch() already duplicates the value it inserts, so nothing in object ends up pointing into the copy. The cost is one cJSON_Duplicate() of the patches per call. If you'd rather not pay that, documenting that patches must not be part of object would also work.

--- a/cJSON_Utils.c
+++ b/cJSON_Utils.c
@@ -1038,6 +1038,7 @@ cleanup:
 CJSON_PUBLIC(int) cJSONUtils_ApplyPatches(cJSON * const object, const cJSON * const patches)
 {
     const cJSON *current_patch = NULL;
+    cJSON *patches_copy = NULL;
     int status = 0;
 
     if (!cJSON_IsArray(patches))
@@ -1046,27 +1047,34 @@ CJSON_PUBLIC(int) cJSONUtils_ApplyPatches(cJSON * const object, const cJSON * co
         return 1;
     }
 
-    if (patches != NULL)
+    /* iterate over a copy: patches may be part of object, and applying a
+     * patch can delete it */
+    patches_copy = cJSON_Duplicate(patches, true);
+    if (patches_copy == NULL)
     {
-        current_patch = patches->child;
+        return 1;
     }
 
+    current_patch = patches_copy->child;
     while (current_patch != NULL)
     {
         status = apply_patch(object, current_patch, false);
         if (status != 0)
         {
+            cJSON_Delete(patches_copy);
             return status;
         }
         current_patch = current_patch->next;
     }
 
+    cJSON_Delete(patches_copy);
     return 0;
 }
 
 CJSON_PUBLIC(int) cJSONUtils_ApplyPatchesCaseSensitive(cJSON * const object, const cJSON * const patches)
 {
     const cJSON *current_patch = NULL;
+    cJSON *patches_copy = NULL;
     int status = 0;
 
     if (!cJSON_IsArray(patches))
@@ -1075,21 +1083,27 @@ CJSON_PUBLIC(int) cJSONUtils_ApplyPatchesCaseSensitive(cJSON * const object, con
         return 1;
     }
 
-    if (patches != NULL)
+    /* iterate over a copy: patches may be part of object, and applying a
+     * patch can delete it */
+    patches_copy = cJSON_Duplicate(patches, true);
+    if (patches_copy == NULL)
     {
-        current_patch = patches->child;
+        return 1;
     }
 
+    current_patch = patches_copy->child;
     while (current_patch != NULL)
     {
         status = apply_patch(object, current_patch, true);
         if (status != 0)
         {
+            cJSON_Delete(patches_copy);
             return status;
         }
         current_patch = current_patch->next;
     }
 
+    cJSON_Delete(patches_copy);
     return 0;
 }
 

With this change the example (and its CaseSensitive variant) prints rc=0 and is clean under ASan, and ctest passes (22/22).