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.
Created attachment 8785487 [details] [diff] [review] patch
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 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.
Created attachment 8785496 [details] [diff] [review] patch
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
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
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 on attachment 8785496 [details] [diff] [review] patch Agreed, this can wait till 50.
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 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 :)
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?
This can be checked in at any time now
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?
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).
Track for 51+ as sec-high regression.
Tracking 52+ for this sec-high issue.
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.
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.
(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.