Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double Free Vulnerability Discovered in cJSON_Delete Function Through Fuzzing #833

Open
wyywang opened this issue Feb 29, 2024 · 6 comments

Comments

@wyywang
Copy link

wyywang commented Feb 29, 2024

During a recent fuzzing session aimed at identifying memory management vulnerabilities within the cJSON library, a double free issue was uncovered in the cJSON_Delete function. This vulnerability poses a significant risk as it could potentially lead to security flaws like crashes or, in worse scenarios, arbitrary code execution if exploited. The issue arises because the function does not set the freed pointers to NULL, which might lead to their unintended reuse.

CJSON_PUBLIC(void) cJSON_Delete(cJSON *item)
{
    cJSON *next = NULL;
    while (item != NULL)
    {
        next = item->next;
        if (!(item->type & cJSON_IsReference) && (item->child != NULL))
        {
            cJSON_Delete(item->child);
        }
        if (!(item->type & cJSON_IsReference) && (item->valuestring != NULL))
        {
            global_hooks.deallocate(item->valuestring);
        }
        if (!(item->type & cJSON_StringIsConst) && (item->string != NULL))
        {
            global_hooks.deallocate(item->string);
        }
        global_hooks.deallocate(item);
        item = next;
    }
}

To mitigate this issue, it is recommended to set pointers to NULL immediately after they are deallocated. This change will prevent the library from accessing or freeing already freed memory, thus avoiding double free vulnerabilities.
Example of suggested change:

if (!(item->type & cJSON_IsReference) && (item->valuestring != NULL)) {
    global_hooks.deallocate(item->valuestring);
    item->valuestring = NULL; // Prevent double free by setting pointer to NULL
}

fuzzcode:

#include "cJSON.h"
#include <stdbool.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <math.h>
typedef uint8_t   u8;   
typedef uint16_t  u16;  
typedef uint32_t  u32;  
typedef uint64_t  u64;
typedef unsigned int usize;
typedef int8_t  i8;
typedef int16_t i16;
typedef int32_t i32;
typedef int64_t i64;
typedef int isize;
typedef float f32;
typedef double f64;
int main() {
    cJSON v0_tmp[] = {{ NULL, NULL, NULL, 1, NULL, 67, -7.939443733432238, NULL,  }, }; // child
    cJSON *v0 = malloc(sizeof v0_tmp);
    memcpy(v0, v0_tmp, sizeof v0_tmp);
    cJSON v1_tmp[] = {{ NULL, NULL, v0, 76, NULL, 35, 62.02300219225285, NULL,  }, }; // parent
    cJSON *v1 = malloc(sizeof v1_tmp);
    memcpy(v1, v1_tmp, sizeof v1_tmp);
    cJSON *v2 = v1; // parent
    cJSON v3_tmp[] = {{ NULL, NULL, NULL, 9, NULL, 32890, 56.250703290040846, NULL,  }, }; // replacement
    cJSON *v3 = malloc(sizeof v3_tmp);
    memcpy(v3, v3_tmp, sizeof v3_tmp);
    cJSON *v4 = v3; // replacement
    i8 v5_tmp[] = {123, 34, 34, 58, 123, 34, 34, 58, 116, 114, 117, 101, 125, 44, 34, 34, 58, 123, 34, 34, 58, 102, 97, 108, 115, 101, 125, 44, 34, 92, 117, 48, 48, -79, 101, 92, 117, 48, 48, 49, 52, 47, 100, -84, 62, 34, 58, 91, 123, 10, 9, 34, -22, 100, -83, 34, 58, 123, 34, 34, 58, 110, 117, 108, 108, 125, 44, 34, 34, 58, 123, 34, 34, 58, 110, 117, 108, 108, 125, 44, 34, 34, 58, 123, 34, 34, 58, 13, 102, 97, 108, 115, 101, 125, 125, 93, 125, -71, -96, -20, -16, 98, -116, 93, 64, 0, 0, 0, 49, 101, 92, 117, 48, 48, 49, 52, 47, 100, -84, 62, 34, 58, 9, 112, 50, -55, -2, -22, 100, -84, 62, 34, 58, 123, 48, 51, 92, 117, -21, 48, 49, 97, 92, 92, 57, 0, }; // value
    i8 *v5 = malloc(sizeof v5_tmp);
    memcpy(v5, v5_tmp, sizeof v5_tmp);
    i8 *v6 = v5; // value
    cJSON *v7 = cJSON_Parse(v6); // item
    if (v7 == NULL) return 0;
    cJSON *v9 = v7; // item
    i8 v10_tmp[] = {35, 0, }; // name
    i8 *v10 = malloc(sizeof v10_tmp);
    memcpy(v10, v10_tmp, sizeof v10_tmp);
    i8 *v11 = v10; // name
    f64 v12 = INFINITY; // number
    cJSON *v13 = cJSON_AddNumberToObject(v9, v11, v12); // $relative
    i8 v14_tmp[] = {82, 0, }; // name
    i8 *v14 = malloc(sizeof v14_tmp);
    memcpy(v14, v14_tmp, sizeof v14_tmp);
    i8 *v15 = v14; // name
    cJSON *v16 = cJSON_AddNullToObject(v9, v15); // $relative
    i32 v17 = 4; // which
    i32 v18 = 4; // which
    cJSON v19_tmp[] = {{ NULL, NULL, NULL, 56, NULL, 1, 7.034879206248807, NULL,  }, }; // next
    cJSON *v19 = malloc(sizeof v19_tmp);
    memcpy(v19, v19_tmp, sizeof v19_tmp);
    cJSON v20_tmp[] = {{ v19, NULL, NULL, 64, NULL, -1, 51.96094189122921, NULL,  }, }; // newitem
    cJSON *v20 = malloc(sizeof v20_tmp);
    memcpy(v20, v20_tmp, sizeof v20_tmp);
    cJSON *v21 = v20; // newitem
    i8 v22_tmp[] = {-97, 0, }; // name
    i8 *v22 = malloc(sizeof v22_tmp);
    memcpy(v22, v22_tmp, sizeof v22_tmp);
    i8 *v23 = v22; // name
    f64 v24 = INFINITY; // number
    cJSON *v25 = cJSON_AddNumberToObject(v21, v23, v24); // $relative
    i8 v26_tmp[] = {4, 0, }; // name
    i8 *v26 = malloc(sizeof v26_tmp);
    memcpy(v26, v26_tmp, sizeof v26_tmp);
    i8 *v27 = v26; // name
    cJSON *v28 = cJSON_AddArrayToObject(v21, v27); // $relative
    i8 v29_tmp[] = {-115, 0, }; // name
    i8 *v29 = malloc(sizeof v29_tmp);
    memcpy(v29, v29_tmp, sizeof v29_tmp);
    i8 *v30 = v29; // name
    cJSON *v31 = cJSON_AddObjectToObject(v21, v30); // $relative
    i32 v32 = cJSON_InsertItemInArray(v9, v17, v21); // $relative
    i8 v33_tmp[] = {-68, 0, }; // name
    i8 *v33 = malloc(sizeof v33_tmp);
    memcpy(v33, v33_tmp, sizeof v33_tmp);
    i8 *v34 = v33; // name
    i32 v35 = 0; // boolean
    cJSON *v36 = cJSON_AddBoolToObject(v9, v34, v35); // $relative
    i32 v37 = cJSON_InsertItemInArray(v9, v18, v21); // $relative
    i32 v38 = cJSON_ReplaceItemViaPointer(v2, v9, v4); // $target
}

