Json: Integer Overflow (OSS-Fuzz 12506)

[nlohmann]

Detailed report: https://oss-fuzz.com/testcase?key=5101274830209024

Project: json
Fuzzer: libFuzzer_json_parse_afl_fuzzer
Fuzz target binary: parse_afl_fuzzer
Job Type: libfuzzer_ubsan_json
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  void nlohmann::detail::serializer<nlohmann::basic_json<std::__1::map, std::__1::
  nlohmann::detail::serializer<nlohmann::basic_json<std::__1::map, std::__1::vecto
  nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<cha

Sanitizer: undefined (UBSAN)

Regressed: https://oss-fuzz.com/revisions?job=libfuzzer_ubsan_json&range=201901130334:201901140333

Reproducer Testcase: https://oss-fuzz.com/download?testcase_id=5101274830209024

Issue filed automatically.

See https://github.com/google/oss-fuzz/blob/master/docs/reproducing.md for instructions to reproduce this bug locally.

This bug is subject to a 90 day disclosure deadline. If 90 days elapse
without an upstream patch, then the bug report will automatically
become visible to the public.

Details:

Running command: /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_json_3dc7f07cae4ab217c21b70d40f93a3acccc6b431/revisions/parse_afl_fuzzer -timeout=10 -rss_limit_mb=2048 -runs=100 /fuzz-1
--
  | INFO: Seed: 4228528690
  | INFO: Loaded 1 modules   (3373 inline 8-bit counters): 3373 [0x78b9c8, 0x78c6f5),
  | INFO: Loaded 1 PC tables (3373 PCs): 3373 [0x52e540,0x53b810),
  | /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_json_3dc7f07cae4ab217c21b70d40f93a3acccc6b431/revisions/parse_afl_fuzzer: Running 1 inputs 100 time(s) each.
  | Running: /fuzz-1
  | ../single_include/nlohmann/json.hpp:11503:58: runtime error: signed integer overflow: 0 - -9223372036854775808 cannot be represented in type 'long'
  | #0 0x46a858 in void nlohmann::detail::serializer<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer> >::dump_integer<long, 0>(long) json/single_include/nlohmann/json.hpp:11503:58
  | #1 0x4659d4 in nlohmann::detail::serializer<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer> >::dump(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer> const&, bool, bool, unsigned int, unsigned int) json/single_include/nlohmann/json.hpp:11112:21
  | #2 0x463eec in nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer>::dump(int, char, bool, nlohmann::detail::error_handler_t) const json/single_include/nlohmann/json.hpp:14443:15
  | #3 0x463a6a in LLVMFuzzerTestOneInput json/test/src/fuzzer-parse_json.cpp:41:33
  | #4 0x4417d8 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:533:15
  | #5 0x432192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/libfuzzer/FuzzerDriver.cpp:285:6
  | #6 0x435c1b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:714:9
  | #7 0x431f18 in main /src/libfuzzer/FuzzerMain.cpp:20:10
  | #8 0x7f345a6ba82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/libc-start.c:291
  | #9 0x405da8 in _start

Test case:

[-9223372036854775808]

Note:

This may be related to #1411.

[nlohmann]

The same input also triggered this issue:

Detailed report: https://oss-fuzz.com/testcase?key=5101274830209024

Project: json
Fuzzer: libFuzzer_json_parse_afl_fuzzer
Fuzz target binary: parse_afl_fuzzer
Job Type: libfuzzer_ubsan_json
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  void nlohmann::detail::serializer<nlohmann::basic_json<std::__1::map, std::__1::
  nlohmann::detail::serializer<nlohmann::basic_json<std::__1::map, std::__1::vecto
  nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<cha

Sanitizer: undefined (UBSAN)

Regressed: https://oss-fuzz.com/revisions?job=libfuzzer_ubsan_json&range=201901130334:201901140333

Reproducer Testcase: https://oss-fuzz.com/download?testcase_id=5101274830209024

Issue filed automatically.

See https://github.com/google/oss-fuzz/blob/master/docs/reproducing.md for instructions to reproduce this bug locally.

