Closed
Bug 1308859
Opened 8 years ago
Closed 8 years ago
Crash in mozilla::dom::CanvasRenderingContext2D::NeedToApplyFilter
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox49 | --- | unaffected |
firefox50 | --- | fixed |
firefox51 | --- | fixed |
firefox52 | --- | fixed |
People
(Reporter: philipp, Assigned: mstange)
References
Details
(4 keywords, Whiteboard: [post-critsmash-triage])
Crash Data
Attachments
(4 files, 1 obsolete file)
5.79 KB,
patch
|
gw280
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
1.39 KB,
text/html
|
Details | |
2.10 KB,
patch
|
jrmuizel
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
5.99 KB,
patch
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-0d27cd58-bb11-418e-89f6-eee5d2161010. ============================================================= Crashing Thread (0) Frame Module Signature Source 0 xul.dll mozilla::dom::CanvasRenderingContext2D::NeedToApplyFilter() dom/canvas/CanvasRenderingContext2D.h:861 1 xul.dll mozilla::dom::CanvasRenderingContext2D::NeedToCalculateBounds() dom/canvas/CanvasRenderingContext2D.h:880 2 xul.dll mozilla::dom::CanvasRenderingContext2D::Fill(mozilla::dom::CanvasWindingRule const&) dom/canvas/CanvasRenderingContext2D.cpp:2962 3 xul.dll mozilla::dom::CanvasRenderingContext2DBinding::fill obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:3316 4 @0x369beccb this signature is regressing in firefox 50 builds and beyond and seems to occur in a codepath changed with (restricted) bug 1298552. on 50.0b5 it's 0.44% of all browser crashes currently.
Flags: needinfo?(mstange)
I'm going to change this to restricted, as that crash looks like UAF, and the original bug we were trying to fix was restricted.
Group: gfx-core-security
Updated•8 years ago
|
Assignee: nobody → mstange
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mstange)
This one is a much higher crash rate than the bug 1298552 that we think is the source of the regression, so we will want to consider backing that out.
Updated•8 years ago
|
Blocks: CVE-2016-9077
Keywords: csectype-uaf,
sec-high
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8802207 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8802207 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8802207 [details] [diff] [review] part 1: add some instrumentation Not sure what the security approval protocol for this case is. This crash is not on a release channel yet, it's on Beta + Aurora + Nightly. And this patch does not fix the bug, it only adds instrumentation. I can't answer the regular security approval questions yet because I don't understand the bug yet.
Attachment #8802207 -
Flags: sec-approval?
Comment 5•8 years ago
|
||
This only needs to be on Nightly though, right? I don't see any reason this would need to be on aurora or beta as well.
Assignee | ||
Comment 6•8 years ago
|
||
Correct. We have two or more crashes per day on Nightly, so having it just on Nightly is enough.
Comment 7•8 years ago
|
||
Comment on attachment 8802207 [details] [diff] [review] part 1: add some instrumentation Check the instrumentation patch into nightly.
Attachment #8802207 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/29f57d48fdf4789847ec80dfeb47535dc643de4c
Comment 9•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/29f57d48fdf4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Status: REOPENED → ASSIGNED
Comment 10•8 years ago
|
||
Crash volume for signature 'mozilla::dom::CanvasRenderingContext2D::NeedToApplyFilter': - nightly (version 52): 26 crashes from 2016-09-19. - aurora (version 51): 65 crashes from 2016-09-19. - beta (version 50): 952 crashes from 2016-09-20. - release (version 49): 1 crash from 2016-09-05. - esr (version 45): 0 crashes from 2016-07-25. Crash volume on the last weeks (Week N is from 10-17 to 10-23): W. N-1 W. N-2 W. N-3 W. N-4 - nightly 11 6 6 0 - aurora 32 12 16 0 - beta 377 330 136 0 - release 1 0 0 0 - esr 0 0 0 0 Affected platform: Windows Crash rank on the last 7 days: Browser Content Plugin - nightly #113 #77 - aurora #211 #45 - beta #41 #49 - release #32752 - esr
Reporter | ||
Comment 11•8 years ago
|
||
these are the new crash reports with the instrumentation patch in place: http://bit.ly/2dCk0Zl
Assignee | ||
Comment 12•8 years ago
|
||
Thanks! All the crashes are in this line: MOZ_RELEASE_ASSERT(filter.mPrimitives.Length() < 1000); Most importantly, this means that the context is alive and well, and that the presshell is available and not being destroyed. So something is writing garbage into filter, or into filter.mPrimitives.
Assignee | ||
Comment 13•8 years ago
|
||
It's possible that the presshell flush caused modifications to mStyleStack, which would make the reference to CurrentState().filter (which we take at the beginning of NeedToApplyFilter()) invalid. I added that reference in bug 1298552 so it would make sense if that's what causes the crashes. I think I even know how to create a testcase that crashes.
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8804362 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8804362 [details] [diff] [review] part 3: fix [Security approval request comment] How easily could an exploit be constructed based on the patch? I don't see a way this could be exploited. It only lets you crash the browser. We only use the garbage pointer once, like this: return garbagePointerToFilterDescription->mPrimitives.Length() > 0; mPrimitives is an nsTArray, which has a pointer to the array's header, which has the length in a field. So all that's happening is that we access a number by following two pointers, and we compare that number to zero. We don't call any virtual methods. After this check, the garbage pointer is not used any more. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? If you know how to make presshell flushes cause synchronous JS execution, then yes. Otherwise no. Which older supported branches are affected by this flaw? No release branches yet. Affected branches are 50, 51, 52. If not all supported branches, which bug introduced the flaw? Bug 1298552. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Straightforward, very low risk. How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions, does not need manual testing.
Attachment #8804362 -
Flags: sec-approval?
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8804361 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8804362 -
Flags: review?(jmuizelaar) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8804362 [details] [diff] [review] part 3: fix Sec-approval+ for trunk. Please nominate for Aurora and Beta ASAP so we can try to avoid shipping this security bug since it only affects 50 and higher.
Attachment #8804362 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 8804362 [details] [diff] [review] part 3: fix Approval Request Comment [Feature/regressing bug #]: bug 1298552 [User impact if declined]: crashes during canvas operations under certain circumstances [Describe test coverage new/current, TreeHerder]: we don't have a test that exercises this particular set of circumstances. I'll add a crash test once we've shipped this fix. [Risks and why]: very low risk, this code change is a strict improvement [String/UUID change made/needed]: none
Attachment #8804362 -
Flags: approval-mozilla-beta?
Attachment #8804362 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a096bd8d900e6f910b2dafa4472aa9c0cd1a2dbd https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c21601825b83ab8f3d85ea93e82e9fd2739d39
Comment 22•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a096bd8d900e https://hg.mozilla.org/mozilla-central/rev/c2c21601825b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment on attachment 8804362 [details] [diff] [review] part 3: fix Sec-high that the sec team wants in 50, Aurora51+, Beta50+
Attachment #8804362 -
Flags: approval-mozilla-beta?
Attachment #8804362 -
Flags: approval-mozilla-beta+
Attachment #8804362 -
Flags: approval-mozilla-aurora?
Attachment #8804362 -
Flags: approval-mozilla-aurora+
Comment 24•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/861f21d94172 https://hg.mozilla.org/releases/mozilla-beta/rev/075f767684d1
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
QA Contact: kjozwiak
Updated•8 years ago
|
Group: gfx-core-security → core-security-release
Reporter | ||
Comment 25•8 years ago
|
||
i think this is now showing up as [@ mozilla::dom::CanvasRenderingContext2D::Fill ] https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3Adom%3A%3ACanvasRenderingContext2D%3A%3AFill
Flags: needinfo?(mstange)
Updated•8 years ago
|
Group: core-security-release
Updated•7 years ago
|
QA Contact: kjozwiak
Assignee | ||
Updated•1 year ago
|
Flags: needinfo?(mstange.moz)
You need to log in
before you can comment on or make changes to this bug.
Description
•