Closed
Bug 1384801
Opened 7 years ago
Closed 7 years ago
[LibFuzzer] SDP: global-buffer-overflow [@base64_decode]
Categories
(Core :: WebRTC: Signaling, defect, P1)
Tracking
()
People
(Reporter: posidron, Assigned: drno)
References
Details
(4 keywords, Whiteboard: [adv-main56-][adv-esr52.4-][post-critsmash-triage] don't disclose until upstream agrees to disclose)
Attachments
(2 files, 2 obsolete files)
1.14 KB,
application/octet-stream
|
Details | |
3.13 KB,
patch
|
bwc
:
review+
lizzard
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 20170727-f1693d664f8e See attachment. Backtrace: ==466==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7fcecbadb71f at pc 0x7fcec431e8c1 bp 0x7ffd19795750 sp 0x7ffd19795748 READ of size 1 at 0x7fcecbadb71f thread T0 SCARINESS: 12 (1-byte-read-global-buffer-overflow) #0 0x7fcec431e8c0 in base64_decode media/webrtc/signaling/src/sdp/sipcc/sdp_base64.c:299:7 #1 0x7fcec43083cf in sdp_parse_sdescriptions_key_param media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:4128:22 #2 0x7fcec43095a1 in sdp_parse_attr_srtp media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:4359:10 #3 0x7fcec42e1947 in sdp_parse_attribute media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:185:14 #4 0x7fcec4320ab2 in sdp_parse media/webrtc/signaling/src/sdp/sipcc/sdp_main.c:1138:18 #5 0x7fcec460fd26 in mozilla::SipccSdpParser::Parse(std::string const&) media/webrtc/signaling/src/sdp/SipccSdpParser.cpp:68:25 #6 0x7fcec53cdacc in RunSdpParserFuzzing(unsigned char const*, unsigned long) media/webrtc/signaling/fuzztest/sdp_parser_libfuzz.cpp:28:20 #7 0x55dd17 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) tools/fuzzing/libfuzzer/FuzzerLoop.cpp:458:13 #8 0x55df33 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long) tools/fuzzing/libfuzzer/FuzzerLoop.cpp:397:3 #9 0x55e7ea in fuzzer::Fuzzer::MutateAndTestOne() tools/fuzzing/libfuzzer/FuzzerLoop.cpp:596:30 #10 0x55e9a7 in fuzzer::Fuzzer::Loop() tools/fuzzing/libfuzzer/FuzzerLoop.cpp:624:5 #11 0x5577d4 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) tools/fuzzing/libfuzzer/FuzzerDriver.cpp:744:6 #12 0x7fcec42847a3 in mozilla::LibFuzzerRunner::Run(int*, char***) tools/fuzzing/libfuzzer/harness/LibFuzzerRunner.cpp:46:10 #13 0x7fcec41cbd9f in XREMain::XRE_mainStartup(bool*) toolkit/xre/nsAppRunner.cpp:3826:38 #14 0x7fcec41d9d65 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4745:12 #15 0x7fcec41db532 in XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4855:21 #16 0x4f9780 in do_main browser/app/nsBrowserApp.cpp:236:22 #17 0x4f9780 in main browser/app/nsBrowserApp.cpp:309 #18 0x7fceda3d63f0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x203f0) #19 0x42237c in _start (/home/worker/firefox/firefox+0x42237c) DEDUP_TOKEN: base64_decode 0x7fcecbadb71f is located 31 bytes to the right of global variable 'raw_to_base64_table' defined in '/home/worker/workspace/build/src/media/webrtc/signaling/src/sdp/sipcc/sdp_base64.c:58:15' (0x7fcecbadb6c0) of size 64 SUMMARY: AddressSanitizer: global-buffer-overflow media/webrtc/signaling/src/sdp/sipcc/sdp_base64.c:299:7 in base64_decode Shadow bytes around the buggy address: 0x0ffa59753690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0ffa597536a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0ffa597536b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 0x0ffa597536c0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00 0x0ffa597536d0: 00 00 00 00 f9 f9 f9 f9 00 00 00 00 00 00 00 00 =>0x0ffa597536e0: f9 f9 f9[f9]00 00 00 00 00 00 00 00 00 00 00 00 0x0ffa597536f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0ffa59753700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0ffa59753710: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0ffa59753720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0ffa59753730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Command: ./firefox -print_pcs=1 -dict=../tokens.dict ../corpora/ ==466==ABORTING
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Blocks: fuzzing-webrtc
Updated•7 years ago
|
status-firefox54:
--- → ?
status-firefox55:
--- → ?
status-firefox56:
--- → affected
status-firefox-esr52:
--- → ?
Updated•7 years ago
|
Group: core-security → media-core-security
Assignee | ||
Comment 2•7 years ago
|
||
Thanks Christoph, I missed this earlier because it came in during my vacation. I'll try to have a look at it this week.
Assignee: nobody → drno
Flags: needinfo?(drno)
Assignee | ||
Updated•7 years ago
|
Rank: 10
Flags: needinfo?(drno)
Priority: -- → P1
Assignee | ||
Comment 3•7 years ago
|
||
This fixes that the parser moves the index one character further and then access the base64_to_raw_table() without checking if that new character actually is a legal base64 character.
Assignee | ||
Updated•7 years ago
|
Attachment #8903835 -
Flags: review?(docfaraday)
Assignee | ||
Updated•7 years ago
|
status-firefox57:
--- → affected
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8903835 [details] [diff] [review] bug1384801.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Not very hard. One only needs to track backwards which tokens in the SDP get this code invoked. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No tests included. I tried to make the commit message not too obvious. Which older supported branches are affected by this flaw? All versions of Firefox since release 18. If not all supported branches, which bug introduced the flaw? The initial import of the SIPcc code in bug 792188. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Patch applies cleanly to all branches. How likely is this patch to cause regressions; how much testing does it need? Highly unlikely as any normal WebRTC call should never execute this code at all. Therefore not really much or any testing needed.
Attachment #8903835 -
Flags: sec-approval?
Comment 5•7 years ago
|
||
sec-approval+ for trunk. We'll want Beta and ESR52 patches made and nominated to land once this is on trunk.
Updated•7 years ago
|
Attachment #8903835 -
Flags: sec-approval? → sec-approval+
Comment 6•7 years ago
|
||
Comment on attachment 8903835 [details] [diff] [review] bug1384801.patch Review of attachment 8903835 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sdp/sipcc/sdp_base64.c @@ +300,5 @@ > + > + if ((cindex & 0x80) || /* only have 128 values, MSB must not be set! */ > + ((val = base64_to_raw_table[cindex]) == INVALID_CHAR) || > + /* Invalid base64 character */ > + (base64_to_raw_table[cindex] != PADDING)) { Maybe just do >= 128 instead of a bitmask? Also, == INVALID_CHAR implies != PADDING, right? Lastly, I see other places where we index into the array without such a check. We should probably wrap this in a function.
Attachment #8903835 -
Flags: review?(docfaraday) → review-
Assignee | ||
Comment 7•7 years ago
|
||
Carrying forward sec-approval. Note: I made the function only specific for padding, because in line 284 http://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/sipcc/sdp_base64.c#284 the code relies on loading the value into |val|.
Attachment #8903835 -
Attachment is obsolete: true
Attachment #8904758 -
Flags: review?(docfaraday)
Comment 8•7 years ago
|
||
Comment on attachment 8904758 [details] [diff] [review] bug1384801.patch Review of attachment 8904758 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sdp/sipcc/sdp_base64.c @@ +292,1 @@ > ((val = base64_to_raw_table[cindex]) == INVALID_CHAR)) { It would be nice if we could wrap this access also, but I guess we would need to change the function signature. Maybe have it return INVALID_CHAR if the index is out of range? @@ +349,5 @@ > * has occurred > */ > if ((val & 0x0F) || > (i+1>=src_bytes) || > + (!base64_decode_index_is_padding(cindex))) { cindex isn't src[i+1] @@ +372,5 @@ > * has occurred > */ > if ((val & 0x03) || > (i+1>=src_bytes) || > + (!base64_decode_index_is_padding(cindex))) { Same here.
Attachment #8904758 -
Flags: review?(docfaraday) → review-
Assignee | ||
Comment 9•7 years ago
|
||
Carrying forward sec-approval
Attachment #8904758 -
Attachment is obsolete: true
Attachment #8904788 -
Flags: review?(docfaraday)
Updated•7 years ago
|
Attachment #8904788 -
Flags: review?(docfaraday) → review+
Assignee | ||
Comment 10•7 years ago
|
||
I think this good go now on trunk. I'll file uplift requests in a minute.
Keywords: checkin-needed
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeeccca3b1e1bc521e01062a1aa0ae32eb4f5cb6
Keywords: checkin-needed
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8904788 [details] [diff] [review] bug1384801.patch Approval Request Comment [Feature/Bug causing the regression]: Bug 792188 imported this code into Firefox 18 [User impact if declined]: Potential crash [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: I verified that with the fix the ASAN fuzzing build no longer reports an issue. As this code is not actively used by WebRTC there is not much else to verify here. [Needs manual test from QE? If yes, steps to reproduce]: No as there isn't really much to test here. [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: No. [Why is the change risky/not risky?]: Because after parsing this the WebRTC call logic never looks at this data. So it's going to affect any WebRTC call. [String changes made/needed]: N/A
Attachment #8904788 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8904788 [details] [diff] [review] bug1384801.patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec high User impact if declined: Potential crashes. Fix Landed on Version: 57 Risk to taking this patch (and alternatives if risky): Almost no risk as what the parser parses here never gets used in a WebRTC call. String or UUID changes made by this patch: N/A See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8904788 -
Flags: approval-mozilla-esr52?
Assignee | ||
Comment 14•7 years ago
|
||
Reminder to self to land the test case after this has been disclosed.
Flags: needinfo?(drno)
Comment 15•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eeeccca3b1e1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 16•7 years ago
|
||
Comment on attachment 8904788 [details] [diff] [review] bug1384801.patch sec-high issue, let's uplift this fix for beta 10.
Attachment #8904788 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Group: media-core-security → core-security-release
Comment 17•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5d3cb4a84b47
Comment 18•7 years ago
|
||
Comment on attachment 8904788 [details] [diff] [review] bug1384801.patch sec-high webrtc issue, esr52.4+
Attachment #8904788 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 19•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/9556e792f905
Assignee | ||
Comment 20•7 years ago
|
||
Upstream has been notified about this issue. But I haven't heard anything back yet.
Whiteboard: don't disclose until upstream agrees to disclose
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(drno)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(drno)
Updated•7 years ago
|
Whiteboard: don't disclose until upstream agrees to disclose → [adv-main56-][adv-esr52.4-] don't disclose until upstream agrees to disclose
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main56-][adv-esr52.4-] don't disclose until upstream agrees to disclose → [adv-main56-][adv-esr52.4-][post-critsmash-triage] don't disclose until upstream agrees to disclose
Updated•6 years ago
|
Group: core-security-release
Assignee | ||
Updated•4 years ago
|
Flags: needinfo?(drno)
You need to log in
before you can comment on or make changes to this bug.
Description
•