akheron / jansson

C library for encoding, decoding and manipulating JSON data
http://www.digip.org/jansson/
Other
3.05k stars 809 forks source link

OOB Read memory corruption bug #548

Closed JordyZomer closed 3 years ago

JordyZomer commented 4 years ago

Hi,

I encountered an OOB read memory corruption bug when fuzzing Jansson.

Below you can find the crash log:

# ./prog -detect_leaks=0
INFO: Seed: 665849601
INFO: Loaded 1 modules   (10 inline 8-bit counters): 10 [0x5a3f90, 0x5a3f9a),
INFO: Loaded 1 PC tables (10 PCs): 10 [0x568020,0x5680c0),
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
#2      INITED cov: 3 ft: 4 corp: 1/1b exec/s: 0 rss: 27Mb
#19711  NEW    cov: 6 ft: 7 corp: 2/4b lim: 198 exec/s: 0 rss: 47Mb L: 3/3 MS: 4 InsertByte-InsertByte-ShuffleBytes-ShuffleBytes-
#19777  REDUCE cov: 6 ft: 7 corp: 2/3b lim: 198 exec/s: 0 rss: 47Mb L: 2/2 MS: 1 EraseBytes-
#524288 pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 262144 rss: 610Mb
#1048576        pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 262144 rss: 658Mb
#2097152        pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 262144 rss: 659Mb
#4194304        pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 262144 rss: 659Mb
#8388608        pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 262144 rss: 659Mb
#16777216       pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 254200 rss: 659Mb
#33554432       pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 252288 rss: 659Mb
#67108864       pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 246723 rss: 659Mb
#134217728      pulse  cov: 6 ft: 7 corp: 2/3b lim: 4096 exec/s: 241833 rss: 659Mb
AddressSanitizer:DEADLYSIGNAL
=================================================================
==1898031==ERROR: AddressSanitizer: SEGV on unknown address 0x610000050000 (pc 0x7f229f4f020b bp 0x7ffcf5010a30 sp 0x7ffcf5010938 T0)
==1898031==The signal is caused by a READ memory access.
    #0 0x7f229f4f020b in string_get /root/jansson-2.13/src/load.c:893:7
    #1 0x7f229f4f06f3 in stream_get.part.0 /root/jansson-2.13/src/load.c:158:13
    #2 0x7f229f4f0837 in stream_get /root/jansson-2.13/src/load.c:154:8
    #3 0x7f229f4f0837 in lex_get_save /root/jansson-2.13/src/load.c:233:13
    #4 0x7f229f4f0a32 in lex_scan /root/jansson-2.13/src/load.c:604:17
    #5 0x7f229f4f15da in parse_json.constprop.0 /root/jansson-2.13/src/load.c:868:9
    #6 0x7f229f4f1721 in json_loads /root/jansson-2.13/src/load.c:920:14
    #7 0x54dad3 in LLVMFuzzerTestOneInput /root/fuzzing/jansson/fuzz.c:12:9
    #8 0x4586a1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/fuzzing/jansson/prog+0x4586a1)
    #9 0x457de5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/root/fuzzing/jansson/prog+0x457de5)
    #10 0x45a087 in fuzzer::Fuzzer::MutateAndTestOne() (/root/fuzzing/jansson/prog+0x45a087)
    #11 0x45ad85 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/root/fuzzing/jansson/prog+0x45ad85)
    #12 0x44973e in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/fuzzing/jansson/prog+0x44973e)
    #13 0x472582 in main (/root/fuzzing/jansson/prog+0x472582)                                                                                                                                                  #14 0x7f229f1810b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16
    #15 0x41e4dd in _start (/root/fuzzing/jansson/prog+0x41e4dd)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /root/jansson-2.13/src/load.c:893:7 in string_get
==1898031==ABORTING
MS: 2 InsertRepeatedBytes-CopyPart-; base unit: 97d170e1550eee4afc0af065b78cda302a97674c
0x5b,0x5d,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67
,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x6
7,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x
67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0
x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,0x67,
[]gggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg
artifact_prefix='./'; Test unit written to ./crash-76b9ab07b593b8511d24aea00b925d60735e9d6c
Base64: W11nZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dn
Z2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dnZ2dn

The fuzzer I used was:

#include <jansson.h>
#include <stdio.h>

int LLVMFuzzerTestOneInput(const char *Data, size_t Size) {
        FILE *devNull;

        devNull = fopen("/dev/null", "w");

        json_t *root;
        json_error_t error;

        root = json_loads(Data, 0, &error);

        json_dumpf(root, devNull, 0);
        json_decref(root);

        fclose(devNull);

        return 0;
}

Kind Regards,

Jordy Zomer

carnil commented 3 years ago

This issue appears to have been assigned CVE-2020-36325.

DavidKorczynski commented 3 years ago

This is not a bug. json_loads expects a null-terminated string - the above fuzzer is invalid as it does not ensure proper null-termination. This is how a lot of C functions operate: think strlen - or basically any other function in string.h - you will find OOBs all over libc if you don't provide null-terminated strings.

If this was really assigned a CVE I recon it should be retracted.

akheron commented 3 years ago

Thanks for the analysis @DavidKorczynski

carnil commented 3 years ago

Thanks @DavidKorczynski for the analysis. In this case probably the CVE should be rejected. Note I was not the one requesting int but just sent a request to reject it via https://cveform.mitre.org/

carnil commented 3 years ago

@DavidKorczynski FTR it got not REJECTED but now marked as "disputed", cf. https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-36325

DavidKorczynski commented 3 years ago

Interesting, thanks for the update! I wonder how this plays out - I would think the CVE team must have some means of handling false positives in particular with memory unsafe languages and libraries.

DavidKorczynski commented 3 years ago

For reference "When one party disagrees with another party's assertion that a particular issue in software is a vulnerability, a CVE Record assigned to that issue may be designated as being "DISPUTED". In these cases, the CVE Program is making no determination as to which party is correct." from here https://cve.mitre.org/about/faqs.html#disputed_signify_in_cve_record

However, in this case it seems rejected is the correct classification: "A CVE Record listed as "REJECT" is a CVE Record that is not accepted as a CVE Record. The reason a CVE Record is marked REJECT will most often be stated in the description of the CVE Record. Possible examples include it being a duplicate CVE Record, it being withdrawn by the original requester, it being assigned incorrectly, or some other administrative reason." from the same URL.

DavidKorczynski commented 3 years ago

Anyways - issue is closed and it's no big deal.

vtamara commented 2 years ago

It is well known that functions like strlen that depend on null-terminated strings exclusively are not safe. But what about improving the API of jansson? json_loads could receive a fourth parameter with the maximal length of the parameter data.