Details:

Running command: /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_json_3dc7f07cae4ab217c21b70d40f93a3acccc6b431/revisions/parse_afl_fuzzer -timeout=10 -rss_limit_mb=2048 -runs=100 /fuzz-1
--
  | INFO: Seed: 4228528690
  | INFO: Loaded 1 modules   (3373 inline 8-bit counters): 3373 [0x78b9c8, 0x78c6f5),
  | INFO: Loaded 1 PC tables (3373 PCs): 3373 [0x52e540,0x53b810),
  | /mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_json_3dc7f07cae4ab217c21b70d40f93a3acccc6b431/revisions/parse_afl_fuzzer: Running 1 inputs 100 time(s) each.
  | Running: /fuzz-1
  | ../single_include/nlohmann/json.hpp:11503:58: runtime error: signed integer overflow: 0 - -9223372036854775808 cannot be represented in type 'long'
  | #0 0x46a858 in void nlohmann::detail::serializer<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer> >::dump_integer<long, 0>(long) json/single_include/nlohmann/json.hpp:11503:58
  | #1 0x4659d4 in nlohmann::detail::serializer<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer> >::dump(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer> const&, bool, bool, unsigned int, unsigned int) json/single_include/nlohmann/json.hpp:11112:21
  | #2 0x463eec in nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long, unsigned long, double, std::__1::allocator, nlohmann::adl_serializer>::dump(int, char, bool, nlohmann::detail::error_handler_t) const json/single_include/nlohmann/json.hpp:14443:15
  | #3 0x463a6a in LLVMFuzzerTestOneInput json/test/src/fuzzer-parse_json.cpp:41:33
  | #4 0x4417d8 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/libfuzzer/FuzzerLoop.cpp:533:15
  | #5 0x432192 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/libfuzzer/FuzzerDriver.cpp:285:6
  | #6 0x435c1b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/libfuzzer/FuzzerDriver.cpp:714:9
  | #7 0x431f18 in main /src/libfuzzer/FuzzerMain.cpp:20:10
  | #8 0x7f345a6ba82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/libc-start.c:291
  | #9 0x405da8 in _start

[nlohmann]

I can reproduce the issue. Line

abs_value = static_cast<number_unsigned_t>(0 - x);

Yields the message:

Signed integer overflow: 0 - -9223372036854775808 cannot be represented in type 'long long'

[nlohmann]

@nickaein Any ideas on this?

[scinart]

I think that when x is std::numeric_limits<long long>::min() (a.k.a -9223372036854775808), the behavior of 0-x is undefined. (because 9223372036854775808 cannot be represented by long long), see https://stackoverflow.com/a/19205206/1889040

Fix is easy, change

abs_value = static_cast<number_unsigned_t>(0 - x);

to

abs_value = static_cast<number_unsigned_t>(~x)+1;

should do.

[smonkewitz]

Doesn't that assume 2's complement signed integers?

abs_value = static_cast<number_unsigned_t>(-1 - x) + 1;

is a bit more portable, and works since this code only runs when x < 0. That said, I don't know of any CPUs using 1's complement for signed integers, so I'm not sure it matters.

[nlohmann]

Thanks @smonkewitz !

[nlohmann]

(Also thanks @scinart !)

[nlohmann]

Both issues have been reported and confirmed fixed by OSS-Fuzz.

[dcleblanc]

Hi - this:
abs_value = static_cast(-1 - x) + 1;
uses non-standard, undefined behavior, and some compilers will toss you out. The SafeInt class solves this problem for you, might want to investigate using it. SafeInt can be found here:
https://github.com/dcleblanc/SafeInt

Your fix that assumes 2's complement is the correct fix, since unsigned overflow is defined by the standard, and the compiler isn't going to throw it out.

[jaredgrubb]

Two's complement is the only game in town (P1236 was voted in and even C is adopting similar assumptions).

[dcleblanc]

Thanks for sharing that. I wasn’t aware of the standard changes. I’m disappointed that signed overflow remains undefined, which allows compilers to continue to remove otherwise valid security checks. BTW, if you want to be really sure that you have two’s complement, you can do this:

