SecurityError after using canvas.filter

RESOLVED FIXED in Firefox 51

Status

()

Core
Canvas: 2D
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: C. Martin, Assigned: mstange)

Tracking

({testcase})

49 Branch
mozilla52
x86_64
Windows 10
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 wontfix, firefox50- wontfix, firefox51+ fixed, firefox52+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
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?
(Reporter)

Comment 1

2 years ago
Just wanted to confirm that this does not happen in 48.0.2 and ESR 45.4 with manually set canvas.filter.enabled = true

Comment 2

2 years ago
Could you provide a testcase, please.
Flags: needinfo?(mail.eraserhead)
Keywords: testcase-wanted
(Reporter)

Comment 3

2 years ago
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.

Comment 4

2 years ago
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

2 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

2 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

Updated

2 years ago
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
(Assignee)

Comment 7

2 years ago
Created attachment 8800419 [details]
reduced testcase
(Assignee)

Comment 8

2 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.
Tracking 52+ since this generates a security error.
tracking-firefox52: ? → +
Track 51+ as security error.
tracking-firefox51: ? → +
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 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

2 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

2 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

2 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

2 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

2 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

2 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
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

2 years ago
Duplicate of this bug: 1311274

Updated

2 years ago
Duplicate of this bug: 1174278
(Assignee)

Comment 26

2 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

2 years ago
Attachment #8802168 - Flags: approval-mozilla-beta?
Attachment #8802168 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 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+

Updated

2 years ago
status-firefox50: affected → wontfix
tracking-firefox50: ? → -

Updated

2 years ago
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)
Sounds reasonable to me as well.
Flags: needinfo?(bugmail)
status-firefox49: affected → wontfix
Blocks: 1294306
You need to log in before you can comment on or make changes to this bug.