Closed Bug 1307740 Opened 8 years ago Closed 8 years ago

SecurityError after using canvas.filter

Categories

(Core :: Graphics: Canvas2D, defect)

49 Branch
x86_64
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox50 - wontfix
firefox51 + fixed
firefox52 + fixed

People

(Reporter: mail.eraserhead, Assigned: mstange)

References

Details

(Keywords: testcase)

Attachments

(4 files)

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)
OS: Unspecified → Windows 10
Product: Firefox → Core
Hardware: Unspecified → x86_64
I agree, this sounds like a bug. Can you check whether it still happens in Firefox 50 Beta?
Flags: needinfo?(mstange)
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
Attached file reduced testcase —
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.
Track 51+ as security error.
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 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 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 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 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.
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 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?
Attachment #8802168 - Flags: approval-mozilla-beta?
Attachment #8802168 - Flags: approval-mozilla-aurora?
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)
Sounds reasonable to me as well.
Flags: needinfo?(bugmail)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: