Closed Bug 1308859 Opened 3 years ago Closed 3 years ago

Crash in mozilla::dom::CanvasRenderingContext2D::NeedToApplyFilter


(Core :: Graphics, defect, critical)

50 Branch
Not set



Tracking Status
firefox49 --- unaffected
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed


(Reporter: philipp, Assigned: mstange, NeedInfo)



(4 keywords, Whiteboard: [post-critsmash-triage])

Crash Data


(4 files, 1 obsolete file)

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
Assignee: nobody → mstange
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.
Attachment #8802207 - Flags: review?(jmuizelaar)
Attachment #8802207 - Flags: review?(jmuizelaar) → review+
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?
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.
Correct. We have two or more crashes per day on Nightly, so having it just on Nightly is enough.
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+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Keywords: leave-open
Resolution: FIXED → ---
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
these are the new crash reports with the instrumentation patch in place:

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.
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.
Attached file testcase
Attached patch part 3: fixSplinter Review
Attachment #8804362 - Flags: review?(jmuizelaar)
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?
Attachment #8804361 - Attachment is obsolete: true
Attachment #8804362 - Flags: review?(jmuizelaar) → review+
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+
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?
Closed: 3 years ago3 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+
Whiteboard: [post-critsmash-triage]
QA Contact: kjozwiak
Group: gfx-core-security → core-security-release
i think this is now showing up as [@ mozilla::dom::CanvasRenderingContext2D::Fill ]
Flags: needinfo?(mstange)
Group: core-security-release
We should land the testcase as a crashtest.
Flags: in-testsuite?
QA Contact: kjozwiak
You need to log in before you can comment on or make changes to this bug.