Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Integer Overflow (OSS-Fuzz 12506) #1447

Closed
nlohmann opened this issue Jan 19, 2019 · 13 comments
Closed

Integer Overflow (OSS-Fuzz 12506) #1447

nlohmann opened this issue Jan 19, 2019 · 13 comments
Assignees
@nlohmann
Labels
kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone
Release 3.6.0

Comments

@nlohmann
Copy link
Owner 

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Owner Author

@nickaein Any ideas on this?

@scinart
Copy link
Contributor

scinart commented Jan 20, 2019

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
Copy link

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
Copy link
Owner Author

Thanks @smonkewitz !

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Jan 20, 2019
@nlohmann nlohmann self-assigned this Jan 20, 2019
@nlohmann nlohmann added this to the Release 3.5.1 milestone Jan 20, 2019
@nlohmann
Copy link
Owner Author

(Also thanks @scinart !)

@nlohmann
Copy link
Owner Author

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

@dcleblanc
Copy link

Hi - this:
abs_value = static_cast<number_unsigned_t>(-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
Copy link
Contributor

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

@dcleblanc
Copy link

dcleblanc commented Apr 9, 2019 via email

@jaredgrubb
Copy link
Contributor

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
Copy link

dcleblanc commented Apr 9, 2019 via email

dnsmichi pushed a commit to Icinga/icinga2 that referenced this issue Dec 13, 2019
JSON: Update to nlohmann/json 3.7.3
5725ba4 
This includes the following fixes:

nlohmann/json#1436

> For a deeply-nested JSON object, the recursive implementation of json_value::destroy function causes stack overflow.

nlohmann/json#1708
nlohmann/json#1722

Stack size

nlohmann/json#1693 (comment)

Integer Overflow

nlohmann/json#1447

UTF8, json dump out of bounds

nlohmann/json#1445

Possibly influences #7532
This was referenced Dec 13, 2019
Merged
Closed
Al2Klimov pushed a commit to Icinga/icinga2 that referenced this issue Dec 16, 2019
@Al2Klimov
JSON: Update to nlohmann/json 3.7.3
be3eaa5 
This includes the following fixes:

nlohmann/json#1436

> For a deeply-nested JSON object, the recursive implementation of json_value::destroy function causes stack overflow.

nlohmann/json#1708
nlohmann/json#1722

Stack size

nlohmann/json#1693 (comment)

Integer Overflow

nlohmann/json#1447

UTF8, json dump out of bounds

nlohmann/json#1445

Possibly influences #7532
N-o-X pushed a commit to Icinga/icinga2 that referenced this issue May 8, 2020
@N-o-X
JSON: Update to nlohmann/json 3.7.3
f109c58 
This includes the following fixes:

nlohmann/json#1436

> For a deeply-nested JSON object, the recursive implementation of json_value::destroy function causes stack overflow.

nlohmann/json#1708
nlohmann/json#1722

Stack size

nlohmann/json#1693 (comment)

Integer Overflow

nlohmann/json#1447

UTF8, json dump out of bounds

nlohmann/json#1445

Possibly influences #7532
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nlohmann nlohmann

Labels
kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Milestone
Release 3.6.0
Development

No branches or pull requests
5 participants
@nlohmann @smonkewitz @jaredgrubb @scinart @dcleblanc