@Up-wind
Copy link
Contributor

Up-wind commented Apr 8, 2024

Hello, I noticed that the style of your fuzzcode is a bit like Hopper. May I ask if you have made any modifications to Hopper to achieve this?

@wyywang
Copy link
Author

wyywang commented Apr 8, 2024

@Up-wind Thank you for your interest and question. Yes, this work was a collective effort by our team. To achieve the fuzzing results, we indeed made some custom modifications to Hopper, but for various reasons, we are currently unable to open-source this customized version. Fortunately, the double free vulnerability we reported has been identified and fixed. Regarding the assignment of a CVE number, we are also looking forward to it, but that decision rests with the relevant security organizations based on their assessment and processes. We have submitted all the necessary information following the standard procedures and hope to receive a response soon. Thank you again for your attention and understanding.

@wyywang wyywang closed this as completed Apr 8, 2024
@wyywang wyywang reopened this Apr 8, 2024
@PeterAlfredLee
Copy link
Contributor

Looks good.
Seems we can also do similar things to item->string.

@Alanscut
Copy link
Collaborator

A pr is always welcome. :)

Alanscut added a commit to Alanscut/cJSON that referenced this issue May 9, 2024
Add some tests for setting NULL to deallocated pointers
releated to DaveGamble#842 and DaveGamble#833
Alanscut added a commit to Alanscut/cJSON that referenced this issue May 9, 2024
Add some tests for setting NULL to deallocated pointers
releated to DaveGamble#842 and DaveGamble#833
@wyywang
Copy link
Author

wyywang commented May 9, 2024

A pr is always welcome. :)

Thank you for addressing the issue so quickly. To track and document this vulnerability properly, could we initiate the process to obtain a CVE number for the "Double Free Vulnerability in cJSON_Delete Function"? This will help in acknowledging the security implications and ensuring it is recognized and addressed appropriately in various security advisories and databases.

@PeterAlfredLee
Copy link
Contributor

As the recent security policy declares, I think we should discuss in mails.

Alanscut added a commit that referenced this issue May 13, 2024
Add some tests for setting NULL to deallocated pointers
releated to #842 and #833
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants