Use-after-free in cJSONUtils_ApplyPatches when a patch removes the patch array
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:14A 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).
Source: DaveGamble/cJSON