1298552 - (CVE-2016-9077) Canvas filters allow feDisplacementMaps to be applied to cross-origin images, allowing timing attacks on them

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Markus Stange [:mstange]]

Attachment: 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.

Comment 1

[Markus Stange [:mstange]]

Attachment: patch

Comment 2

[Markus Stange [:mstange]]

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.

Comment 3

[Markus Stange [:mstange]]

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.

Comment 4

[Markus Stange [:mstange]]

Attachment: patch

Comment 5

[Markus Stange [:mstange]]

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

Comment 6

[Al Billings [:abillings - ex-MoCo]]

This seems like a sec-moderate bug. We need to figure out a rating before it can get sec-approval.

Comment 7

[Daniel Veditz [:dveditz]]

Seems like it should be sec-high like bug 711043

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8785496 [details] [diff] [review]
patch

Agreed, this can wait till 50.

Comment 10

[Daniel Veditz [:dveditz]]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 13

[Daniel Veditz [:dveditz]]

This can be checked in at any time now

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e451d0e6324

Comment 16

[Markus Stange [:mstange]]

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).

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e451d0e6324

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d531a9166e20
https://hg.mozilla.org/releases/mozilla-beta/rev/37a9c564fc95

Comment 19

[Gerry Chang [:gchang]]

Track for 51+ as sec-high regression.

Comment 20

[Marcia Knous [:marcia]]

Tracking 52+ for this sec-high issue.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

It'd be useful if I attached a bug 1318283 that is tracking these crashes.

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

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 :)

Comment 24

[Milan Sreckovic [:milan] (needinfo for best results)]

AdjustedTargetForFilter calls MaxSourceNeededBoundsForFilter which calls NeedToApplyFilter, even when NeedToCalculateBounds doesn't.

Comment 25

[Markus Stange [:mstange]]

Attachment: testcase for that other crash bug

Comment 26

[Markus Stange [:mstange]]

(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.