Closed
Bug 1275648
Opened 8 years ago
Closed 8 years ago
Bug 1208371: check return value of GetPipelineByTrackId_m
Categories
(Core :: Audio/Video: MediaStreamGraph, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | fixed |
firefox49 | --- | fixed |
firefox-esr45 | --- | unaffected |
People
(Reporter: drno, Assigned: drno)
References
Details
Attachments
(1 file, 1 obsolete file)
1.25 KB,
patch
|
jesup
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
When running mochitests with the code removal from bug 1275119 lots of tests start to crash here: https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?from=UpdatePrincipal_m#1462 Which makes sense as GetPipelineByTrackId_m() can return nullptr, which doesn't get checked. Marking sec to be on the sage side until we know for sure that this can't cause harm.
Assignee | ||
Comment 1•8 years ago
|
||
This obviously fixes the actual crash, but then all mochitests start to hang @PC_LOCAL_WAIT_FOR_MEDIA_FLOW with this: 332 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_dataChannel_basicAudioVideo.html | Should have RTP stats for track {cd2efa9d-e4f0-0749-8fa1-474bbb31b12e} waitForRtpFlow/hasFlow@dom/media/tests/mochitest/pc.js:1431:7 waitForRtpFlow/retry/<@dom/media/tests/mochitest/pc.js:1445:22 promise callback*waitForRtpFlow/retry@dom/media/tests/mochitest/pc.js:1444:23 waitForRtpFlow@dom/media/tests/mochitest/pc.js:1447:12 PeerConnectionWrapper.prototype.waitForMediaFlow/<@dom/media/tests/mochitest/pc.js:1461:43 PeerConnectionWrapper.prototype.waitForMediaFlow@dom/media/tests/mochitest/pc.js:1461:7 PC_LOCAL_WAIT_FOR_MEDIA_FLOW@dom/media/tests/mochitest/templates.js:418:12 CommandChain.prototype.execute/</<@dom/media/tests/mochitest/head.js:623:31 promise callback*CommandChain.prototype.execute/<@dom/media/tests/mochitest/head.js:621:14 CommandChain.prototype.execute@dom/media/tests/mochitest/head.js:616:12 promise callback*runNetworkTest/</<@dom/media/tests/mochitest/pc.js:1880:7 runTestWhenReady/<@dom/media/tests/mochitest/head.js:349:41 promise callback*runTestWhenReady@dom/media/tests/mochitest/head.js:349:10 runNetworkTest/<@dom/media/tests/mochitest/pc.js:1879:5 promise callback*runNetworkTest@dom/media/tests/mochitest/pc.js:1878:10 @dom/media/tests/mochitest/test_dataChannel_basicAudioVideo.html:15:3 Looks like SetPrincipalHandle_m() needs to be called, but what can we do it the pipeline is null at this point?
Flags: needinfo?(pehrsons)
Comment 2•8 years ago
|
||
Cache it? I don't think this was a problem when bug 1208371 landed because we probably guaranteed that all remote tracks had a pipeline by the time the principal handle was set (on connection of DTLS or some such). Perhaps bug 1273136 will help with this.
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 3•8 years ago
|
||
So I just took the patch from bug 1275119 and applied to Aurora and I get the same null pointer crash as I get on Nightly. I think it does not really matter if this was a problem when bug 1208371 landed or not, this is potential null reference crash, which is 48 now. Without the patch from bug 1275119 it does not appear to be easily triggerable, but you never know if someone finds another way of abusing this. And BTW a similar call to GetPipelineByTrackId_m() here https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#117 checks for the return type. So I think we need to land the attached patch ASAP and uplift it to 48. Landing bug 1275119 can wait until bug 1273136 is fixed
Assignee | ||
Updated•8 years ago
|
Attachment #8756456 -
Flags: review?(rjesup)
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b6b10b23e00
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8756456 -
Attachment is obsolete: true
Attachment #8756456 -
Flags: review?(rjesup)
Attachment #8756994 -
Flags: review?(rjesup)
Updated•8 years ago
|
Attachment #8756994 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8756994 [details] [diff] [review] bug_1275648.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? As the patch in bug 1275119 removes quite a bit of code and therefore apparently reduces the time until the UpdatePrincipal_m() gets called an attacker would need to find a way to speed up the execution of the code between calling setLocalDescription() or setRemoteDescription() from the JS side up to when these JS calls trigger the call to UpdatePrincipal_m(). That appears pretty hard to me. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No comments or tests Which older supported branches are affected by this flaw? Up to 48 If not all supported branches, which bug introduced the flaw? Bug 1208371 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? How likely is this patch to cause regressions; how much testing does it need?
Attachment #8756994 -
Flags: sec-approval?
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8756994 [details] [diff] [review] bug_1275648.patch Approval Request Comment [Feature/regressing bug #]: Regression from bug 1208371 [User impact if declined]: It might be possible to crash Firefox via a Null reference. [Describe test coverage new/current, TreeHerder]: No test coverage. [Risks and why]: very minimal as it only adds an extra check. [String/UUID change made/needed]: N/A
Attachment #8756994 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Rank: 10
Comment 8•8 years ago
|
||
I'm not sure how to rate this one. If pipeline can only ever be null then it's a DoS/crash that doesn't need to be hidden. If it can potentially be some other uninitialized value (possibly implied by the title of bug 1273136) then this patch won't fix the security problem!
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox-esr45:
--- → unaffected
Flags: needinfo?(drno)
Updated•8 years ago
|
Group: core-security → media-core-security
Assignee | ||
Comment 9•8 years ago
|
||
I have only seen |pipeline| being null or a meaningful value. My understanding is that bug 1273136 tries to fix the problem that |pipeline| can be null for some amount of time, before it gets a meaningful value. I guess that makes this just a regular crash bug.
Flags: needinfo?(drno)
Comment 10•8 years ago
|
||
Comment on attachment 8756994 [details] [diff] [review] bug_1275648.patch Clearing sec-approval then. Let's just check this in.
Attachment #8756994 -
Flags: sec-approval?
Assignee | ||
Updated•8 years ago
|
Summary: Bug 1208371 added buggy code → Bug 1208371: check return value of GetPipelineByTrackId_m
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/87d45d38ab4ae69ce67788899982c7776725cccb Bug 1275648: check return value of GetPipelineByTrackId_m. r=jesup
Comment 12•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/87d45d38ab4a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 13•8 years ago
|
||
Comment on attachment 8756994 [details] [diff] [review] bug_1275648.patch Fix a potential crash, taking it.
Attachment #8756994 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Group: media-core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•