Bug 1433005 (CVE-2018-5130)

Crash when WebRTC RTP payload type is incorrect

VERIFIED FIXED in Firefox -esr52

Status

()

defect
P1
normal
Rank:
5
VERIFIED FIXED
a year ago
9 months ago

People

(Reporter: drunkenf00l, Assigned: dminor)

Tracking

({sec-high})

57 Branch
mozilla60
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr5259+ verified, firefox58 wontfix, firefox59+ verified, firefox60+ verified)

Details

(Whiteboard: [post-critsmash-triage][adv-main59+][adv-esr52.7+])

Attachments

(4 attachments, 6 obsolete attachments)

Reporter

Description

a year ago
We discovered by accident that if from our remote WebRTC endpoint we send RTP packets with a correct SSRC and body but an incorrect PayloadType we can crash Firefox and cause invalid read/write or divide by zero exceptions.

The details of what we are sending are RTP packets with contents originally generated by a client using Chrome with opus/48000/2 contents, but the payload type from Chrome is 111 and we were inadvertently putting that into the payload type for the packet forwarded to Firefox rather than 109 as Firefox desired via the negotiated SDP for it.  The Firefox client receiving the packet seemed to consider it valid, about:webrtc showed packets being received and no errors, various WebRTC logging in Firefox also looked happy, but fairly quickly we’d crash with one of the below presumably because the audio decoding was invalid due to incorrect payload type.

Once we fixed our server code forwarding these packets to set correct payload types all the crashes stopped, but this appears to be a remotely exploitable crash bug at the least with possible remote code execution potential if packets were carefully crafted and targeted? So just in case, I've marked this as a security bug.

Links to submitted crash reports:

https://crash-stats.mozilla.com/report/index/bp-c1cd72f1-a6f4-40de-84bb-a1ac30180123 - Write Violation
https://crash-stats.mozilla.com/report/index/bp-ffd44cad-17cc-4d8c-b94d-98b0f0180123 - Divide by Zero
https://crash-stats.mozilla.com/report/index/54087df8-9b05-4c7c-af2e-a2c270180123 - Read Violation
Randell, can you take a look at this?
Flags: needinfo?(rjesup)
Keywords: sec-high
Group: core-security → media-core-security
Forwarding...
Flags: needinfo?(rjesup)
Flags: needinfo?(drno)
Flags: needinfo?(docfaraday)
This is all stuff in the media stack. We could maybe add some filtering logic in MediaPipeline that will strip out bad packets, but the fact that the media stack screws up the memory management when the payload type is wrong suggests that there are probably similar memory management bugs that will be triggered by bad payloads, which is something we can't check in MediaPipeline...
Flags: needinfo?(docfaraday)
Flags: needinfo?(dminor)
We've fuzzed some video stuff, but this is audio data... probably we should fuzz audio inputs (feed in "Opus" data (and other codecs!) with fuzzed data (wrong/wrong-length/right-header-wrong-rest, etc).  Most of these appear to be in NetEq, which is after the codecs; it would be interesting to see what the codecs are decoding to or if failure/sanity (output length) checks are missing somewhere.

Also check upstream for neteq fixes.
Flags: needinfo?(cdiehl)
Yeah I was also thinking about checking for the PT type. But I don't get why the PT actually matters after the transport layer. So I would like to replicate to find out the root cause, rather then plastering over it.
Flags: needinfo?(drno)
Our current version is:

#define WEBRTC_SVNVERSION 7864
#define WEBRTC_PULL_DATE "Wed Dec 10 12:23:33 EST 2014"

Though, I went back through the commits dating back 4 month and picked out the most obvious things to me:

