Closed Bug 863929 Opened 7 years ago Closed 7 years ago

WebRTC heap-buffer-overflow crash [@gsmsdp_negotiate_codec]


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




Tracking Status
firefox21 --- disabled
firefox22 + fixed
firefox23 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: posidron, Assigned: jesup)


(Blocks 1 open bug)


(4 keywords, Whiteboard: [webrtc][blocking-webrtc+][adv-main22-])

Crash Data


(4 files, 1 obsolete file)

Attached file 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


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
Attached file callstack
Keywords: sec-critical
Priority: -- → P1
Attachment #739891 - Flags: review?(ethanhugg)
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift]
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+
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: nobody → rjesup
Flags: in-testsuite?
Attachment #739891 - Flags: sec-approval? → sec-approval+
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
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?
Attachment #739891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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:

Flags: needinfo?(rjesup)
Keywords: verifyme
I'm spinning up an ASAN build to check this
Flags: needinfo?(rjesup)
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)
Flags: needinfo?(cdiehl)
Attached patch Crashtest v1 (obsolete) — Splinter Review
Attachment #744021 - Attachment is obsolete: true
Attachment #744022 - Flags: review?(ethanhugg)
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+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

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