If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.
Bug 1298552 (CVE-2016-9077)

Canvas filters allow feDisplacementMaps to be applied to cross-origin images, allowing timing attacks on them

RESOLVED FIXED in Firefox 50

Status

()

Core
Canvas: 2D
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

({csectype-disclosure, regression, sec-high})

49 Branch
mozilla52
csectype-disclosure, regression, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox48 unaffected, firefox49 wontfix, firefox-esr45 unaffected, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed)

Details

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

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Created attachment 8785481 [details]
testcase

See the last paragraph of bug 1287316 comment 29 - updateFilterOnWriteOnly is only checked on the first draw after the filter was set. If the canvas's IsWriteOnly state changes after that first draw, we don't update the filter, and happily apply feDisplacementMap filters.
This is bad because feDisplacementMap filter rendering can take different amounts of time depending on the input pixel, and the input pixel can be a cross-origin image drawn through drawImage.

In the testcase, the stuff in the top left corner should be a black square, not a black funny looking  blob.

Chrome also has this bug.
(Assignee)

Comment 1

a year ago
Created attachment 8785487 [details] [diff] [review]
patch
Assignee: nobody → mstange
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
Comment on attachment 8785487 [details] [diff] [review]
patch

r=jrmuizel

Jeff reviewed this in person but had to leave before he could set the r+ state.
Attachment #8785487 - Flags: review+
Group: core-security
(Assignee)

Comment 3

a year ago
Comment on attachment 8785487 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
With medium difficulty

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really

Which older supported branches are affected by this flaw?
Beta 49, Aurora 50

If not all supported branches, which bug introduced the flaw?
Enabling canvas filters by default, bug 1173545

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The code hasn't changed since 49, so trivial.

How likely is this patch to cause regressions; how much testing does it need?
Not likely - canvas filters are a new feature that will be released with 49.
Attachment #8785487 - Flags: sec-approval?
(Assignee)

Comment 4

a year ago
Created attachment 8785496 [details] [diff] [review]
patch
Attachment #8785487 - Attachment is obsolete: true
Attachment #8785487 - Flags: sec-approval?
Attachment #8785496 - Flags: sec-approval?
Attachment #8785496 - Flags: review+
(Assignee)

Comment 5

a year ago
Comment on attachment 8785496 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: enabling canvas filters by default, bug 1173545
[User impact if declined]: cross-origin images can be read through timing attacks with a lot of work (Chrome has the same bug)
[Describe test coverage new/current, TreeHerder]: canvas filters have reasonable test coverage but there is no test for this particular bug, and this bug doesn't add one
[Risks and why]: low, the patch is straightforward
[String/UUID change made/needed]: none
Attachment #8785496 - Flags: approval-mozilla-beta?
Attachment #8785496 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

a year ago
status-firefox48: --- → unaffected
status-firefox49: --- → affected
status-firefox50: --- → affected
Keywords: regression
This seems like a sec-moderate bug. We need to figure out a rating before it can get sec-approval.
Seems like it should be sec-high like bug 711043
Keywords: csectype-disclosure, sec-high
(Assignee)

Updated

a year ago
Attachment #8785496 - Flags: sec-approval?
(Assignee)

Updated

a year ago
Attachment #8785496 - Flags: sec-approval?
This isn't going to go into 49. It has been too late for a week or two really. I'm giving sec-approval+ for check on September 20, two weeks into the next cycle.
status-firefox49: affected → wontfix
Whiteboard: [checkin on 9/20]
Attachment #8785496 - Flags: sec-approval? → sec-approval+
Comment on attachment 8785496 [details] [diff] [review]
patch

Agreed, this can wait till 50.
Attachment #8785496 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Blocks: 1173545
Group: core-security → gfx-core-security
status-firefox-esr45: --- → affected
The wallpaper fix in bug 1297316 moved the crash to a MOZ_RELEASE_ASSERT() added in
[@ mozilla::dom::AdjustedTargetForFilter::~AdjustedTargetForFilter ]. According to Markus this patch will fix that, but we should test once this is landed.
Crash Signature: [@ mozilla::dom::AdjustedTargetForFilter::~AdjustedTargetForFilter ]
Version: Trunk → 49 Branch
Comment on attachment 8785496 [details] [diff] [review]
patch

