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

RESOLVED FIXED in Firefox 50

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: philipp, Assigned: mstange, NeedInfo)

Tracking

(4 keywords)

50 Branch
mozilla52
crash, csectype-uaf, regression, sec-high
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox49 unaffected, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [post-critsmash-triage], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
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
(Assignee)

Updated

a year 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.
Blocks: 1298552
Keywords: csectype-uaf, sec-high
(Assignee)

Comment 3

a year ago
Created attachment 8802207 [details] [diff] [review]
part 1: add some instrumentation
Attachment #8802207 - Flags: review?(jmuizelaar)
Attachment #8802207 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 4

a year 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?
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

a year ago
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+
https://hg.mozilla.org/mozilla-central/rev/29f57d48fdf4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

a year ago
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
(Assignee)

Updated

a year ago
Status: REOPENED → ASSIGNED
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
status-firefox49: unaffected → affected
(Reporter)

Comment 11

a year ago
these are the new crash reports with the instrumentation patch in place: http://bit.ly/2dCk0Zl
(Assignee)

Comment 12

a year 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

a year 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

a year ago
Created attachment 8804146 [details]
testcase
(Assignee)

Comment 15

a year ago
Created attachment 8804361 [details] [diff] [review]
part 2: remove instrumentation again
(Assignee)

Comment 16

a year ago
Created attachment 8804362 [details] [diff] [review]
part 3: fix
Attachment #8804362 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
status-firefox49: affected → unaffected
status-firefox52: fixed → affected
(Assignee)

Comment 17

a year 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

a year ago
Created attachment 8804377 [details] [diff] [review]
part 2: remove instrumentation again
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+
(Assignee)

Comment 20

a year 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?
https://hg.mozilla.org/mozilla-central/rev/a096bd8d900e
https://hg.mozilla.org/mozilla-central/rev/c2c21601825b
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox52: affected → fixed
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/861f21d94172
https://hg.mozilla.org/releases/mozilla-beta/rev/075f767684d1
status-firefox50: affected → fixed
status-firefox51: affected → fixed
Keywords: leave-open
Whiteboard: [post-critsmash-triage]
QA Contact: kjozwiak
Group: gfx-core-security → core-security-release
(Reporter)

Comment 25

a year 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)
Group: core-security-release

Comment 26

11 months ago
We should land the testcase as a crashtest.
Flags: in-testsuite?

Updated

11 months ago
QA Contact: kjozwiak
You need to log in before you can comment on or make changes to this bug.