[Libfuzzer] Heap-buffer-overflow in sdp_parse_attr_fmtp

RESOLVED FIXED in Firefox -esr52
(NeedInfo from)

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: drno, Assigned: drno, NeedInfo)

Tracking

(Blocks 1 bug, {csectype-bounds, sec-high})

Trunk
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox-esr5255+ fixed, firefox54 wontfix, firefox55+ fixed, firefox56+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main55-][adv-esr52.3-] don't disclose until upstream agrees to disclose)

Attachments

(3 attachments, 1 obsolete attachment)

Running LibFuzzer tests...
[21289] WARNING: NS_ENSURE_TRUE(greBinD) failed: file /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/harness/LibFuzzerTestHarness.h, line 227
Warning: MOZILLA_FIVE_HOME not set.
[21289] WARNING: NS_ENSURE_TRUE(greD) failed: file /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/harness/LibFuzzerTestHarness.h, line 220
INFO: Seed: 1060379189
INFO: Loaded 12 modules (2453320 guards): [0x698e00, 0x69c370), [0x7fc8fb7557a0, 0x7fc8fb75c6c8), [0x7fc8fedec7c0, 0x7fc8fedecd6c), [0x7fc8feddc660, 0x7fc8feddc87c), [0x7fc8fb5788e0, 0x7fc8fb57a5d4), [0x7fc8fb4ca9a0, 0x7fc8fb4ce368), [0x7fc8fb3c9e20, 0x7fc8fb3dbc08), [0x7fc8fb0ebec0, 0x7fc8fb0f108c), [0x7fc8fb009280, 0x7fc8fb026284), [0x7fc8fa9bdee0, 0x7fc8fa9c8d04), [0x7fc8feda2140, 0x7fc8feda2178), [0x7fc8f10b5300, 0x7fc8f19c1ef0), 
./firefox: Running 1 inputs 1 time(s) each.
Running: crash-1f1f843179967d9ca456ac9113972e3db3619677
=================================================================
==21289==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000054dff at pc 0x7fc8e49e9c45 bp 0x7ffcbaf729d0 sp 0x7ffcbaf729c8
READ of size 1 at 0x602000054dff thread T0
    #0 0x7fc8e49e9c44 in next_token /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c:78:13
    #1 0x7fc8e49e998a in sdp_getnextstrtok /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c:336:13
    #2 0x7fc8e49692b1 in sdp_parse_attr_fmtp /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:657:18
    #3 0x7fc8e49655da in sdp_parse_attribute /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:185:14
    #4 0x7fc8e49d0f40 in sdp_parse /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_main.c:1138:18
    #5 0x7fc8e4cd0a68 in mozilla::SipccSdpParser::Parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/SipccSdpParser.cpp:68:25
    #6 0x7fc8e5905b5f in RunSdpParserFuzzing(unsigned char const*, unsigned long) /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/fuzztest/sdp_parser_libfuzz.cpp:32:20
    #7 0x5f8db6 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:458:13
    #8 0x5f9056 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:397:3
    #9 0x5df0e7 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:268:6
    #10 0x5e19ec in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:683:9
    #11 0x7fc8e489286d in mozilla::LibFuzzerRunner::Run(int*, char***) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/harness/LibFuzzerRunner.cpp:46:10
    #12 0x7fc8e46c80cc in XREMain::XRE_mainStartup(bool*) /home/nohlmeier/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:3805:38
    #13 0x7fc8e46da1d0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/nohlmeier/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4734:12
    #14 0x7fc8e46dba35 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/nohlmeier/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4844:21
    #15 0x7fc8e471947d in mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/nohlmeier/src/mozilla-central/toolkit/xre/Bootstrap.cpp:45:12
    #16 0x55500a in do_main(int, char**, char**) /home/nohlmeier/src/mozilla-central/browser/app/nsBrowserApp.cpp:236:22
    #17 0x553ffa in main /home/nohlmeier/src/mozilla-central/browser/app/nsBrowserApp.cpp:309:16
    #18 0x7fc8fdad382f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291
    #19 0x45d6a8 in _start (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x45d6a8)

