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

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: Testcase

Comment 2

[Nils Ohlmeier [:drno]]

Thanks Christoph, I missed this earlier because it came in during my vacation. I'll try to have a look at it this week.

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: bug1384801.patch

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.

Comment 4

[Nils Ohlmeier [:drno]]

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.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.
We'll want Beta and ESR52 patches made and nominated to land once this is on trunk.

Comment 6

[Byron Campen [:bwc]]

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.

Comment 7

[Nils Ohlmeier [:drno]]

Attachment: bug1384801.patch

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|.

Comment 8

[Byron Campen [:bwc]]

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.

Comment 9

[Nils Ohlmeier [:drno]]

Attachment: bug1384801.patch

Carrying forward sec-approval

Comment 10

[Nils Ohlmeier [:drno]]

I think this good go now on trunk.

I'll file uplift requests in a minute.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeeccca3b1e1bc521e01062a1aa0ae32eb4f5cb6

Comment 12

[Nils Ohlmeier [:drno]]

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

Comment 13

[Nils Ohlmeier [:drno]]

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.

Comment 14

[Nils Ohlmeier [:drno]]

Reminder to self to land the test case after this has been disclosed.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eeeccca3b1e1

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8904788 [details] [diff] [review]
bug1384801.patch

sec-high issue, let's uplift this fix for beta 10.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d3cb4a84b47

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8904788 [details] [diff] [review]
bug1384801.patch

sec-high webrtc issue, esr52.4+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/9556e792f905

Comment 20

[Nils Ohlmeier [:drno]]

Upstream has been notified about this issue. But I haven't heard anything back yet.