863929 - WebRTC heap-buffer-overflow crash [@gsmsdp_negotiate_codec]

Status: VERIFIED FIXED

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

Description

[Christoph Diehl [:posidron]]

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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: check for out-of-space before using it to fill in codec data

Comment 3

[Ethan Hugg [:ehugg]]

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.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d94e34d8eb4

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4d94e34d8eb4

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ff68eb94307

Comment 9

[Jason Smith [:jsmith]]

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

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

I'm spinning up an ASAN build to check this

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 12

[Christoph Diehl [:posidron]]

Fixed.

Comment 13

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 14

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 15

[Ethan Hugg [:ehugg]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Daniel Holbert [:dholbert]]

(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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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