The following testcase crashes on mozilla-central revision 20180523-d36cd8bdbc5c See attachment. Backtrace: ==34==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000808 (pc 0x7f25d7b97472 bp 0x7ffde57d9f50 sp 0x7ffde57d9e00 T0) ==34==The signal is caused by a READ memory access. ==34==Hint: address points to the zero page. SCARINESS: 10 (null-deref) #0 0x7f25d7b97471 in nr_stun_decode_message /builds/worker/workspace/build/src/media/mtransport/third_party/nICEr/src/stun/stun_codec.c:1418:11 #1 0x7f25d8c0ebe5 in RunStunParserFuzzing(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/media/mtransport/fuzztest/stun_parser_libfuzz.cpp:31:3 #2 0x5893fd in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13 #3 0x588c7b in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:442:3 #4 0x58a16d in fuzzer::Fuzzer::MutateAndTestOne() /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:650:19 #5 0x58ab25 in fuzzer::Fuzzer::Loop(std::vector<std::string, fuzzer::fuzzer_allocator<std::string> > const&) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:773:5 #6 0x582185 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:754:6 #7 0x7f25d7af81e5 in mozilla::FuzzerRunner::Run(int*, char***) /builds/worker/workspace/build/src/tools/fuzzing/interface/harness/FuzzerRunner.cpp:60:10 #8 0x7f25d7a37b33 in XREMain::XRE_mainStartup(bool*) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:3916:35 #9 0x7f25d7a48f45 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4857:12 #10 0x7f25d7a4a6d4 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4964:21 #11 0x4f4ef5 in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:233:22 #12 0x4f4ef5 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:306 #13 0x7f25ef20a1c0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x211c0) #14 0x42476c in _start (/home/worker/firefox/firefox+0x42476c) DEDUP_TOKEN: nr_stun_decode_message AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/media/mtransport/third_party/nICEr/src/stun/stun_codec.c:1418:11 in nr_stun_decode_message Command: ./firefox/firefox -print_pcs=1 -handle_segv=0 -handle_bus=0 -handle_abrt=0 ./corpora/ -handle_ill=0 -handle_fpe=0 ==34==ABORTING
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8980643 [details] Bug 1464069 - Check for null message in nr_stun_decode_message; https://reviewboard.mozilla.org/r/246810/#review253026 LGTM
Attachment #8980643 - Flags: review?(drno) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/8ea04450a354 Check for null message in nr_stun_decode_message; r=drno
I'm trying to understand how this happens. Part of the API contract to this function is that you pass it a valid pointer. So, one of two things is happening here: 1. You're directly violating the contract, in which case this isn't a valid test case. 2. Some other nICEr function is calling it with a null pointer, in which case that function has a bug. Based on the call stack, the problem looks like 1, in which case, this change is unnecessary.
(In reply to Eric Rescorla (:ekr) from comment #5) > I'm trying to understand how this happens. Part of the API contract to this > function is that you pass it a valid pointer. So, one of two things is > happening here: > > 1. You're directly violating the contract, in which case this isn't a valid > test case. > 2. Some other nICEr function is calling it with a null pointer, in which > case that function has a bug. > > Based on the call stack, the problem looks like 1, in which case, this > change is unnecessary. It was found by calling  in the STUN libfuzzer module and by feeding it these dumped STUN corpora .  https://searchfox.org/mozilla-central/source/media/mtransport/fuzztest/stun_parser_libfuzz.cpp#31  https://github.com/MozillaSecurity/fuzzdata/tree/master/samples/stun
This test case doesn't seem quite right. Here's nr_stun_message_create2() https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_msg.c#53. The prototype is: int nr_stun_message_create2(nr_stun_message **msg, UCHAR *buffer, size_t length); The only real ways it can fail are: 1. Out of memory 2. You pass it a too large message (|length| > NR_STUN_MAX_MESSAGE_SIZE) I'm assuming you are doing the second. In either case, it leaves |*msg| unset (you have it initialized to NULL) and returns a nonzero error code. In order to use this API correctly, you need to check the error code prior to calling nr_stun_decode_message(). Here is an example from the nICEr code. https://searchfox.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c#565 In general, (nearly) every nICEr function returns an error value, and you can't use its outputs if that error is nonzero.
Dan, looks like we fixed the wrong end here. Can you fix the fuzzing client please?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry about this, I saw what looked like a quick fix while doing triage. In the future, I'll leave these alone unless I'm familiar with the code involved.
Comment on attachment 8981563 [details] [diff] [review] bug-1464069-fix.patch Review of attachment 8981563 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8981563 - Flags: review?(drno) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/66c01f311f4a Check error code in stun_parser_libfuzz.cpp; r=drno
Status: REOPENED → RESOLVED
Closed: Last year → Last year
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.