0x602000054dff is located 0 bytes to the right of 15-byte region [0x602000054df0,0x602000054dff)
allocated by thread T0 here:
    #0 0x515d38 in __interceptor_malloc (/home/nohlmeier/src/mozilla-central/objdir-ff-asan/dist/bin/firefox+0x515d38)
    #1 0x558025 in moz_xmalloc /home/nohlmeier/src/mozilla-central/memory/mozalloc/mozalloc.cpp:83:17
    #2 0x7fc8e495a015 in cpr_strdup /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/cpr_string.c:272:11
    #3 0x7fc8e49691af in sdp_parse_attr_fmtp /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:649:16
    #4 0x7fc8e49655da in sdp_parse_attribute /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:185:14
    #5 0x7fc8e49d0f40 in sdp_parse /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_main.c:1138:18
    #6 0x7fc8e4cd0a68 in mozilla::SipccSdpParser::Parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/SipccSdpParser.cpp:68:25
    #7 0x7fc8e5905b5f in RunSdpParserFuzzing(unsigned char const*, unsigned long) /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/fuzztest/sdp_parser_libfuzz.cpp:32:20
    #8 0x5f8db6 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:458:13
    #9 0x5f9056 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/FuzzerLoop.cpp:397:3
    #10 0x5df0e7 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:268:6
    #11 0x5e19ec in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/FuzzerDriver.cpp:683:9
    #12 0x7fc8e489286d in mozilla::LibFuzzerRunner::Run(int*, char***) /home/nohlmeier/src/mozilla-central/tools/fuzzing/libfuzzer/harness/LibFuzzerRunner.cpp:46:10
    #13 0x7fc8e46c80cc in XREMain::XRE_mainStartup(bool*) /home/nohlmeier/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:3805:38
    #14 0x7fc8e46da1d0 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/nohlmeier/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4734:12
    #15 0x7fc8e46dba35 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/nohlmeier/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4844:21
    #16 0x7fc8e471947d in mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/nohlmeier/src/mozilla-central/toolkit/xre/Bootstrap.cpp:45:12
    #17 0x55500a in do_main(int, char**, char**) /home/nohlmeier/src/mozilla-central/browser/app/nsBrowserApp.cpp:236:22
    #18 0x553ffa in main /home/nohlmeier/src/mozilla-central/browser/app/nsBrowserApp.cpp:309:16
    #19 0x7fc8fdad382f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c:78:13 in next_token
Shadow bytes around the buggy address:
  0x0c0480002960: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
  0x0c0480002970: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
  0x0c0480002980: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
  0x0c0480002990: fa fa fd fa fa fa fd fa fa fa fd fa fa fa fd fa
  0x0c04800029a0: fa fa 00 00 fa fa 00 04 fa fa 00 00 fa fa 00 fa
=>0x0c04800029b0: fa fa 00 00 fa fa 00 02 fa fa 00 02 fa fa 00[07]
  0x0c04800029c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c04800029d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c04800029e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c04800029f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480002a00: fa fa 00 05 fa fa 01 fa fa fa 00 05 fa fa fa fa
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
==21289==ABORTING
Group: core-security
Input/test case file created by LibFuzzer
Posted patch bug1372467.patch (obsolete) — Splinter Review
This fixes where an unknown attribute causes the parser to leave the pointer on '\n' which then gets skipped over.
Attachment #8877014 - Flags: review?(docfaraday)
Comment on attachment 8877014 [details] [diff] [review]
bug1372467.patch

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