https://webrtc.googlesource.com/src/+/bc5c69f8e7a5b4b8b9dcfc979d8bff09ffb78daa Use of unititialized value in AECM.
https://webrtc.googlesource.com/src/+/d2b5b1f5bae94c24cc279fe198ff28236821f1ac Division by zero in NoiseSuppression.
https://webrtc.googlesource.com/src/+/ab20a6016c5d0798a00dd566c78f5f49065a9492 AEC-m and AEC-2 fuzzing.
https://webrtc.googlesource.com/src/+/49ccbdb9d69f47b66df286e766f18f95dc611119 Add fuzzer for ForwardErrorCorrection::DecodeFec.
https://webrtc.googlesource.com/src/+/32c6ae249fb879d75de119bc63b9a16f9f14bdca Fix fuzzer-found undefined behavior in webrtc_cng

Before Dec 10:
https://webrtc.googlesource.com/src/+/5dcbbfd1533bf14efee5b6bd2b879d515d1095c5 Create a fuzzer for ComfortNoiseDecoder
https://webrtc.googlesource.com/src/+/bc5a40870c6d83642523d27c660f75896ac5d6c3 Fix off-by-one error when removing information about missing packet in PacketBuffer.
https://webrtc.googlesource.com/src/+/8731176b922ad7d82d0407f2ac36d9f5396f9ac2 NetEq: Fix an UBSan error
https://webrtc.googlesource.com/src/+/ba68aabb06b627e1773fdfdcbeec05cd50c8c500 Fix of integer overflow in WebRtcAecm_ProcessBlock / ApmTest.Process


