1464069 - LibFuzzer: STUN crash [@nr_stun_decode_message]

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Christoph Diehl [:posidron]]

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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: Testcase

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1464069 - Check for null message in nr_stun_decode_message;

Review commit: https://reviewboard.mozilla.org/r/246810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246810/

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8980643 [details]
Bug 1464069 - Check for null message in nr_stun_decode_message;

https://reviewboard.mozilla.org/r/246810/#review253026

LGTM

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ea04450a354
Check for null message in nr_stun_decode_message; r=drno

Comment 5

[Eric Rescorla (:ekr)]

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.

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8ea04450a354

Comment 7

[Christoph Diehl [:posidron]]

(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 [1] in the STUN libfuzzer module and by feeding it these dumped STUN corpora [2].

[1] https://searchfox.org/mozilla-central/source/media/mtransport/fuzztest/stun_parser_libfuzz.cpp#31
[2] https://github.com/MozillaSecurity/fuzzdata/tree/master/samples/stun

Comment 8

[Eric Rescorla (:ekr)]

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.

Comment 9

[Nils Ohlmeier [:drno]]

Dan, looks like we fixed the wrong end here. Can you fix the fuzzing client please?

Comment 10

[Dan Minor [:dminor]]

Sure thing.

Comment 11

[Dan Minor [:dminor]]

Attachment: bug-1464069-fix.patch

Comment 12

[Dan Minor [:dminor]]

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 13

[Nils Ohlmeier [:drno]]

Comment on attachment 8981563 [details] [diff] [review]
bug-1464069-fix.patch

Review of attachment 8981563 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c01f311f4a
Check error code in stun_parser_libfuzz.cpp; r=drno

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/66c01f311f4a