I wonder if maybe we need to make the parse utility functions safer by adding an end pointer... it isn't as if parsing SDP is a performance problem for us.
Attachment #8877014 - Flags: review?(docfaraday) → review+
Blocks: 792125
Group: core-security → media-core-security
I'm marking this sec-high, but feel free to adjust if this isn't accessible from content.
(In reply to Andrew McCreight [:mccr8] from comment #4)
> I'm marking this sec-high, but feel free to adjust if this isn't accessible
> from content.

I think this is pretty easily accessible form content.
This is originally external code. I notified upstream to access the impact on their end.
We need sec-approval so we can land this!  and probably uplift to beta
(In reply to Randell Jesup [:jesup] from comment #7)
> We need sec-approval so we can land this!  and probably uplift to beta

Still waiting for upstream to access, but I can get the sec-approval going.
Comment on attachment 8877014 [details] [diff] [review]
bug1372467.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not very hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? All branches up to release and ESR.

If not all supported branches, which bug introduced the flaw? 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 on release. Backporting if needed is easy.

How likely is this patch to cause regressions; how much testing does it need? This patch could cause potentially problems. Mostly interop problems with other browsers or media servers. We should probably run a manual test cycle against the well known services to be safe.
Attachment #8877014 - Flags: sec-approval?
Comment on attachment 8877014 [details] [diff] [review]
bug1372467.patch

sec-approval+.
We'll want a Beta and ESR52 patch nominated as well.
Attachment #8877014 - Flags: sec-approval? → sec-approval+
Upstream has confirmed to me that they are affected by this problem.
This has had sec-approval+ since July 5. Is this going to land soon?
Flags: needinfo?(drno)
Just rebased and updated commit message.

Carrying forward r+bwc and sec-approval:abillings.
Attachment #8877014 - Attachment is obsolete: true
Flags: needinfo?(drno)
Attachment #8888740 - Flags: review+
This is the same patch as in attachment 8888740 [details] [diff] [review] just for ESR52.
Attachment #8888742 - Flags: review+
Comment on attachment 8888740 [details] [diff] [review]
bug1372467.patch

Approval Request Comment
[Feature/Bug causing the regression]: The bug existed since the WebRTC got added to Firefox.
[User impact if declined]: Browser/content process crashes
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes manually
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: The patch is meant to protect against malformed SDP strings in WebRTC. There is a very small chance that this patch would reject properly formed SDP with this patch applied and then preventing WebRTC calls getting established. But as all of our tests are passing I think the risk is pretty low.
[String changes made/needed]: N/A
Attachment #8888740 - Flags: approval-mozilla-beta?
Comment on attachment 8888742 [details] [diff] [review]
bug1372467_esr52.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is sec-high.
User impact if declined: Browser or content process crashes.
Fix Landed on Version: Not landed yet
Risk to taking this patch (and alternatives if risky): No alternatives. The risk is pretty low. In worst case some WebRTC calls might not get established if the patch has unforeseen side effects.
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8888742 - Flags: approval-mozilla-esr52?
Setting checkin-needed as it's ready to get assuming patches gets approved for beta and esr52.

We should probably not disclose this until upstream has signaled us that they agree to disclose, as they have products affected by this.
Keywords: checkin-needed
Whiteboard: don't disclose until upstream agrees to disclose
(In reply to Nils Ohlmeier [:drno] from comment #9)
> How likely is this patch to cause regressions; how much testing does it
> need? This patch could cause potentially problems. Mostly interop problems
> with other browsers or media servers. We should probably run a manual test
> cycle against the well known services to be safe.

(In reply to Nils Ohlmeier [:drno] from comment #15)
> [Needs manual test from QE? If yes, steps to reproduce]: No

Trying to reconcile both comments, is that because your own testing is sufficient?
Flags: needinfo?(drno)
(In reply to Julien Cristau [:jcristau] from comment #19)
> (In reply to Nils Ohlmeier [:drno] from comment #9)
> > How likely is this patch to cause regressions; how much testing does it
> > need? This patch could cause potentially problems. Mostly interop problems
> > with other browsers or media servers. We should probably run a manual test
> > cycle against the well known services to be safe.
> 
> (In reply to Nils Ohlmeier [:drno] from comment #15)
> > [Needs manual test from QE? If yes, steps to reproduce]: No
> 
> Trying to reconcile both comments, is that because your own testing is
> sufficient?

Sorry I meant to say on the first question that I think the risk for regressions is really low. As we have automated tests between Firefox and Firefox I think that is sufficiently covered. Since we don't have automated test for browser interop this is where there is a small risk.
I did manually run all our automated tests. I also continued to run the libfuzzer test after applying the fix and it did not report any remaining issues.

I'm not objecting to manually testing for example with other browsers and/or WebRTC services at all if you capacity to do so.

If you decide to manual testing I would recommend to test a build which also includes the fix for bug 1372383 as it touches the same code area.
Flags: needinfo?(drno)
https://hg.mozilla.org/mozilla-central/rev/55330a883a17
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8888740 [details] [diff] [review]
bug1372467.patch

sec-high, beta55+
Attachment #8888740 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8888742 [details] [diff] [review]
bug1372467_esr52.patch

webrtc sec fix, esr52.3+
Attachment #8888742 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Reminder to self to add unit tests once disclosed.
Flags: needinfo?(drno)
Whiteboard: don't disclose until upstream agrees to disclose → [adv-main55-][adv-esr52.3-] don't disclose until upstream agrees to disclose
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [adv-main55-][adv-esr52.3-] don't disclose until upstream agrees to disclose → [post-critsmash-triage][adv-main55-][adv-esr52.3-] 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.