I'm going to add some SDP injection routines to our WebRTC DOM module. We lately sticked in fuzzing SDP separately.
Flags: needinfo?(cdiehl)
(In reply to Christoph Diehl [:posidron] from comment #6)
> Our current version is:
> 
> #define WEBRTC_SVNVERSION 7864
> #define WEBRTC_PULL_DATE "Wed Dec 10 12:23:33 EST 2014"

Actually, we're on Bug 1341285: Webrtc updated to branch 57 rev 52b6562a10b495; initial pull made Feb 2 2017 14:38 EST

They switched off SVN some time ago, so that file is no longer relevant.

> 
> Though, I went back through the commits dating back 4 month and picked out
> the most obvious things to me:
> 
> https://webrtc.googlesource.com/src/+/
> bc5c69f8e7a5b4b8b9dcfc979d8bff09ffb78daa Use of unititialized value in AECM.
> https://webrtc.googlesource.com/src/+/
> d2b5b1f5bae94c24cc279fe198ff28236821f1ac Division by zero in
> NoiseSuppression.
> https://webrtc.googlesource.com/src/+/
> ab20a6016c5d0798a00dd566c78f5f49065a9492 AEC-m and AEC-2 fuzzing.
> https://webrtc.googlesource.com/src/+/
> 49ccbdb9d69f47b66df286e766f18f95dc611119 Add fuzzer for
> ForwardErrorCorrection::DecodeFec.
> https://webrtc.googlesource.com/src/+/
> 32c6ae249fb879d75de119bc63b9a16f9f14bdca Fix fuzzer-found undefined behavior
> in webrtc_cng
> 
> Before Dec 10:
> https://webrtc.googlesource.com/src/+/
> 5dcbbfd1533bf14efee5b6bd2b879d515d1095c5 Create a fuzzer for
> ComfortNoiseDecoder
> https://webrtc.googlesource.com/src/+/
> bc5a40870c6d83642523d27c660f75896ac5d6c3 Fix off-by-one error when removing
> information about missing packet in PacketBuffer.
> https://webrtc.googlesource.com/src/+/
> 8731176b922ad7d82d0407f2ac36d9f5396f9ac2 NetEq: Fix an UBSan error
> https://webrtc.googlesource.com/src/+/
> ba68aabb06b627e1773fdfdcbeec05cd50c8c500 Fix of integer overflow in
> WebRtcAecm_ProcessBlock / ApmTest.Process

Those may still be relevant, however.  We can cherrypick if needed/useful.
Thanks jesup, I first overlooked '14 and after posting thought we wouldn't update it for cherry picks or minor updates. "Before Dec 10" are of course simply from the past December.
Posted file testcase.html (obsolete) —
Based on the description of the reporter I tried to come up with a testcase (attached):

See line 36:

sdp.replace(/a=rtpmap:\d+ opus\/48000\/2\r\n/g, 'a=rtpmap:111 opus/4800/2\r\n')

Seem to crash reliably.
Posted file testcase.html
Attachment #8945713 - Attachment is obsolete: true
Assignee

Comment 12

a year ago
Ran the supplied testcase (thank you!) on an ASAN build, appears to be memory corruption in JsepTrack:

==23077==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000263e98 at pc 0x7f21358a788f bp 0x7fff0f4a6bf0 sp 0x7fff0f4a6be8
WRITE of size 8 at 0x602000263e98 thread T0 (file:// Content)
    #0 0x7f21358a788e in mozilla::JsepTrack::NegotiateCodecs(mozilla::SdpMediaSection const&, std::vector<mozilla::JsepCodecDescription*, std::allocator<mozilla::JsepCodecDescription*> >*, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >*) const /home/dminor/src/firefox-signaling/media/webrtc/signaling/src/jsep/JsepTrack.cpp:501:28
    #1 0x7f213589743d in mozilla::JsepTrack::Negotiate(mozilla::SdpMediaSection const&, mozilla::SdpMediaSection const&) /home/dminor/src/firefox-signaling/media/webrtc/signaling/src/jsep/JsepTrack.cpp:515:3
    #2 0x7f2135894518 in mozilla::JsepSessionImpl::MakeNegotiatedTransceiver(mozilla::SdpMediaSection const&, mozilla::SdpMediaSection const&, bool, unsigned long, mozilla::JsepTransceiver*) /home/dminor/src/firefox-signaling/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1069:29
    #3 0x7f2135888c4a in mozilla::JsepSessionImpl::HandleNegotiatedSession(mozilla::UniquePtr<mozilla::Sdp, mozilla::DefaultDelete<mozilla::Sdp> > const&, mozilla::UniquePtr<mozilla::Sdp, mozilla::DefaultDelete<mozilla::Sdp> > const&) /home/dminor/src/firefox-signaling/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:993:10
    #4 0x7f213588d7fc in mozilla::JsepSessionImpl::SetRemoteDescriptionAnswer(mozilla::JsepSdpType, mozilla::UniquePtr<mozilla::Sdp, mozilla::DefaultDelete<mozilla::Sdp> >) /home/dminor/src/firefox-signaling/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1376:17
    #5 0x7f213588d7fc in mozilla::JsepSessionImpl::SetRemoteDescription(mozilla::JsepSdpType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/dminor/src/firefox-signaling/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:924
    #6 0x7f21359877c5 in mozilla::PeerConnectionImpl::SetRemoteDescription(int, char const*) /home/dminor/src/firefox-signaling/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1876:32
    #7 0x7f21380f7bf6 in mozilla::PeerConnectionImpl::SetRemoteDescription(int, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /home/dminor/src/firefox-signaling/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:368:10
    #8 0x7f21380f7bf6 in mozilla::dom::PeerConnectionImplBinding::setRemoteDescription(JSContext*, JS::Handle<JSObject*>, mozilla::PeerConnectionImpl*, JSJitMethodCallArgs const&) /home/dminor/src/firefox-signaling/objdir-ff-asan/dom/bindings/PeerConnectionImplBinding.cpp:240
    #9 0x7f2139da4f13 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/dminor/src/firefox-signaling/dom/bindings/BindingUtils.cpp:3036:13
    #10 0x7f21411d509d in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/dminor/src/firefox-signaling/js/src/jscntxtinlines.h:291:15
Assignee: nobody → dminor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(dminor)
Assignee

Comment 13

a year ago
Oops, I see the callstack was already attached :)
Assignee

Comment 14

a year ago
Posted patch Don't insert DTMF codec twice (obsolete) — Splinter Review
The problem was that we only had the DTMF codec present in the codecs array, so we would attempt to insert a second copy of it past the end of the array, and then resize the array to accommodate it. This moves the resize so that we are guaranteed to have enough space for any extra codecs we're adding. It also adds a check that prevents us from adding a second copy of the DTMF codec, as that leads to a use-after-free soon after.
Attachment #8945831 - Flags: review?(docfaraday)
Comment on attachment 8945831 [details] [diff] [review]
Don't insert DTMF codec twice

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

This is getting hard to read. My suggestion is to:

1. Find each entry we want to use (the actual codec, the dtmf, the red, and anything else), and remove it from the codec array.
2. Empty out the codec array, deleting anything that is left.
3. Build a new codec array.
Attachment #8945831 - Flags: review?(docfaraday)
Assignee

Comment 16

a year ago
That makes sense to me.
Attachment #8945831 - Attachment is obsolete: true
Attachment #8945843 - Flags: review?(docfaraday)
Comment on attachment 8945843 [details] [diff] [review]
Simplify codec pruning in NegotiateCodecs

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

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp
@@ +492,5 @@
>    if (!codecs->empty() && !red) {
> +    std::vector<JsepCodecDescription*> codecsToKeep;
> +
> +    // Keep our preferred codec.
> +    codecsToKeep.push_back((*codecs)[0]);

Might this be dtmf or ulpfec? We probably need to find by iterating, probably in the same loop that handles deletions below.

@@ +504,2 @@
>      for (size_t i = 1; i < codecs->size(); ++i) {
> +      if (dtmf != (*codecs)[i]) {

Probably need to check ulpfec here too.
Attachment #8945843 - Flags: review?(docfaraday) → review-
Assignee

Comment 18

a year ago
(In reply to Byron Campen [:bwc] from comment #17)
> Comment on attachment 8945843 [details] [diff] [review]
> Simplify codec pruning in NegotiateCodecs
> 
> Review of attachment 8945843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/jsep/JsepTrack.cpp
> @@ +492,5 @@
> >    if (!codecs->empty() && !red) {
> > +    std::vector<JsepCodecDescription*> codecsToKeep;
> > +
> > +    // Keep our preferred codec.
> > +    codecsToKeep.push_back((*codecs)[0]);
> 
> Might this be dtmf or ulpfec? We probably need to find by iterating,
> probably in the same loop that handles deletions below.
> 

Does this mean that we should iterate through the array and take the first codec that is not dtmf or ulpfec as our preferred codec? Shouldn't the sort done at [1] make sure that the preferred codec is sorted ahead of dtmf or ulpfec? If not, should we fix the sort?

> @@ +504,2 @@
> >      for (size_t i = 1; i < codecs->size(); ++i) {
> > +      if (dtmf != (*codecs)[i]) {
> 
> Probably need to check ulpfec here too.

We are now keeping ulpfec as well as dtmf?

It might be safer to just fix the crash here and make any behavioural changes in a non-sec bug where things are easier to test.

[1] https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/media/webrtc/signaling/src/jsep/JsepTrack.cpp#483
(In reply to Dan Minor [:dminor] from comment #18)
> (In reply to Byron Campen [:bwc] from comment #17)
> > Comment on attachment 8945843 [details] [diff] [review]
> > Simplify codec pruning in NegotiateCodecs
> > 
> > Review of attachment 8945843 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/webrtc/signaling/src/jsep/JsepTrack.cpp
> > @@ +492,5 @@
> > >    if (!codecs->empty() && !red) {
> > > +    std::vector<JsepCodecDescription*> codecsToKeep;
> > > +
> > > +    // Keep our preferred codec.
> > > +    codecsToKeep.push_back((*codecs)[0]);
> > 
> > Might this be dtmf or ulpfec? We probably need to find by iterating,
> > probably in the same loop that handles deletions below.
> > 
> 
> Does this mean that we should iterate through the array and take the first
> codec that is not dtmf or ulpfec as our preferred codec? Shouldn't the sort
> done at [1] make sure that the preferred codec is sorted ahead of dtmf or
> ulpfec? If not, should we fix the sort?

  That sort only causes "strongly preferred" codecs to override the preference indicated by the remote end. Unless prefs are set, there are no "strongly preferred" codecs. (https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/media/webrtc/signaling/src/jsep/JsepTrack.cpp#388) So, you'll need to iterate.

> > @@ +504,2 @@
> > >      for (size_t i = 1; i < codecs->size(); ++i) {
> > > +      if (dtmf != (*codecs)[i]) {
> > 
> > Probably need to check ulpfec here too.
> 
> We are now keeping ulpfec as well as dtmf?
> 
> It might be safer to just fix the crash here and make any behavioural
> changes in a non-sec bug where things are easier to test.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 062e1cf551f5bf3f0af33671b818f75a55ac497b/media/webrtc/signaling/src/jsep/
> JsepTrack.cpp#483

It is easier to reason about this code's memory safety if we cover all the bases.
Assignee

Comment 20

a year ago
Thank you for the explanation, I was having a hard time reconciling the changes you were asking for with the current behaviour.
Assignee

Comment 21

a year ago
Attachment #8945843 - Attachment is obsolete: true
Attachment #8945891 - Flags: review?(docfaraday)
Comment on attachment 8945891 [details] [diff] [review]
Simplify codec pruning in NegotiateCodecs

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

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp
@@ +496,5 @@
> +        codecsToKeep.push_back(codec);
> +      } else if (codec == ulpfec) {
> +        codecsToKeep.push_back(codec);
> +      } else if (!foundPreferredCodec) {
> +        codecsToKeep.push_back(codec);

I wonder if webrtc.org is gonna barf if this isn't first? Should check this, or put it at the front using insert.
Attachment #8945891 - Flags: review?(docfaraday) → review+
Assignee

Comment 23

a year ago
This inserts the preferred codec at the beginning of the vector, as suggested. Carrying forward r+.
Attachment #8945891 - Attachment is obsolete: true
Attachment #8946238 - Flags: review+
Assignee

Comment 24

a year ago
Comment on attachment 8946238 [details] [diff] [review]
Simplify codec pruning in NegotiateCodecs

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Since the patch looks like a refactor, I don't think the underlying problem it fixes is that obvious.

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, the code was introduced in Bug 1291714.
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?
I believe the same patch will apply to all branches. If not, the changes required should be minimal.
How likely is this patch to cause regressions; how much testing does it need?
Unlikely, I would expect our standard unit tests to catch any problems.
Attachment #8946238 - Flags: sec-approval?
Assignee

Updated

a year ago
Flags: in-testsuite?
sec-approval+ for trunk. Can we get the patch nominated for Beta and ESR52 as well (though we might delay ESR52 checkin for a week or two to not shine a giant spotlight on it as a security bug).
Attachment #8946238 - Flags: sec-approval? → sec-approval+
Assignee

Comment 26

a year ago
Comment on attachment 8946238 [details] [diff] [review]
Simplify codec pruning in NegotiateCodecs

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1291714
[User impact if declined]:
Crashes / security problems.
[Is this code covered by automated tests?]:
Not currently, but there is a testcase attached to this bug that could be worked into mochitest.
[Has the fix been verified in Nightly?]:
No.
[Needs manual test from QE? If yes, steps to reproduce]:
No. 
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
This is just a small change to how codecs are chosen during negotiation.
[String changes made/needed]:
None.
Attachment #8946238 - Flags: approval-mozilla-beta?
Assignee

Comment 27

a year ago
Comment on attachment 8946238 [details] [diff] [review]
Simplify codec pruning in NegotiateCodecs

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Crashes / security problems.
Fix Landed on Version:
Will land on 60.
Risk to taking this patch (and alternatives if risky): 
Limited risk.
String or UUID changes made by this patch: 
None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8946238 - Flags: approval-mozilla-esr52?
Assignee

Comment 29

a year ago
I forgot to run the gtests before landing this. We're now keeping the ulpfec codec which breaks some of the tests in JsepSessionTest::ValidateAnsweredCodecParams. This looks like expected changes to me, but I wanted to double check just to be safe. Assuming this is fine, I'll roll this up into the main patch when I reland.
Attachment #8946708 - Flags: review?(docfaraday)
Assignee

Updated

a year ago
Rank: 5
Priority: -- → P1
Comment on attachment 8946708 [details] [diff] [review]
Fix up JsepSessionTest::ValidateAnsweredCodecParams gtest

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

Verify with jesup that enabling ulpfec is something we want to do. If not, we need to comment those lines out, and put a TODO + bug number in the source.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp
@@ +3565,4 @@
>    // ASSERT_EQ(3U, video_section.GetFormats().size());
>    ASSERT_EQ("120", video_section.GetFormats()[0]);
> +  ASSERT_EQ("123", video_section.GetFormats()[1]); // ULPFEC
> +  //ASSERT_EQ("121", video_section.GetFormats()[1]);

Re-index these I guess.
Attachment #8946708 - Flags: review?(docfaraday) → review+
Backed out for Gtest failures on Linux x64 asan and Windows 10 x64 debug:

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=62da02a0aa77e00dc89de31040bb6fbc357b5136&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=159356534&repo=mozilla-inbound

[task 2018-01-30T16:17:56.476Z] 16:17:56  WARNING -  TEST-UNEXPECTED-FAIL | JsepSessionTest.ValidateAnsweredCodecParams |       Expected: 1U
[task 2018-01-30T16:17:56.476Z] 16:17:56     INFO -        Which is: 1
[task 2018-01-30T16:17:56.477Z] 16:17:56     INFO -  To be equal to: video_section.GetFormats().size()
[task 2018-01-30T16:17:56.477Z] 16:17:56     INFO -        Which is: 2 @ /builds/worker/workspace/build/src/media/webrtc/signaling/gtest/jsep_session_unittest.cpp:3564
[task 2018-01-30T16:17:56.478Z] 16:17:56  WARNING -  TEST-UNEXPECTED-FAIL | JsepSessionTest.ValidateAnsweredCodecParams | test completed (time: 7ms)
Flags: needinfo?(dminor)
Assignee

Comment 32

a year ago
Randell, I'm guessing we shouldn't retain the ulpfec codec here until we enable ulpfec support over in Bug 875922?
Flags: needinfo?(dminor) → needinfo?(rjesup)
Correct - and certainly wouldn't block this fix for something we're not enabling yet
Flags: needinfo?(rjesup)
Assignee

Comment 34

a year ago
Version of the patch with ulpfec disabled.
Attachment #8946708 - Attachment is obsolete: true
Assignee

Comment 35

a year ago
Correct version of patch :/
Attachment #8946788 - Attachment is obsolete: true
Comment on attachment 8946789 [details] [diff] [review]
Simplify codec pruning in NegotiateCodecs (no ulpfec codec)

Crash fix for a sec-high issue, let's uplift it. (This patch rather than the originally marked one)
Attachment #8946789 - Flags: approval-mozilla-esr52+
Attachment #8946789 - Flags: approval-mozilla-beta+
Attachment #8946238 - Flags: approval-mozilla-esr52?
Attachment #8946238 - Flags: approval-mozilla-esr52-
Attachment #8946238 - Flags: approval-mozilla-beta?
Attachment #8946238 - Flags: approval-mozilla-beta-
https://hg.mozilla.org/releases/mozilla-beta/rev/90ae99509f1e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Group: media-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
I have reproduced this bug using an affected ASAN build, 59.01 Nightly (2018-01-15) and running the test case from comment 11.

I cannot reproduce it anymore on the latest ASAN builds under Ubuntu 16.04 x64: latest Nightly 60.0a1 (2018-03-05), Beta 59.0b14 (20180303190036) and 52.7.0 esr (20180304210040).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+][adv-esr52.7+]
Alias: CVE-2018-5130
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.