red parser goes into endless loop on broken SDP

RESOLVED FIXED in Firefox 50

Status

()

Core
WebRTC: Signaling
P1
normal
Rank:
10
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: drno, Assigned: drno)

Tracking

({csectype-dos, regression})

50 Branch
mozilla52
csectype-dos, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox-esr45 unaffected, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
backlog: --- → webrtc/webaudio+
Rank: 10
status-firefox49: --- → unaffected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox-esr45: --- → unaffected
(Assignee)

Updated

2 years ago
Blocks: 1309641
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

https://reviewboard.mozilla.org/r/89058/#review88262

::: media/webrtc/signaling/test/sdp_unittests.cpp:444
(Diff revision 1)
>  
>  TEST_F(SdpTest, parseRtcpFbFooBarBaz) {
>    ParseSdp(kVideoSdp + "a=rtcp-fb:120 foo bar baz\r\n");
>  }
>  
> +static const std::string kVideoSdpWithUnknonwBrokenFtmp =

typo Unknonw should be Unknown

Comment 3

2 years ago
mozreview-review
Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

https://reviewboard.mozilla.org/r/89058/#review88270

One nit, otherwise good.
Attachment #8805313 - Flags: review?(mfroman) → review+
Comment hidden (mozreview-request)

Comment 5

2 years ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/aeb0df69182a
exit red parser loop if stroul failed. r=mjf
(Assignee)

Comment 6

2 years ago
Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

Approval Request Comment
[Feature/regressing bug #]: Bug 1275360 added support for parsing red, which has a problem.

[User impact if declined]: Firefox users who use WebRTC services or connect to WebRTC devices which send broken SDP to Firefox can cause Firefox's main thread to enter an endless loop from which the user can only terminate Firefox via the OS.

[Describe test coverage new/current, TreeHerder]: The patch contains a new unit test which shows that their is no longer an endless loop.

[Risks and why]: The risk is pretty low as it only set an additional exit criteria from a loop which should only be entered in rare corner cases.

[String/UUID change made/needed]: N/A
Attachment #8805313 - Flags: approval-mozilla-beta?
Attachment #8805313 - Flags: approval-mozilla-aurora?
Note: this could be used to trivially DOS the browser (though I'm sure there are other ways to do so easily).
Keywords: csectype-dos

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aeb0df69182a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 9

2 years ago
Comment on attachment 8805313 [details]
Bug 1313527: exit red parser loop if stroul failed.

Fixes a hang (and see comment 7) with invalid SDP, Aurora51+, Beta50+
Attachment #8805313 - Flags: approval-mozilla-beta?
Attachment #8805313 - Flags: approval-mozilla-beta+
Attachment #8805313 - Flags: approval-mozilla-aurora?
Attachment #8805313 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.