1677590 - stack-buffer-overflow in [@ sdp_parse_error]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: clusterfuzz-testcase-minimized-SdpParser-4737518792343552

This was found via the SdpParser fuzzer in oss-fuzz.

==1==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd0befe390 at pc 0x55cc4f7da2d4 bp 0x7ffd0befd6b0 sp 0x7ffd0befce38
READ of size 257 at 0x7ffd0befe390 thread T0
    #0 0x55cc4f7da2d3 in firefox
    #1 0x7f2f167028dd in flex_string_vsprintf mozilla-central/third_party/sipcc/cpr_string.c:198:22
    #2 0x7f2f1674de80 in sdp_parse_error mozilla-central/third_party/sipcc/sdp_main.c:1361:5
    #3 0x7f2f16723f04 in sdp_parse_attr_cpar mozilla-central/third_party/sipcc/sdp_attr.c:2915:9
    #4 0x7f2f1670c47b in sdp_parse_attribute mozilla-central/third_party/sipcc/sdp_attr.c:185:14
    #5 0x7f2f1674e812 in sdp_parse mozilla-central/third_party/sipcc/sdp_main.c:1164:18
    #6 0x7f2f1f06128e in mozilla::SipccSdpParser::Parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) mozilla-central/dom/media/webrtc/sdp/SipccSdpParser.cpp:67:25
    #7 0x7f2f15d1a702 in RunSdpParserFuzzing(unsigned char const*, unsigned long) mozilla-central/dom/media/webrtc/tests/fuzztests/sdp_parser_libfuzz.cpp:25:20
    #1 0x55cc4f9ed4ae in firefox
    #2 0x55cc4f9dc3e4 in firefox
    #3 0x55cc4f9e0ac1 in firefox
    #11 0x7f2f24420770 in mozilla::FuzzerRunner::Run(int*, char***) mozilla-central/tools/fuzzing/interface/harness/FuzzerRunner.cpp:75:13
    #12 0x7f2f24354872 in XREMain::XRE_mainStartup(bool*) mozilla-central/toolkit/xre/nsAppRunner.cpp:4190:35
    #13 0x7f2f24365fea in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) mozilla-central/toolkit/xre/nsAppRunner.cpp:5262:12
    #14 0x7f2f24366e40 in XRE_main(int, char**, mozilla::BootstrapConfig const&) mozilla-central/toolkit/xre/nsAppRunner.cpp:5331:21
    #15 0x7f2f243808b0 in mozilla::BootstrapImpl::XRE_main(int, char**, mozilla::BootstrapConfig const&) mozilla-central/toolkit/xre/Bootstrap.cpp:45:12
    #4 0x55cc4f868daa in firefox
    #17 0x7f2f3e78783f in __libc_start_main (/mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_firefox_6180546f5e5f1d75138bf6cea3784693af7a2aa9/revisions/lib/libc.so.6+0x2083f)
    #18 0x55cc4f7bbef8 in _start (/mnt/scratch0/clusterfuzz/bot/builds/clusterfuzz-builds_firefox_6180546f5e5f1d75138bf6cea3784693af7a2aa9/revisions/firefox/firefox+0xb9ef8)

DEDUP_TOKEN: flex_string_vsprintf--sdp_parse_error
Address 0x7ffd0befe390 is located in stack of thread T0 at offset 304 in frame
    #0 0x7f2f1672393f in sdp_parse_attr_cpar mozilla-central/third_party/sipcc/sdp_attr.c:2853

DEDUP_TOKEN: sdp_parse_attr_cpar
  This frame has 2 object(s):
    [32, 36) 'result' (line 2855)
    [48, 304) 'tmp' (line 2859) <== Memory access at offset 304 overflows this variable

Comment 1

[Daniel Veditz [:dveditz]]

Is our copy of sipcc up to date wrt any recent security fixes?

Comment 2

[Nils Ohlmeier [:drno]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

Is our copy of sipcc up to date wrt any recent security fixes?

Unfortunately sipcc was a one time code drop from Cisco and is not something we can sync from regularly. I'll see if I can find an owner for this.

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: Bug 1677590: improved SDP parser error handling. r=ng

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Byron, what do you think about always terminating the output buffer: https://searchfox.org/mozilla-central/source/third_party/sipcc/sdp_utils.c#99 ?

Comment 5

[Byron Campen [:bwc]]

Seems reasonable.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:ng, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 9190458 [details]
Bug 1677590: improved SDP parser error handling. r=ng

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it is probably pretty easy to figure out that this patch is addressing missing zero termination.
• 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
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Highly unlikely as the issue is in part of the code which is not utilized for Firefox WebRTC functionality at all.

Comment 8

[Nils Ohlmeier [:drno]]

Attachment: bug1677590_esr78.patch

Comment 9

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Nils, are you saying that this code is not reachable in Firefox; and is just a fix so the fuzzer doesn't find and re-report it? If so, this can be changed to sec-other and sec-approval is not needed.

Comment 10

[Nils Ohlmeier [:drno]]

(In reply to Tom Ritter [:tjr] (ni? for response to sec-[advisories/bounties/ratings/cves]) from comment #9)

Nils, are you saying that this code is not reachable in Firefox; and is just a fix so the fuzzer doesn't find and re-report it? If so, this can be changed to sec-other and sec-approval is not needed.

SDP is a text blob which gets passed in via JS into our parser. Thus an attacker who knows about this issue can very easily craft an SDP text blob which triggers this bug. But unfortunately our SDP parser supports lots of tokens which are never used in WebRTC (because it was a one time gift from a different product which supported a lot more features then needed in WebRTC).
After parsing Firefox ignores these unused tokens. And no other browser will ever create these tokens. That is why it is highly unlikely that they will cause regressions.

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9190458 [details]
Bug 1677590: improved SDP parser error handling. r=ng

Approved to land Jan 13th

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/cbe57264ea8bfa1537a7b425270939ca27baac91
https://hg.mozilla.org/mozilla-central/rev/cbe57264ea8b

Please submit the uplift requests.

Comment 13

[Nils Ohlmeier [:drno]]

Comment on attachment 9190458 [details]
Bug 1677590: improved SDP parser error handling. r=ng

Beta/Release Uplift Approval Request

• User impact if declined: Could potentially be abused to take over Firefox remotely.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This code never gets triggered in normal WebRTC calls. So it should only get executed when someone tries to find a problem in our parser, which unfortunately parses more then it needs to.
• String changes made/needed: N/A

Comment 14

[Nils Ohlmeier [:drno]]

Comment on attachment 9191731 [details] [diff] [review]
bug1677590_esr78.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: Could potentially be abused to take over Firefox remotely.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This code never gets triggered in normal WebRTC calls. So it should only get executed when someone tries to find a problem in our parser, which unfortunately parses more then it needs to.
• String or UUID changes made by this patch: N/A

Comment 15

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

Comment on attachment 9190458 [details]
Bug 1677590: improved SDP parser error handling. r=ng

approved for 85.0b9

Comment 16

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

https://hg.mozilla.org/releases/mozilla-beta/rev/f2a3f73c5e9b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9191731 [details] [diff] [review]
bug1677590_esr78.patch

Approved for 78.7esr.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/925d02e4589c

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.