If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

WebRTC heap-buffer-overflow crash [@gsmsdp_negotiate_codec]

VERIFIED FIXED in Firefox 22

Status

()

Core
WebRTC: Signaling
P1
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: posidron, Assigned: jesup)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla23
x86_64
Mac OS X
crash, csectype-bounds, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 disabled, firefox22+ fixed, firefox23+ verified, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [webrtc][blocking-webrtc+][adv-main22-], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 739875 [details]
testcase

This happened during fuzzing the SDP based on a diff with the original SDP the following attribute seems to make trouble:

-a=rtpmap:120 VP8/90000
+a=rtpmap:120 telephone-event/90000


media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:3018

static int
gsmsdp_negotiate_codec (fsmdef_dcb_t *dcb_p, cc_sdp_t *sdp_p,
                        fsmdef_media_t *media, boolean offer,
                        boolean initial_offer, uint16 media_level)
{
[...]
    /* We've found a codec in common. Configure the coresponding
       payload information structure */
    codec = slave_list_p[j];
    payload_info = &(media->payloads[media->num_payloads]);

    if (master_list_p == remote_codecs) {
        remote_pt = remote_payload_types[i];
    } else {
        remote_pt = remote_payload_types[j];
    }

*   payload_info->codec_type = codec;


Tested with m-i changeset: 129367:9ff5c0134bbf
(Reporter)

Comment 1

5 years ago
Created attachment 739879 [details]
callstack
(Reporter)

Updated

5 years ago
Keywords: sec-critical
(Assignee)

Updated

5 years ago
Priority: -- → P1
(Assignee)

Comment 2

5 years ago
Created attachment 739891 [details] [diff] [review]
check for out-of-space before using it to fill in codec data
(Assignee)

Updated

5 years ago
Attachment #739891 - Flags: review?(ethanhugg)
(Assignee)

Updated

5 years ago
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift]

Comment 3

5 years ago
Comment on attachment 739891 [details] [diff] [review]
check for out-of-space before using it to fill in codec data

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

Looks good to me.
Attachment #739891 - Flags: review?(ethanhugg) → review+
(Assignee)

Comment 4

5 years ago
Comment on attachment 739891 [details] [diff] [review]
check for out-of-space before using it to fill in codec data

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very tough, you'd need to control what bytes are immediately following the allocated buffer (and have them not be unused/waste space), and the cause the SDP information in them to provoke a security break.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The bug title and bugs clearly show the memory error.  They don't make it easy to turn it into an exploit.

Which older supported branches are affected by this flaw? 22 (I'll ask for uplift); earlier versions have the flaw but are disabled-by-default.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backport will be trivial.
 
How likely is this patch to cause regressions; how much testing does it need?  Should be very easy to test.  Very little risk of regressions
Attachment #739891 - Flags: sec-approval?
(Assignee)

Updated

5 years ago
Assignee: nobody → rjesup
(Assignee)

Updated

5 years ago
status-firefox21: --- → disabled
status-firefox22: --- → affected
status-firefox23: --- → affected

Updated

5 years ago
Flags: in-testsuite?
Attachment #739891 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d94e34d8eb4
Target Milestone: --- → mozilla23

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/4d94e34d8eb4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox23: affected → fixed
Resolution: --- → FIXED

Updated

5 years ago
Keywords: verifyme
(Assignee)

Comment 7

5 years ago
Comment on attachment 739891 [details] [diff] [review]
check for out-of-space before using it to fill in codec data

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Possible security issues (hard to exploit as it's a limited fence overwrite without much if any modifiable data)

Testing completed (on m-c, etc.): on m-c.  Need cdiehl to verify

Risk to taking this patch (and alternatives if risky): Very low.  Bumps counter before checking if the counter will be out-of-bounds for the coming iteratioon.

String or IDL/UUID changes made by this patch: none
Attachment #739891 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #739891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox22: --- → +
tracking-firefox23: --- → +
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ff68eb94307
status-firefox22: affected → fixed
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift] → [webrtc][blocking-webrtc+]
So I'm actually still crashing with the test case, but I'm getting different stacks unrelated to WebRTC I think. Given that I'm not seeing the original stack on the bug, I'll mark as verified, but I'm putting needinfo on Randell to find out why we're still crashing.

Crash Reports:

* https://crash-stats.mozilla.com/report/index/bp-d3651525-4d9a-480d-b38f-e6ffa2130424
* https://crash-stats.mozilla.com/report/index/bp-e1940a39-f976-4110-a4a3-8c9512130424
Status: RESOLVED → VERIFIED
Flags: needinfo?(rjesup)
Keywords: verifyme

Updated

5 years ago
status-firefox23: fixed → verified
(Assignee)

Comment 10

5 years ago
I'm spinning up an ASAN build to check this
Flags: needinfo?(rjesup)
(Assignee)

Comment 11

5 years ago
Ok, with an ASAN build on Linux off current inbound, I'm not seeing anything.  I reloaded the testcase a whole bunch of times.

So it seems to me like those crashes are unrelated, or not due to a memory error (and not reproducible on Linux).

Christoph, can you retest when you have a chance?

Also, those stacks don't make a ton of sense to me in any case....
Flags: needinfo?(cdiehl)
(Reporter)

Comment 12

5 years ago
Fixed.
Flags: needinfo?(cdiehl)
Created attachment 744021 [details] [diff] [review]
Crashtest v1

Updated

5 years ago
Attachment #744021 - Attachment is obsolete: true
Created attachment 744022 [details] [diff] [review]
Crashtest v1

Updated

5 years ago
Attachment #744022 - Flags: review?(ethanhugg)
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected

Comment 15

4 years ago
Comment on attachment 744022 [details] [diff] [review]
Crashtest v1

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

Ran this test under the debugger and it does hit the changed code in gsm_sdp.c
Attachment #744022 - Flags: review?(ethanhugg) → review+

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/adaaf2bf1883
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/adaaf2bf1883

nit: This added "load 863929.html" at the wrong spot in crashtests.list.  I pushed a DONTBUILD followup to shift it to the correct position:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/678644b5ba99
https://hg.mozilla.org/mozilla-central/rev/adaaf2bf1883
Whiteboard: [webrtc][blocking-webrtc+] → [webrtc][blocking-webrtc+][adv-main22-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.