Closed
Bug 1307740
Opened 8 years ago
Closed 8 years ago
SecurityError after using canvas.filter
Categories
(Core :: Graphics: Canvas2D, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: mail.eraserhead, Assigned: mstange)
References
Details
(Keywords: testcase)
Attachments
(4 files)
507 bytes,
text/html
|
Details | |
58 bytes,
text/x-review-board-request
|
jwatt
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details |
58 bytes,
text/x-review-board-request
|
tschneider
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details |
58 bytes,
text/x-review-board-request
|
tschneider
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID: 20160922113459 Steps to reproduce: This is kind of tricky and happens only from Firefox 49 on: if you use a (experimental) canvas.filter on a canvas object and then try to further process the canvas you now get a SecurityError. In my case I apply a greyscale filter (canvasContext.filter = 'grayscale(100%)';) and then call a bilinear resize function. Before Firefox 49 this worked just fine but now it only works if I don't apply the greyscale filter at all (or use a grayscale function instead like I do for other browsers). I don't really see how the filter makes the canvas insecure, so I guess this is a bug. But maybe I don't see the whole picture. Actual results: Security error on resize function after applying canvas.filter Expected results: No security error?
Just wanted to confirm that this does not happen in 48.0.2 and ESR 45.4 with manually set canvas.filter.enabled = true
Could you provide a testcase, please.
Flags: needinfo?(mail.eraserhead)
Keywords: testcase-wanted
Sorry, this testcase is a bit more complicated than it has to be: http://kunden.cmcm.info/moz-canvas-bug/ Open console and refresh. The important stuff happens between lines 51 and 56. The bilinear resize function creates a temporary canvas, inserts the data from the given source canvas, does its magic, then writes the new data back to the source canvas. No problems with the first source canvas without filter. But for the second source canvas the grayscale filter is applied and when that filtered canvas is used as source for getImageData in line 56 we get the Security Error. But it works in FF<49 and also in Chrome.
It appears after canvas CSS/SVG filters enabled by default. https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dd6fb8e96ae4b9b2c2150e0515919ca8985b672b&tochange=daf8583252b256d279453f6cbd7e8494f1a811c4 Markus, any thoughts about this error?
Blocks: 1173545
Component: Untriaged → Canvas: 2D
Flags: needinfo?(mail.eraserhead) → needinfo?(mstange)
Keywords: testcase-wanted → testcase
OS: Unspecified → Windows 10
Product: Firefox → Core
Hardware: Unspecified → x86_64
Assignee | ||
Comment 5•8 years ago
|
||
I agree, this sounds like a bug. Can you check whether it still happens in Firefox 50 Beta?
Flags: needinfo?(mstange)
Assignee | ||
Comment 6•8 years ago
|
||
Nevermind, it still happens on Nightly, so it probably wasn't fixed by the patch that I was thinking of.
Assignee: nobody → mstange
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
tracking-firefox50:
--- → ?
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Comment 8•8 years ago
|
||
It looks like this is only implemented properly for SVG filters, and CSS filters always taint the canvas. Sorry I missed that :-( I'm working on a patch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8802167 [details] Bug 1307740 - Properly handle filter input tainting with CSS filters and multiple filters. https://reviewboard.mozilla.org/r/86660/#review85608 ::: layout/svg/nsCSSFilterInstance.h:55 (Diff revision 1) > * Creates at least one new FilterPrimitiveDescription based on the filter > * from the style system. Appends the new FilterPrimitiveDescription(s) to the > * aPrimitiveDescrs list. > */ > - nsresult BuildPrimitives(nsTArray<FilterPrimitiveDescription>& aPrimitiveDescrs); > + nsresult BuildPrimitives(nsTArray<FilterPrimitiveDescription>& aPrimitiveDescrs, > + bool aInputIsTainted); It would be nice to note in the comment what aInputIsTainted means. ::: layout/svg/nsCSSFilterInstance.h:63 (Diff revision 1) > /** > * Returns a new FilterPrimitiveDescription with its basic properties set up. > */ > FilterPrimitiveDescription CreatePrimitiveDescription(PrimitiveType aType, > - const nsTArray<FilterPrimitiveDescription>& aPrimitiveDescrs); > + const nsTArray<FilterPrimitiveDescription>& aPrimitiveDescrs, > + bool aInputIsTainted); Likewise. ::: layout/svg/nsFilterInstance.h:69 (Diff revision 1) > * render the filter (from feImage primitives). > * @return A FilterDescription describing the filter. > */ > static FilterDescription GetFilterDescription(nsIContent* aFilteredElement, > const nsTArray<nsStyleFilter>& aFilterChain, > + bool aFilterInputIsTainted, And here. ::: layout/svg/nsFilterInstance.h:141 (Diff revision 1) > */ > nsFilterInstance(nsIFrame *aTargetFrame, > nsIContent* aTargetContent, > const UserSpaceMetrics& aMetrics, > const nsTArray<nsStyleFilter>& aFilterChain, > + bool aFilterInputIsTainted, And here. ::: layout/svg/nsFilterInstance.h:245 (Diff revision 1) > * filter primitives and their connections. This populates > * mPrimitiveDescriptions and mInputImages. > */ > nsresult BuildPrimitives(const nsTArray<nsStyleFilter>& aFilterChain, > - nsIFrame* aTargetFrame); > + nsIFrame* aTargetFrame, > + bool aFilterInputIsTainted); And here. ::: layout/svg/nsFilterInstance.h:254 (Diff revision 1) > * reference filter or CSS filter. This populates mPrimitiveDescrs and > * mInputImages. > */ > nsresult BuildPrimitivesForFilter(const nsStyleFilter& aFilter, > - nsIFrame* aTargetFrame); > + nsIFrame* aTargetFrame, > + bool aInputIsTainted); And here. ::: layout/svg/nsFilterInstance.cpp:70 (Diff revision 1) > const nsRegion *aDirtyArea) > { > auto& filterChain = aFilteredFrame->StyleEffects()->mFilters; > UniquePtr<UserSpaceMetrics> metrics = UserSpaceMetricsForFrame(aFilteredFrame); > nsFilterInstance instance(aFilteredFrame, aFilteredFrame->GetContent(), *metrics, > - filterChain, aPaintCallback, aTransform, > + filterChain, true, aPaintCallback, aTransform, Can you add an /* InputIsTainted */ here since we're not using an enum? Probably also a separate comment on the line above saying something about why it's hardcoded to true. ::: layout/svg/nsFilterInstance.cpp:90 (Diff revision 1) > > gfxMatrix unused; // aPaintTransform arg not used since we're not painting > auto& filterChain = aFilteredFrame->StyleEffects()->mFilters; > UniquePtr<UserSpaceMetrics> metrics = UserSpaceMetricsForFrame(aFilteredFrame); > nsFilterInstance instance(aFilteredFrame, aFilteredFrame->GetContent(), *metrics, > - filterChain, nullptr, unused, nullptr, > + filterChain, true, nullptr, unused, nullptr, And here. ::: layout/svg/nsFilterInstance.cpp:109 (Diff revision 1) > const nsRegion& aPostFilterDirtyRegion) > { > gfxMatrix unused; // aPaintTransform arg not used since we're not painting > auto& filterChain = aFilteredFrame->StyleEffects()->mFilters; > UniquePtr<UserSpaceMetrics> metrics = UserSpaceMetricsForFrame(aFilteredFrame); > nsFilterInstance instance(aFilteredFrame, aFilteredFrame->GetContent(), *metrics, And here. ::: layout/svg/nsFilterInstance.cpp:141 (Diff revision 1) > > gfxMatrix unused; // aPaintTransform arg not used since we're not painting > auto& filterChain = aFilteredFrame->StyleEffects()->mFilters; > UniquePtr<UserSpaceMetrics> metrics = UserSpaceMetricsForFrame(aFilteredFrame); > nsFilterInstance instance(aFilteredFrame, aFilteredFrame->GetContent(), *metrics, > - filterChain, nullptr, unused, nullptr, > + filterChain, true, nullptr, unused, nullptr, And here. ::: layout/svg/nsSVGFilterInstance.h:104 (Diff revision 1) > * FilterPrimitiveDescription(s) to the aPrimitiveDescrs list. Also, appends > * new images from feImage filter primitive elements to the aInputImages list. > */ > nsresult BuildPrimitives(nsTArray<FilterPrimitiveDescription>& aPrimitiveDescrs, > - nsTArray<RefPtr<SourceSurface>>& aInputImages); > + nsTArray<RefPtr<SourceSurface>>& aInputImages, > + bool aInputIsTainted); Again, update the comment to remark on the aInputIsTainted parameter.
Attachment #8802167 -
Flags: review?(jwatt) → review+
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8802168 [details] Bug 1307740 - Merge filter taintedness tests into one file. https://reviewboard.mozilla.org/r/86662/#review85688 Looking good. Would it make sense to add another tests that uses SourceGraphics as input, appplied on an already tainted canvas (e.g. tainted via a leading drawImage) to make sure applying such filter wouldn't ignore the current canvas state?
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8802169 [details] Bug 1307740 - Add more canvas filter tainting tests. https://reviewboard.mozilla.org/r/86664/#review85690
Attachment #8802169 -
Flags: review?(tschneider) → review+
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8802168 [details] Bug 1307740 - Merge filter taintedness tests into one file. https://reviewboard.mozilla.org/r/86662/#review85692
Attachment #8802168 -
Flags: review?(tschneider) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8802168 [details] Bug 1307740 - Merge filter taintedness tests into one file. https://reviewboard.mozilla.org/r/86662/#review85688 Good idea, I added two tests.
Comment 22•8 years ago
|
||
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/71e3cf762783 Properly handle filter input tainting with CSS filters and multiple filters. r=jwatt https://hg.mozilla.org/integration/autoland/rev/e7331770d28b Merge filter taintedness tests into one file. r=tobytailor https://hg.mozilla.org/integration/autoland/rev/97034d83e30b Add more canvas filter tainting tests. r=tobytailor
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/71e3cf762783 https://hg.mozilla.org/mozilla-central/rev/e7331770d28b https://hg.mozilla.org/mozilla-central/rev/97034d83e30b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8802167 [details] Bug 1307740 - Properly handle filter input tainting with CSS filters and multiple filters. Approval Request Comment [Feature/regressing bug #]: enabling canvas filters, 1173545 [User impact if declined]: Unexpected exceptions when reading from canvases [Describe test coverage new/current, TreeHerder]: the patches in this bug add lots of tests [Risks and why]: low to medium, but the tests make me confident, and I think taking this patch is less risky than shipping canvas filters without this fix [String/UUID change made/needed]: none
Attachment #8802167 -
Flags: approval-mozilla-beta?
Attachment #8802167 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8802168 -
Flags: approval-mozilla-beta?
Attachment #8802168 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Attachment #8802169 -
Flags: approval-mozilla-beta?
Attachment #8802169 -
Flags: approval-mozilla-aurora?
Hi Milan, Kats, I wanted to ping you guys regarding this late beta uplift. This has been a regression since 49 and at this point I don't feel comfortable uplifting this change. We enter RC week next week. We are still trying to fix top crashers and I would prefer not to disrupt quality and stability unless it's a release blocker. Please let me know if there are strong reasons to uplift this.
Flags: needinfo?(milan)
Flags: needinfo?(bugmail)
Comment on attachment 8802167 [details] Bug 1307740 - Properly handle filter input tainting with CSS filters and multiple filters. Let's uplift to Aurora51, given that we lived 6 weeks with this regression release channel, I am ok shipping 50 with it. Uplifting this change (seems pretty big and has low-medium risk) could disrupt 50 stability/quality. Let this change ride the 51 train.
Attachment #8802167 -
Flags: approval-mozilla-beta?
Attachment #8802167 -
Flags: approval-mozilla-beta-
Attachment #8802167 -
Flags: approval-mozilla-aurora?
Attachment #8802167 -
Flags: approval-mozilla-aurora+
Attachment #8802168 -
Flags: approval-mozilla-beta?
Attachment #8802168 -
Flags: approval-mozilla-beta-
Attachment #8802168 -
Flags: approval-mozilla-aurora?
Attachment #8802168 -
Flags: approval-mozilla-aurora+
Comment on attachment 8802169 [details] Bug 1307740 - Add more canvas filter tainting tests. (For future, test only changes don't require relman review)
Attachment #8802169 -
Flags: approval-mozilla-beta?
Attachment #8802169 -
Flags: approval-mozilla-beta-
Attachment #8802169 -
Flags: approval-mozilla-aurora?
Attachment #8802169 -
Flags: approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #28) > Comment on attachment 8802167 [details] > Bug 1307740 - Properly handle filter input tainting with CSS filters and > multiple filters. > > Let's uplift to Aurora51, given that we lived 6 weeks with this regression > release channel, I am ok shipping 50 with it. Uplifting this change (seems > pretty big and has low-medium risk) could disrupt 50 stability/quality. Let > this change ride the 51 train. OK.
Flags: needinfo?(milan)
Comment 31•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/09a44149a839 https://hg.mozilla.org/releases/mozilla-aurora/rev/05dbe33c8ef2 https://hg.mozilla.org/releases/mozilla-aurora/rev/74f8462e83d4
Flags: in-testsuite+
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•