Skip to content

Commit e9f2729

Browse files
committed
Fix: heap-use-after-free in merge_patch when patch is subtree of target
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>
1 parent fb16e5c commit e9f2729

2 files changed

Lines changed: 40 additions & 2 deletions

File tree

cJSON_Utils.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1324,9 +1324,13 @@ static cJSON *merge_patch(cJSON *target, const cJSON * const patch, const cJSON_
13241324

13251325
if (!cJSON_IsObject(patch))
13261326
{
1327-
/* scalar value, array or NULL, just duplicate */
1327+
/* scalar value, array or NULL, just duplicate.
1328+
* Duplicate the patch first in case it is a subtree of target,
1329+
* otherwise cJSON_Delete(target) would free the patch memory
1330+
* and the subsequent cJSON_Duplicate would read freed memory. */
1331+
cJSON *duplicate = cJSON_Duplicate(patch, 1);
13281332
cJSON_Delete(target);
1329-
return cJSON_Duplicate(patch, 1);
1333+
return duplicate;
13301334
}
13311335

13321336
if (!cJSON_IsObject(target))

tests/old_utils_tests.c

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,39 @@ static void merge_tests(void)
189189
}
190190
}
191191

192+
static void merge_patch_should_not_read_freed_memory_when_patch_is_subtree(void)
193+
{
194+
/* When patch is a subtree of target, merge_patch must duplicate the patch
195+
* before deleting target. Otherwise cJSON_Delete(target) frees the patch
196+
* memory and the subsequent cJSON_Duplicate reads freed memory (UAF).
197+
* See CVE candidate: heap-use-after-free in merge_patch (cJSON_Utils.c). */
198+
cJSON *target = cJSON_Parse("{\"a\":[1,2,3]}");
199+
cJSON *patch = cJSON_GetObjectItem(target, "a");
200+
cJSON *result = NULL;
201+
cJSON *first = NULL;
202+
cJSON *second = NULL;
203+
cJSON *third = NULL;
204+
205+
TEST_ASSERT_NOT_NULL(target);
206+
TEST_ASSERT_NOT_NULL(patch);
207+
208+
/* patch (array [1,2,3]) is a subtree of target. This used to trigger
209+
* heap-use-after-free under AddressSanitizer before the fix. */
210+
result = cJSONUtils_MergePatch(target, patch);
211+
TEST_ASSERT_NOT_NULL(result);
212+
TEST_ASSERT_TRUE(cJSON_IsArray(result));
213+
TEST_ASSERT_EQUAL_INT(3, cJSON_GetArraySize(result));
214+
215+
first = cJSON_GetArrayItem(result, 0);
216+
second = cJSON_GetArrayItem(result, 1);
217+
third = cJSON_GetArrayItem(result, 2);
218+
TEST_ASSERT_EQUAL_INT(1, first->valueint);
219+
TEST_ASSERT_EQUAL_INT(2, second->valueint);
220+
TEST_ASSERT_EQUAL_INT(3, third->valueint);
221+
222+
cJSON_Delete(result);
223+
}
224+
192225
static void generate_merge_tests(void)
193226
{
194227
size_t i = 0;
@@ -219,6 +252,7 @@ int main(void)
219252
RUN_TEST(misc_tests);
220253
RUN_TEST(sort_tests);
221254
RUN_TEST(merge_tests);
255+
RUN_TEST(merge_patch_should_not_read_freed_memory_when_patch_is_subtree);
222256
RUN_TEST(generate_merge_tests);
223257

224258
return UNITY_END();

0 commit comments

Comments
 (0)