50 moved to Beta today. I can't reset the nomination on this, but it'll need to be :)
Flags: needinfo?(lhenry)
Flags: needinfo?(lhenry)
Attachment #8785496 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
Hi Al, Dan, given that Fx49 release slipped by a week, do we need to move the checkin by 9/20 to 9/27 on this one?
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
This can be checked in at any time now
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Whiteboard: [checkin on 9/20]
Comment on attachment 8785496 [details] [diff] [review]
patch

Sec-high, approved for uplift to Aurora51, Beta50. 

Hi Markus, should we uplift to esr45 as well?
Flags: needinfo?(mstange)
Attachment #8785496 - Flags: approval-mozilla-beta?
Attachment #8785496 - Flags: approval-mozilla-beta+
Attachment #8785496 - Flags: approval-mozilla-aurora?
Attachment #8785496 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e451d0e6324
status-firefox52: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox-esr45: --- → ?
Flags: in-testsuite?
(Assignee)

Comment 16

a year ago
es45 is unaffected. This is a regression from bug 1174278 which only landed on 49, and the code is only used when canvas filters are enabled, which also only happened in 49 (bug 1173545).
status-firefox-esr45: affected → unaffected
tracking-firefox-esr45: ? → ---
Flags: needinfo?(mstange)
https://hg.mozilla.org/mozilla-central/rev/4e451d0e6324
status-firefox52: affected → fixed
Target Milestone: --- → mozilla52
https://hg.mozilla.org/releases/mozilla-aurora/rev/d531a9166e20
https://hg.mozilla.org/releases/mozilla-beta/rev/37a9c564fc95
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
status-firefox51: affected → fixed
Resolution: --- → FIXED
Track for 51+ as sec-high regression.
tracking-firefox51: ? → +
Group: gfx-core-security → core-security-release

Updated

a year ago
tracking-firefox50: ? → +
Tracking 52+ for this sec-high issue.
tracking-firefox52: ? → +
Depends on: 1308859
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Updated

11 months ago
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+]

Updated

11 months ago
Alias: CVE-2016-9077
As of the patch on this bug, CanvasRenderContext2D::NeedToCalculateBounds, called before most of the canvas operations (Fill*, Stroke*, DrawImage, etc.), can end up calling UpdateFilter, which can invalidate the draw target.  ::DrawImage on its own is enough to make it a top crasher on beta.

NeedToCalculateBounds checks NeedToDrawShadow (no side effects) and NeedToApplyFilter (side effects as of this bug.)

NeedToApplyFilter is called in a few other places, and it appears that in at least some of them, the draw target disappearing would be an unexpected (and bad) thing.

So, the question is - is the right solution to not have NeedToApplyFilter call UpdateFilter, or is the right thing to call EnsureTarget after/as a part of NeedToApplyFilter?  Or something completely different?

Markus, thoughts?  We really shouldn't ship this in 51, if we can at all help it.
Flags: needinfo?(mstange)
It'd be useful if I attached a bug 1318283 that is tracking these crashes.
Another potential issue; NeedToCalculateBounds() only calls NeedToApplyFilter() (with its side effects of updating filters) if NeedToDrawShadow() returns false.  Does that mean that we still have the original bug when there is a shadow involved?

(Randomly and unreproducibly, I had this crash when I was testing with a slight modification of the above testcase - https://crash-stats.mozilla.com/report/index/1fafea4d-132e-4d36-bf1e-945052161230, though I can't quite recall what the modification was :)
AdjustedTargetForFilter calls MaxSourceNeededBoundsForFilter which calls NeedToApplyFilter, even when NeedToCalculateBounds doesn't.
(Assignee)

Comment 25

9 months ago
Created attachment 8822771 [details]
testcase for that other crash bug
Flags: needinfo?(mstange)
(Assignee)

Comment 26

9 months ago
(In reply to Milan Sreckovic [:milan] from comment #21)
> So, the question is - is the right solution to not have NeedToApplyFilter
> call UpdateFilter

I don't think so. If we know that the cached filter is out of date, we need to update it at some point before drawing. We could conceivably do that in each drawing function before we call EnsureTarget, but NeedToApplyFilter feels like a better location to trigger the update.

> or is the right thing to call EnsureTarget after/as a
> part of NeedToApplyFilter?

I think that's a fine solution.

> Or something completely different?

Maybe UpdateFilter should not have the presshell flush in the general case. The flush is needed so that url(#filter) references to SVG filters work, but I think this flush only has to be done when the filter is changed through JS setting the ctx.filter property. If the filter is invalidated by something else, we might not need the flush. So we could potentially move the flush to SetFilter. I don't know if that would help, I'd need to look at it more closely.
Depends on: 1318283
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.