huamuyichun commented 4 months ago

Bug Report

Required Info:

Operating System: Debian GNU/Linux 12 (bookworm)
FreeRTOS Version: V10.5.1 (ESP-IDF SMP modified)
ESP-IDF Version: v5.4-dev-78-gd4cd437ede-dirty
Commit hash: d4cd437ede613fffacc06ac6d6c93a083829022f
Component: cJSON

Steps to reproduce issue:

Hi, while fuzz testing FreeRTOS using Syzkaller, I encountered an illegal memory access error in the cJSON module. this is my SPEC which can trigger the bug

static long syz_cjson_parse(volatile long a0)
    {
        const char *json_string = (const char *)a0;

        // Parse the JSON string
        cJSON *json = cJSON_Parse(json_string);
        if (json == NULL) {
            return -1; // Return error code
        }

        // Delete the cJSON object
        cJSON_Delete(json);

        return 0; // Success
    }

Error Report

During the fuzzing process, I received the following error report:

Level: 0: 1074606676, /path/to/panic_handler.c : panic_handler : 132 : 
Level: 1: 1074274120, /path/to/panic_handler.c : xt_unhandled_exception : 242 : Level: 2: 1074296803, /path/to/xtensa_vectors.S : ?? : 805 : 
Level: 3: 1074308477, /path/to/strlen.S : strlen : 82 : 
Level: 4: 1074670777, /path/to/cJSON.c : cJSON_ParseWithOpts : 1094 : 
Level: 5: 1074670817, /path/to/cJSON.c : cJSON_Parse : 1182 : 
Level: 6: 1074681688, /path/to/common_freertos.h : syz_cjson_parse : 1281 : Level: 7: 1074672916, /path/to/executor.h : execute_syscall : 347 : 
Level: 8: 1074686731, /path/to/executor.h : fuzz_start_one : 543 : 
Level: 9: 1074686940, /path/to/executor.h : executor_main : 579 : 
Level: 10: 1074687402, /path/to/hello_world_main.c : app_main : 64 : 
Level: 11: 1075129006, /path/to/app_startup.c : main_task : 208 : 
Level: 12: 1074320224, /path/to/port.c : vPortTaskWrapper : 134

Guru Meditation Error: Core  0 panic'ed (LoadProhibited). Exception was unhandled.

Backtrace: 0x4008a57a:0x3ffc0f70 0x400e2cb6:0x3ffc0f80 0x400e2cde:0x3ffc0fa0 0x400e5755:0x3ffc0fc0 0x400e3511:0x3ffc0ff0 0x400e6b08:0x3ffc1040 0x400e6bd9:0x3ffc11f0 0x400e6da7:0x3ffc1210 0x40152aab:0x3ffc12d0 0x4008d35d:0x3ffc1300

It appears there is an illegal access at components/json/cJSON/cJSON.c in the cJSON_ParseWithOpts function. The specific error occurs on this linebuffer_length = strlen(value) + sizeof(""); If the value string is pointing to an illegal memory address, then an illegal memory access occurs when strlen() is called. So in this function you should first determine if the pointer is pointing to the correct memory address, and then call the strlen() function.

CJSON_PUBLIC(cJSON *) cJSON_ParseWithOpts(const char *value, const char **return_parse_end, cJSON_bool require_null_terminated)
{
    size_t buffer_length;

    if (NULL == value)
    {
        return NULL;
    }

    /* Adding null character size due to require_null_terminated. */
    buffer_length = strlen(value) + sizeof("");

    return cJSON_ParseWithLengthOpts(value, buffer_length, return_parse_end, require_null_terminated);
}

Expected Behavior

The program should execute without any memory errors or crashes.

Actual Behavior

The program crashes with an unhandled exception indicating an illegal memory access error.

Additional Information

I would greatly appreciate it if you could review this bug report. Any suggestions or feedback you can provide would be very helpful. Thank you for your time.

snake-4 commented 4 months ago

So in this function you should first determine if the pointer is pointing to the correct memory address, and then call the strlen() function.

This is impossible to do in a portable manner. Also, your spec is UB, you can't cast an invalid pointer value. It is safe to assume that your program won't work correctly if you invoke UB. C is a memory unsafe language.

imaami commented 4 months ago

You're passing an invalid memory address to a public API function. It expects the caller to pass a pointer to a valid address.

The function already checks for a null pointer value, and that's all it can do. There is no magic pointer validating in low-level code. The responsibility of not feeding garbage pointers is on the programmer who calls the API methods. This is not a philosophical question but a practical reality.

PeterAlfredLee commented 3 months ago