heap-use-after-free and assertion with RTCPeerConnection
Categories
(Core :: WebRTC, defect, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox-esr60 | --- | unaffected |
| firefox67 | --- | unaffected |
| firefox68 | + | fixed |
| firefox69 | + | fixed |
People
(Reporter: nils, Assigned: bwc, NeedInfo)
References
(Regression)
Details
(Keywords: csectype-uaf, regression, sec-high, Whiteboard: [post-critsmash-triage])
Attachments
(3 files)
|
8.73 KB,
text/plain
|
Details | |
|
12.50 KB,
text/plain
|
Details | |
|
47 bytes,
text/x-phabricator-request
|
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Review |
The following testcase crashes the latest ASAN build of Firefox 68.0a1.
Before minimising the testcase triggered a heap-use-after-free (see attached asan_uaf.txt after minimising it trigger the following assertion (see asan_assert.txt):
Assertion failure: !mMainThread, at /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:119
crash.html:
<script>
function start() {
FuzzingFunctions.garbageCollect();FuzzingFunctions.cycleCollect();
o5=document.createElement('canvas');
o15=document.documentElement;
o79=document.createElementNS('http://www.w3.org/1999/xhtml','iframe');
o15.appendChild(o79);
o136=o79.contentWindow;
o182=new Blob([], {'type': 'audio/mp3'});
o207=new FileReader();
o207.onload=fun0;
o207.readAsBinaryString(o182);
o383=o5.mozGetAsFile('undefined','image/jpeg');
}
function fun0() {
o544=o207.readAsDataURL(o383);
document.write('<html><body><div></div><div></div></body></html>');
o207.onload=fun1;
}
function fun1() {
o136.eval("new RTCPeerConnection();");
location.reload();
}
</script>
<body onload="start()"></body>
|
||
Updated•5 years ago
|
|
|
||
Comment 3•5 years ago
|
||
Any ideas, Byron? It looks like you've worked on PeerConnectionMedia.cpp recently.
|
Assignee | |
Comment 4•5 years ago
|
||
Not sure, maybe a regression from bug 1550540? Maybe we're somehow managing to create a PeerConnectionMedia and start it down the path of doing web proxy lookup stuff, without actually succeeding in initializing the PeerConnectionImpl?
|
||
Comment 5•5 years ago
|
||
Is it possible that we never cancel pending proxy resolution requests when we terminate a PeerConneectionImpl?
|
|
Assignee | |
Comment 6•5 years ago
|
||
|
|
Assignee | |
Comment 7•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29f8c68d5ff92fa0026de212722e1b03cbdd345c
|
|
Assignee | |
Comment 8•5 years ago
|
||
When you get a chance, can you try the attached patch and see if the problem is fixed?
|
|
Assignee | |
Comment 9•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=182a6b822323ebb9a768cd4e39e06df69358c013
|
|
Assignee | |
Comment 10•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=067fd7b8e72544c19f9b742886ddbb43ca1b03a2
|
|
Assignee | |
Comment 11•5 years ago
|
||
Latest patch seems to fix the assertion in comment 1. Not sure what the un-minimized test case will do, but I think the root issue has been addressed.
|
|
Assignee | |
Comment 12•5 years ago
|
||
Comment on attachment 9068502 [details]
Bug 1551836: Ensure that we clean up PeerConnectionMedia properly when PeerConnectionImpl::Initialize fails. r?mjf
Security Approval Request
- How easily could an exploit be constructed based on the patch?: Not sure. Probably not too hard, although the commit comment is a little misleading.
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
- Which older supported branches are affected by this flaw?: 68
- If not all supported branches, which bug introduced the flaw?: Bug 1550540
- Do you have backports for the affected branches?: Yes
- If not, how different, hard to create, and risky will they be?: Patch applies cleanly to 68.
- How likely is this patch to cause regressions; how much testing does it need?: None, probably.
|
||
Updated•5 years ago
|
|
||
Comment 13•5 years ago
|
||
Sec-approval+ for trunk. We'll want a beta nomination for this patch as well so we can avoid shipping the issue.
|
|
||
Comment 14•5 years ago
|
||
Comment on attachment 9068502 [details]
Bug 1551836: Ensure that we clean up PeerConnectionMedia properly when PeerConnectionImpl::Initialize fails. r?mjf
And, duh, there is already a beta request, which I am approving.
|
|
Assignee | |
Comment 15•5 years ago
|
||
Phabricator is not allowing me to land this, claiming I need to provide it with an API token, despite the fact that I am already logged in and can view the revision.
|
||
Comment 16•5 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/d992318dbf93f654a5ad1b2995e38f6b1fc6391d
Phabricator and Lando manage their logins semi-independent from each other. On the Lando page, press the menu button at the top right, then click the settings button. Add the token starting with "api-" from https://phabricator.services.mozilla.com/settings/user/bwc/page/apitokens/
|
|
||
Comment 17•5 years ago
|
||
|
|
||
Updated•5 years ago
|
|
|
||
Comment 18•5 years ago
|
||
| uplift | ||
|
||
Updated•5 years ago
|
|
||
Updated•5 years ago
|
|
||
Comment 19•5 years ago
|
||
I have attempted to reproduce your issue in Nightly68 build from the date (15th of May 2019) of the bug report, build ID: 20190515092556. I did not manage to make the browser crash or see an assertion failure or address sanitizer errors in the debug console while using the test case in comment 0.
Nils, can you reproduce this issue on the affected build? I cannot validate the fix because I cannot reproduce it. Am I using the right affected build? Is there anything else I should do other than open the HTML test case with the affected build to reproduce the issue?
Thank you for your contribution!
|
|
||
Updated•5 years ago
|
|
||
Updated•5 years ago
|
|
|
||
Comment 20•5 years ago
|
||
This is on Release from some time and no new info was added in order to verify this. Remove the qe-verify+ flag.
|
||
Updated•4 years ago
|
|
||
Updated•2 years ago
|
Description
•