cJSON_Parse has buffer overflow with missing comma

[brianwyld]

Using cJSON version 1.7.14 as bundled in the Nordic Semi SDKConnect under Zephyr.

If I try to parse using cJSON_ParseWithLength(tmp_json_buffer, load_len) for a buffer containing JSON missing the comma between items, then depending on where the item is in the overall buffer I either get a parse failure:

[00:00:01.024,841] base: Failed to parse contents of allocdata.json, error is "toto":"hello"
}

or a nasty

• buffer overflow detected *

followed by a zephyr panic and a fatal error/restart.

In the first case, I have

....
}
"toto":"hello"
}

as the end of my JSON (about 8kB's worth)

In the 2nd case its the first element...

{
"toto":"hello"
"o1": {
...

Zephyr main stack size is configred to 64kB;

[PeterAlfredLee]

Hi @brianwyld. Thank you for your reporting.
It is appreciated if you can provide a test to reproduce this.

[brianwyld]

Attached JSON file that causes * buffer overflow* and crash on my build due to missing comma line 2
allocdata.json

[PeterAlfredLee]

Looks like you are trying to parse a corrupted json with cJSON_ParseWithLength. cjson does not provide a json validation feature. For corrupted json, cJSON_ParseWithLength will return null instead. What do you expect from cjson when parsing a corrupted json?

And I noticed there is a lot of content after the missing comma line 2, which I think is the root cause of overflow. Considering your json is corrupted with a missing comma, I do not have any better good idea but to validate json on the caller side.

[brianwyld]

For corrupted json, cJSON_ParseWithLength will return null instead. What do you expect from cjson when parsing a corrupted json?
Yes, I expect NULL as the JSON is not legal. The buffer overflow and subsequant crash is what I would characterise as a bug... probably due to the content after the error indeed. Is it possible to make the parser stop and return null when it gets an unexpected parse error? In a device, its best to be defensive and able to detect/reject bad input without crashing....

[PeterAlfredLee]

Is it possible to make the parser stop and return null when it gets an unexpected parse error?

Actually cjson is implemented in this way. See
https://github.com/DaveGamble/cJSON/blob/master/cJSON.c#L1706

    while (can_access_at_index(input_buffer, 0) && (buffer_at_offset(input_buffer)[0] == ','));

    if (cannot_access_at_index(input_buffer, 0) || (buffer_at_offset(input_buffer)[0] != '}'))
    {
        goto fail; /* expected end of object */
    }

As you can see, it stops parsing when a comma is missed. And it expects a '}' after it. When cjson can not find a '}', it directly go to fail section, which will handle with memory and return null.

[PeterAlfredLee]

As for the overflow, I can not reproduce it locally. IIRC cjson does not create any buffer when parsing json, it only iterate the buffer you provide when calling cJSON_ParseWithLength.

[brianwyld]

Ok, so either there is a problem with the version bundled in zephyr 1.7.14 or there is another case... I will take a look in the code to see where the 'buffer overflow detected' log comes from and how it gets there.