Closed Bug 1384801 Opened 2 years ago Closed 2 years ago

[LibFuzzer] SDP: global-buffer-overflow [@base64_decode]

Categories

(Core :: WebRTC: Signaling, defect, P1, critical)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 56+ fixed
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: posidron, Assigned: drno, NeedInfo)

References

(Blocks 1 open bug)

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)

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
Attached file Testcase
Blocks: 792125
Group: core-security → media-core-security
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)
Rank: 10
Flags: needinfo?(drno)
Priority: -- → P1
Attached patch bug1384801.patch (obsolete) — Splinter Review
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.
Attachment #8903835 - Flags: review?(docfaraday)
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?
sec-approval+ for trunk.
We'll want Beta and ESR52 patches made and nominated to land once this is on trunk.
Attachment #8903835 - Flags: sec-approval? → sec-approval+
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-
Attached patch bug1384801.patch (obsolete) — Splinter Review
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 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-
Attached patch bug1384801.patchSplinter Review
Carrying forward sec-approval
Attachment #8904758 - Attachment is obsolete: true
Attachment #8904788 - Flags: review?(docfaraday)
Attachment #8904788 - Flags: review?(docfaraday) → review+
I think this good go now on trunk.

I'll file uplift requests in a minute.
Keywords: checkin-needed
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?
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?
Reminder to self to land the test case after this has been disclosed.
Flags: needinfo?(drno)
https://hg.mozilla.org/mozilla-central/rev/eeeccca3b1e1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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+
Group: media-core-security → core-security-release
Comment on attachment 8904788 [details] [diff] [review]
bug1384801.patch

sec-high webrtc issue, esr52.4+
Attachment #8904788 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Upstream has been notified about this issue. But I haven't heard anything back yet.
Whiteboard: don't disclose until upstream agrees to disclose
Flags: needinfo?(drno)
Flags: needinfo?(drno)
Whiteboard: don't disclose until upstream agrees to disclose → [adv-main56-][adv-esr52.4-] don't disclose until upstream agrees to disclose
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
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.