A global-buffer-overflow found in cJSON_ParseWithLengthOpts

[Anza2001]

Hi, I found a global-buffer-overflow when fuzzing：

==6675==ERROR: AddressSanitizer: global-buffer-overflow on address 0x5584b59ea025 at pc 0x5584b59e1653 bp 0x7fff64037180 sp 0x7fff64037170
READ of size 1 at 0x5584b59ea025 thread T0
    #0 0x5584b59e1652 in buffer_skip_whitespace /home/anza/cjson_verify/cJSON.c:1059
    #1 0x5584b59e5f52 in cJSON_ParseWithLengthOpts /home/anza/cjson_verify/cJSON.c:1129
    #2 0x5584b59e139c in main /home/anza/cjson_verify/mytest.c:10
    #3 0x7f3e481e6082 in __libc_start_main ../csu/libc-start.c:308
    #4 0x5584b59e143d in _start (/home/anza/cjson_verify/a.out+0x243d)

I reproduce this in Ubuntu20.04. poc.zip

[imaami]

Well, one could say your PoC is buggy. You're passing an input string that's 5 bytes long, but claiming the length is 16. If your input data is bogus (as is the case when sizeof is misused like the PoC does) then you're going to have a bad time.

But on the other hand, fuzzing is supposed to probe what happens when you misuse the public API. So fair game.

The problem is that the whitespace-skipping function skips any byte value 0x20 or less, including 0. If the intention is to explicitly allow embedded '\0', then I suppose the whitespace skipping implementation is "only" a dangerous thing to wield, and places a lot of responsibility on the library user. Otherwise I'd say it's a bug.

I actually haven't read the API documentation yet, please excuse my laziness. I have no idea if null bytes are mentioned therein. In my opinion, instead of checking <= 32 it would be safer to stop if a null byte is encountered. Oh and to nitpick: it's best not to hardcode the value of a space, just use the character literal ' '.

[PeterAlfredLee]

To address this, copied from your test

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "cJSON.h"

int main(){
    cJSON *json;
    const char *data = "\012\012\012\012";
    const size_t size = 4;
    json = cJSON_ParseWithLengthOpts(data, sizeof(data) + 8, NULL, 0);
    cJSON_Delete(json);
    return 0;
}

cJSON_ParseWithLengthOpts is designed to believe the input buffer_length matches the actual length of input value, while you are passing a invalid buffer length to cJSON_ParseWithLengthOpts. To be more clear, the buffer_skip_whitespace in cJSON_ParseWithLengthOpts skipped all bytes equal or less than 0x20, to skip whitespace, cr/lf or something else. As @imaami said, it also skipped \0, which is 0x0. It is designed like this long before.

[Anza2001]

Appreciate each reply. It appears the bug is caused by misuse of API. Before then I thought there should be a length check, but if cJSON_ParseWithLengthOpts is designed to receive a buffer matching the given length, there should be no argument.

[imaami]

To address this, copied from your test

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "cJSON.h"

int main(){
    cJSON *json;
    const char *data = "\012\012\012\012";
    const size_t size = 4;
    json = cJSON_ParseWithLengthOpts(data, sizeof(data) + 8, NULL, 0);
    cJSON_Delete(json);
    return 0;
}

cJSON_ParseWithLengthOpts is designed to believe the input buffer_length matches the actual length of input value, while you are passing a invalid buffer length to cJSON_ParseWithLengthOpts. To be more clear, the buffer_skip_whitespace in cJSON_ParseWithLengthOpts skipped all bytes equal or less than 0x20, to skip whitespace, cr/lf or something else. As @imaami said, it also skipped \0, which is 0x0. It is designed like this long before.

While it does the job, skipping every value up to 0x20 is against the JSON spec. There are only four whitespace characters: 0x09, 0x0a, 0x0d, and 0x20. That by itself makes the current implementation a bug regardless. Reading past a null byte, even if it happens to be within the input buffer boundary, is no different than accepting any other invalid character. The parser should stop immediately if it encounters any of 0x00 to 0x08, 0x0b, 0x0c, or 0x0e to 0x1f.