fix invalid memory access in parse_string

[Calcor]

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]

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

[niooss-ledger]

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

[PeterAlfredLee]

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

[niooss-ledger]

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 https://github.com/DaveGamble/cJSON/commit/3ef4e4e730e5efd381be612df41e1ff3f5bb3c32, 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 https://github.com/DaveGamble/cJSON/pull/852.

[sbvoxel]

@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]

@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 */
        }