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.

Sign up for GitHub

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

Jump to bottom

fix invalid memory access in parse_string #775

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix invalid memory access in parse_string #775

wants to merge 1 commit into from

Conversation

Calcor
Copy link

@Calcor Calcor commented Aug 9, 2023 •
edited
Loading

parse_string may access invalid memory address.
Here is the poc code:

#include <stdio.h>
#include <stdlib.h>
#include <cJSON.h>
#include <sys/mman.h>

int main(int argc, char *argv[])
{

    char json_str[] = "{\"name\":\"james\",\"age\":20,";
    int str_len = strlen(json_str);

#define MMAP_SIZE 4096

    // this make sure memory after p   MMAP_SIZE is not accessable
    char *p = mmap(0xff00000000, MMAP_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_ANONYMOUS, -1, 0);
    if (p == NULL) {
        return -1;
    }
    memset(p, '\x20', MMAP_SIZE);
    memcpy(p   MMAP_SIZE- str_len, json_str, str_len);

    cJSON *json = cJSON_ParseWithLength(p, MMAP_SIZE);
    if (json == NULL) {
        printf("json parse failed\n");
        return -1;
    }

    printf("json parse succeed\n");

    cJSON_Delete(json);

    return 0;
}

gdb call stack:

Program received signal SIGSEGV, Segmentation fault.
0x0000555555556372 in parse_string (item=0x55555555f3b0, input_buffer=0x7fffffffdf80) at cJSON.c:781
781         if (buffer_at_offset(input_buffer)[0] != '\"')
(gdb) bt
#0  0x0000555555556372 in parse_string (item=0x55555555f3b0, input_buffer=0x7fffffffdf80) at cJSON.c:781
#1  0x0000555555557ca3 in parse_object (item=0x55555555f260, input_buffer=0x7fffffffdf80) at cJSON.c:1660
#2  0x0000555555557449 in parse_value (item=0x55555555f260, input_buffer=0x7fffffffdf80) at cJSON.c:1360
#3  0x0000555555556c00 in cJSON_ParseWithLengthOpts (value=0xff00000000 ' ' <repeats 200 times>..., buffer_length=4096, return_parse_end=0x0, require_null_terminated=0)
    at cJSON.c:1120
#4  0x0000555555556d4f in cJSON_ParseWithLength (value=0xff00000000 ' ' <repeats 200 times>..., buffer_length=4096) at cJSON.c:1182
#5  0x00005555555552d2 in main (argc=1, argv=0x7fffffffe128) at main.c:22
(gdb) p *input_buffer
$1 = {content = 0xff00000000 ' ' <repeats 200 times>..., length = 4096, offset = 4096, depth = 1, hooks = {allocate = 0x7ffff7e7a510 <__GI___libc_malloc>, 
    deallocate = 0x7ffff7e7ab60 <__GI___libc_free>, reallocate = 0x7ffff7e7adb0 <__GI___libc_realloc>}}
(gdb)

This PR fixed this with add more check at the entry of parse_string.

@niooss-ledger
Copy link

Hello,
I confirm this issue still occurs on current git master branch. What is blocking this PR from being merged?

@niooss-ledger
Copy link

@Alanscut can you please take a look at this fix?

@PeterAlfredLee
Copy link
Contributor

It will be appreciated if you can also attach some tests for this fix. :)

@niooss-ledger
Copy link

Hello, I tested the proof-of-concept C code again and it no longer crashes (it crashed with cJSON v1.7.17). Instead, it displays json parse failed (as expected). If I revert commit 3ef4e4e, the PoC crashes again with a segmentation fault.

Therefore I believe the issue fixed by this Pull Request was already fixed in v1.7.18 with #852.

@sbvoxel
Copy link
Contributor

sbvoxel commented May 19, 2024 •
edited
Loading

@Calcor I like your fix better than mine.

We have this kind of pattern in places:

        input_buffer->offset  ;
        buffer_skip_whitespace(input_buffer);
        if (!parse_value(current_item, input_buffer))
        {
            goto fail; /* failed to parse value */
        }

We increment the buffer, potentially into out of bounds territory. Then we try to skip whitespace. And then we try to continue parsing. And it's that code that continues parsing which everywhere else handles the out of bounds edge case. parse_value does it, and so too should parse_string.

Here is my code with context:

        if (cannot_access_at_index(input_buffer, 1))
        {
            goto fail; /* nothing comes after the comma */
        }

        /* parse the name of the child */
        input_buffer->offset  ;
        buffer_skip_whitespace(input_buffer);
        if (!parse_string(current_item, input_buffer))
        {
            goto fail; /* failed to parse name */
        }
        buffer_skip_whitespace(input_buffer);

My code does the job but it does so oddly.

@sbvoxel
Copy link
Contributor

sbvoxel commented May 19, 2024

@Alanscut For consistency and clarity, it's probably best to accept this pull request and remove the check I introduced:

        if (cannot_access_at_index(input_buffer, 1))
        {
            goto fail; /* nothing comes after the comma */
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Calcor @niooss-ledger @PeterAlfredLee @sbvoxel