static_assert( -1 == static_cast(0xffffffff), "Two's complement signed numbers are required" );

From: Jared Grubb [e-mail]
Sent: Monday, April 8, 2019 5:34 PM
To: nlohmann/json [e-mail]
Cc: David LeBlanc [e-mail]; Comment [e-mail]
Subject: Re: [nlohmann/json] Integer Overflow (OSS-Fuzz 12506) (#1447)

Two's complement is the only game in town (P1236https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwg21.link%2FP1236&data=02%7C01%7Cdleblanc%40exchange.microsoft.com%7Cd432f11a70854e1cd58908d6bc8301d2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636903668187763668&sdata=2LccmRoNFhm%2B%2BEQes%2BAwdmP8OROuQZ33uPLFrMi%2Fnzs%3D&reserved=0 was voted in and even C is adopting similar assumptionshttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftwitter.com%2Fjfbastien%2Fstatus%2F989242576598327296&data=02%7C01%7Cdleblanc%40exchange.microsoft.com%7Cd432f11a70854e1cd58908d6bc8301d2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636903668187763668&sdata=9yN3VVhifIWirQq8Xqp2mn%2FZEr3qJC%2Fzf%2BHUdxO1TwU%3D&reserved=0).

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnlohmann%2Fjson%2Fissues%2F1447%23issuecomment-481056713&data=02%7C01%7Cdleblanc%40exchange.microsoft.com%7Cd432f11a70854e1cd58908d6bc8301d2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636903668187773659&sdata=oSZwV%2Bq%2FvgfxlVo1Kc%2F0LupzYHdcSkf7TMDRh6AAes0%3D&reserved=0, or mute the threadhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAe_sL2CiJS3NDWTOYWqSB9pKUGXJZQjLks5ve9_hgaJpZM4aJOHZ&data=02%7C01%7Cdleblanc%40exchange.microsoft.com%7Cd432f11a70854e1cd58908d6bc8301d2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636903668187773659&sdata=tqSNAQ59fUADc963z%2F3P4XMrjAH4gAkMlPNdfhHcYwM%3D&reserved=0.

[jaredgrubb]

Signed overflow is a tough one, as there are demonstrable benefits to it being UB. However, as you say, providing defined behavior has good benefits too, although there are many forms of that (wrap, saturate, throw). I personally hope we get such types in C++ one day (which are not named "int", eg), but we shall see what the Committee does!

[dcleblanc]

At least for addition, it is practically defined because both signed and unsigned addition use the same assembly instruction. At any rate, happy to have that conversation offline, not relevant to this code, AFAIK.

From: Jared Grubb [e-mail]
Sent: Monday, April 8, 2019 6:07 PM
To: nlohmann/json [e-mail]
Cc: David LeBlanc [e-mail]; Comment [e-mail]
Subject: Re: [nlohmann/json] Integer Overflow (OSS-Fuzz 12506) (#1447)

Signed overflow is a tough one, as there are demonstrable benefits to it being UB. However, as you say, providing defined behavior has good benefits too, although there are many forms of that (wrap, saturate, throw). I personally hope we get such types in C++ one day (which are not named "int", eg), but we shall see what the Committee does!

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnlohmann%2Fjson%2Fissues%2F1447%23issuecomment-481062337&data=02%7C01%7Cdleblanc%40exchange.microsoft.com%7C8094489204694756b65208d6bc87a1ff%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636903688061734930&sdata=n1%2FTMuUvjQZaennXBjWxEz9r9HPUQJu%2FxuMYUBga9yc%3D&reserved=0, or mute the threadhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAe_sL0WLgayAGn3jKIB5m3WiBP9aw52kks5ve-ekgaJpZM4aJOHZ&data=02%7C01%7Cdleblanc%40exchange.microsoft.com%7C8094489204694756b65208d6bc87a1ff%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636903688061734930&sdata=PowVVe%2FjRODqM2JC94pycbNZQRlOZmwnY2p4HiuUOkQ%3D&reserved=0.