Closed
Bug 1230483
Opened 8 years ago
Closed 8 years ago
crash in mozilla::MediaDecoder::NotifySuspendedStatusChanged
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
VERIFIED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox42 | --- | wontfix |
firefox43 | --- | wontfix |
firefox44 | + | verified |
firefox45 | + | verified |
firefox46 | + | verified |
firefox-esr38 | 44+ | verified |
firefox-esr45 | --- | fixed |
b2g-v2.0 | --- | wontfix |
b2g-v2.0M | --- | wontfix |
b2g-v2.1 | --- | wontfix |
b2g-v2.1S | --- | wontfix |
b2g-v2.2 | --- | affected |
b2g-v2.5 | --- | affected |
b2g-v2.2r | --- | affected |
b2g-master | --- | fixed |
People
(Reporter: bc, Assigned: jwwang)
References
()
Details
(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [adv-main44+][adv-esr38.6+])
Crash Data
Attachments
(5 files, 2 obsolete files)
161.78 KB,
text/plain
|
Details | |
4.80 KB,
text/plain
|
Details | |
20.07 KB,
application/x-bzip2
|
Details | |
1.14 KB,
patch
|
jwwang
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.69 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-e2e0677b-ff59-4f29-af04-3f6fb2151204. ============================================================= s-s since this is rated exploitability high. This is not that reproducible when testing manually. I did it once above with released Firefox 42 by reloading several times and crashing on shutdown. You can try: 1. http://interactive.aljazeera.com/aje/2014/piratefishingdoc/#stage1 2. reload x times 3. shutdown Found in bughunter on http://interactive.aljazeera.com/aje/2014/piratefishingdoc/#stage1 on Beta/43, Aurora/44, Nightly/45 Windows *only* both x86 and x86_64 as a different media encoder related crash [@ nsURIHashKey::HashKey(nsIURI const *)
Reporter | ||
Comment 1•8 years ago
|
||
Reporter | ||
Comment 2•8 years ago
|
||
Beta 43 bp-f606512a-243b-410a-a3f5-dd6b62151204 [@ mozalloc_abort | NS_DebugBreak | mozilla::net::PNeckoChild::SendPHttpChannelConstructor ] See bug 1152087 crash in mozalloc_abort(char const*) | NS_DebugBreak | mozilla::net::PNeckoChild::SendPHttpChannelConstructor Nightly 45 bp-cb77d306-009d-438b-8217-b77552151204 also [@ mozilla::MediaDecoder::NotifySuspendedStatusChanged ] and exploitability high ditto bp-ccaa3eb5-8d3f-4cc8-b1dd-2ddf32151204 1. Load http://interactive.aljazeera.com/aje/2014/piratefishingdoc/#stage1 2. Use Developer Tools web console to open a new window 3. In new window, Use Developer web console to execute setInterval('opener.document.location.reload()', 20000) 4. Grab a coffee and wait.
Crash Signature: [@ mozilla::MediaDecoder::NotifySuspendedStatusChanged]
[@ nsURIHashKey::HashKey(nsIURI const *) → [@ mozilla::MediaDecoder::NotifySuspendedStatusChanged]
[@ nsURIHashKey::HashKey(nsIURI const *)]
[@ mozalloc_abort | NS_DebugBreak | mozilla::net::PNeckoChild::SendPHttpChannelConstructor ]
Comment 3•8 years ago
|
||
So far, I've reproduced this as far back as Fx36, though the signature has changed. https://crash-stats.mozilla.com/report/index/1cfab399-48f2-4f5f-addc-632c12151218
Comment 4•8 years ago
|
||
The big difference I can see in the good vs. bad case is that playback continues on after refreshing on bad builds and stops immediately after hitting reload on good builds. Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2360c3c46aca&tochange=bc108d2ce8d1 Too old to bisect on inbound. Bug 804387 looks highly-likely, however. Would be good if we could get a sec rating on this too.
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1:
--- → wontfix
status-b2g-v2.1S:
--- → wontfix
status-b2g-v2.2:
--- → affected
status-b2g-v2.2r:
--- → affected
status-b2g-v2.5:
--- → affected
status-b2g-master:
--- → affected
status-firefox42:
--- → wontfix
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox-esr38:
--- → affected
status-firefox-esr45:
--- → affected
tracking-firefox44:
--- → ?
tracking-firefox45:
--- → ?
tracking-firefox46:
--- → ?
tracking-firefox-esr38:
--- → ?
Flags: needinfo?(roc)
OS: Windows NT → All
Comment 5•8 years ago
|
||
The crash value in the crash in comment 3 is 0x5a5a5a62, which indicates a use-after-free. The crash in the newer signatures is 0xffffffffe5e5e625, which has enough of the new poison value 0xe5 that I'm going to say this is still some kind of use-after-free. An ASan or Valgrind run of this test case would give more information.
Keywords: csectype-uaf,
sec-critical
Reporter | ||
Comment 6•8 years ago
|
||
Unfortunately retesting on bughunter which includes asan opt on Linux x86_64 only shows the nsURIHashKey::HashKey crashes. Testing manually on Windows x86_64 did give me bp-1a7c86b6-8eba-4e6e-be44-7e0482151218 [@ mozilla::MediaDecoder::NotifySuspendedStatusChanged ] with an exploitablity rating of low along with bp-44ae699d-58a1-412f-96e2-df52f2151218 [@ RuleHash::EnumerateAllRules ]. I'll try locally with valgrind if no one beats me to it.
Reporter | ||
Comment 7•8 years ago
|
||
2 each of these errors; ==14849== Invalid read of size 8 ==14849== at 0x7C89438: mozilla::MediaDecoder::NotifySuspendedStatusChanged() (dom/media/MediaDecoder.cpp:1123) ==14849== Invalid read of size 1 ==14849== at 0x7B84BA4: operator= (firefox-debug/dom/html/../../dist/include/mozilla/StateWatching.h:149)
Assignee | ||
Comment 8•8 years ago
|
||
[Child 26974] ###!!! ASSERTION: Shouldn't have a decoder: 'mDecoder == nullptr', file /home/jwwang/codebase/mozilla-central/dom/html/HTMLMediaElement.cpp, line 2808 Hit this assertion in debug build while trying to repro comment 2. This assertion means we have a dangling MediaDecoder that will never be shut down by the media element and it is no surprise we crash in MediaDecoder::NotifySuspendedStatusChanged() since MediaDecoder::mOwner becomes invalid after the media element is deleted.
Assignee: nobody → jwwang
Flags: needinfo?(roc)
Tracked for FF44 and FF45 as it's sec-critical.
Assignee | ||
Comment 10•8 years ago
|
||
add a test case to repro the crash: https://treeherder.mozilla.org/logviewer.html#?job_id=14774017&repo=try try with fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0d544308ad2
Assignee | ||
Comment 11•8 years ago
|
||
Part 1 - add a test case to repro the assertion in comment 8.
Attachment #8700571 -
Flags: review?(cpearce)
Assignee | ||
Comment 12•8 years ago
|
||
Part 2 - LoadFromSourceChildren() should be queued at most once in an event cycle.
Attachment #8700574 -
Flags: review?(cpearce)
Assignee | ||
Comment 13•8 years ago
|
||
Looks like Chris is on PTO now.
Assignee | ||
Updated•8 years ago
|
Attachment #8700571 -
Flags: review?(cpearce) → review?(roc)
Assignee | ||
Updated•8 years ago
|
Attachment #8700574 -
Flags: review?(cpearce) → review?(roc)
Attachment #8700571 -
Flags: review?(roc) → review+
Attachment #8700574 -
Flags: review?(roc) → review+
Assignee | ||
Comment 14•8 years ago
|
||
Thanks for the review!
Assignee | ||
Comment 15•8 years ago
|
||
rebase.
Attachment #8700574 -
Attachment is obsolete: true
Attachment #8701701 -
Flags: review+
Assignee | ||
Comment 16•8 years ago
|
||
rebase.
Attachment #8700571 -
Attachment is obsolete: true
Attachment #8701703 -
Flags: review+
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8701701 [details] [diff] [review] 1230483_part2_fix_NotifyAddedSource_rev2.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Can be easily reproduced by repeatedly refreshing the page. 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 flawed code has been there long ago. 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? Yes. How likely is this patch to cause regressions; how much testing does it need? Very unlikely. The fix is simple and we attach a test case to verify the fix.
Attachment #8701701 -
Flags: sec-approval?
Comment 18•8 years ago
|
||
Comment on attachment 8701701 [details] [diff] [review] 1230483_part2_fix_NotifyAddedSource_rev2.patch sec-approval+ for trunk. Once it has landed, we'll want Aurora, Beta, and ESR38 patches made and nominated as well. Please don't check in the test until we ship a release that fixes the security problem to avoid 0day'ing ourselves.
Attachment #8701701 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to Al Billings [:abillings] from comment #18) > Comment on attachment 8701701 [details] [diff] [review] > 1230483_part2_fix_NotifyAddedSource_rev2.patch > > sec-approval+ for trunk. > Once it has landed, we'll want Aurora, Beta, and ESR38 patches made and > nominated as well. > > Please don't check in the test until we ship a release that fixes the > security problem to avoid 0day'ing ourselves. Sure. That's why I didn't ask for approval for the test case immediately. Do I need to put "check-in needed" and sheriff will do the rest?
Assignee | ||
Comment 21•8 years ago
|
||
I see. Thanks.
Comment 22•8 years ago
|
||
landed https://hg.mozilla.org/integration/mozilla-inbound/rev/3941a5040b06 (part2) - not the tests of course
Comment 23•8 years ago
|
||
JW, could you fill the uplift request to aurora & beta? thanks
Flags: needinfo?(jwwang)
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8701701 [details] [diff] [review] 1230483_part2_fix_NotifyAddedSource_rev2.patch Approval Request Comment [Feature/regressing bug #]:unknown [User impact if declined]:crash might be experienced when refreshing the page [Describe test coverage new/current, TreeHerder]:a new test case is added [Risks and why]: low. the change is simple and we add a test to repro the crash [String/UUID change made/needed]:none
Flags: needinfo?(jwwang)
Attachment #8701701 -
Flags: approval-mozilla-beta?
Attachment #8701701 -
Flags: approval-mozilla-aurora?
Comment 25•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3941a5040b06
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed → leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 27•8 years ago
|
||
Comment on attachment 8701701 [details] [diff] [review] 1230483_part2_fix_NotifyAddedSource_rev2.patch Taking it. Should be in 44 beta 6.
Attachment #8701701 -
Flags: approval-mozilla-beta?
Attachment #8701701 -
Flags: approval-mozilla-beta+
Attachment #8701701 -
Flags: approval-mozilla-aurora?
Attachment #8701701 -
Flags: approval-mozilla-aurora+
Comment 28•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f8711a3f2cf https://hg.mozilla.org/releases/mozilla-beta/rev/476594dd7053
Updated•8 years ago
|
Group: core-security → core-security-release
Comment 29•8 years ago
|
||
Reproduced the initial crash using old build 42 beta 9 on Windows 7 64bit crash: https://crash-stats.mozilla.com/report/index/f9616c54-a5a7-4b75-8c86-44b842160114 Verified that the crash did not occur on Firefox 44 beta 8, 45.0a2 and 46.0a1 across platforms (Windows 7 64, Mac OS X 10.10.5 and Ubuntu 14.04 64).
Status: RESOLVED → VERIFIED
Comment 30•8 years ago
|
||
Do you know why we didn't take this in ESR38? It is a sec-critical and I mentioned needing an ESR38 patch in my sec-approval.
Flags: needinfo?(jwwang)
Whiteboard: [adv-main44+]
Updated•8 years ago
|
Flags: needinfo?(lhenry)
Comment 31•8 years ago
|
||
Yes, this should land for 38.6.0esr. JW, can you give us a patch that will apply correctly for esr38 and request uplift? Thanks.
Flags: needinfo?(lhenry)
Assignee | ||
Comment 32•8 years ago
|
||
I can apply 1230483_part2_fix_NotifyAddedSource_rev2.patch correctly to esr38 (https://hg.mozilla.org/releases/mozilla-esr38). Did you get any errors?
Flags: needinfo?(jwwang)
Comment 33•8 years ago
|
||
We're not developers so we generally can't check the patches for you.
Keywords: checkin-needed
Comment 34•8 years ago
|
||
Comment on attachment 8701701 [details] [diff] [review] 1230483_part2_fix_NotifyAddedSource_rev2.patch Approved for uplift to esr38.
Attachment #8701701 -
Flags: approval-mozilla-esr38+
Updated•8 years ago
|
Whiteboard: [adv-main44+] → [adv-main44+][adv-esr38.6+]
Updated•8 years ago
|
Keywords: checkin-needed
Comment 36•8 years ago
|
||
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29) > Reproduced the initial crash using old build 42 beta 9 on Windows 7 64bit > crash: > https://crash-stats.mozilla.com/report/index/f9616c54-a5a7-4b75-8c86- > 44b842160114 > Verified that the crash did not occur on Firefox 44 beta 8, 45.0a2 and > 46.0a1 across platforms (Windows 7 64, Mac OS X 10.10.5 and Ubuntu 14.04 64). Also verified fixed on Firefox 38.6.0esr across platforms (Windows 10 32-bit, Windows 8.1 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit).
Updated•8 years ago
|
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
•