Make filter tainting and canvas tainting interact properly

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mstange, Assigned: tschneider)

Tracking

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, relnote-firefox -)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8621764 [details] [diff] [review]
old wip, needs tests

- Drawing something to a canvas with a filter that contains a cross-origin feImage should taint the canvas.
- Calling drawImage with a cross-origin image should treat any feDisplacementMap filter primitives as pass-through if their map input is based on the image.
(Assignee)

Comment 1

3 years ago
Created attachment 8738899 [details] [diff] [review]
Part 1: Make filter tainting and canvas tainting interact properly

Rebased old wip patch.
Attachment #8621764 - Attachment is obsolete: true
Attachment #8738899 - Flags: review?(mstange)
(Assignee)

Comment 2

3 years ago
Created attachment 8738900 [details] [diff] [review]
Part 2: Tests

Tests.
Attachment #8738900 - Flags: review?(mstange)
(Assignee)

Comment 3

3 years ago
Created attachment 8738901 [details] [diff] [review]
Part 2: Tests

Sorry, uploaded the wrong patch earlier.
Attachment #8738900 - Attachment is obsolete: true
Attachment #8738900 - Flags: review?(mstange)
Attachment #8738901 - Flags: review?(mstange)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8738901 [details] [diff] [review]
Part 2: Tests

Review of attachment 8738901 [details] [diff] [review]:
-----------------------------------------------------------------

Can you also add a reftest that checks that we render successfully, even if cross-origin images are used? And a reftest that tests that displacement maps with a cross-origin image as the map input don't render?
(Reporter)

Comment 5

3 years ago
Oh, and also two tests that draw a cross-origin image on the canvas using drawImage (which makes the canvas read-only), while a filter is applied that makes use of the SourceImage. And in one of them, use the SourceImage as the map input to a displacement map, and make sure that that one does not render.
(Reporter)

Comment 6

3 years ago
Comment on attachment 8738899 [details] [diff] [review]
Part 1: Make filter tainting and canvas tainting interact properly

I wrote this patch, so I shouldn't be the one who reviews it.
Attachment #8738899 - Flags: review?(mstange) → review?(jmuizelaar)
(Assignee)

Comment 7

3 years ago
> Can you also add a reftest that checks that we render successfully, even if cross-origin images are used?

Will do.

> And a reftest that tests that displacement maps with a cross-origin image as the map input don't render?

Isn't that what test_filter_tainted2.html from my patch is doing?
(Assignee)

Comment 8

3 years ago
> Can you also add a reftest that checks that we render successfully, even if cross-origin images are used?

Ho would I create such a test if can't read pixels to check for their values?

> Oh, and also two tests that draw a cross-origin image on the canvas using drawImage (which makes the 
> canvas read-only), while a filter is applied that makes use of the SourceImage. And in one of them, use 
> the SourceImage as the map input to a displacement map, and make sure that that one does not render.

Would that actually make sense? I we draw a cross-origin image, the Canvas would already be tainted when trying to draw something with such filter applied. Ho would we test if it gets tainted as the result of it then?
Flags: needinfo?(mstange)
(Reporter)

Comment 9

3 years ago
(In reply to Tobias Schneider [:tobytailor] from comment #8)
> > Can you also add a reftest that checks that we render successfully, even if cross-origin images are used?
> 
> Ho would I create such a test if can't read pixels to check for their values?

Using a reftest. You compare a page that has the tainted canvas with a page that has a canvas that you render the expected stuff to.

> > Oh, and also two tests that draw a cross-origin image on the canvas using drawImage (which makes the 
> > canvas read-only), while a filter is applied that makes use of the SourceImage. And in one of them, use 
> > the SourceImage as the map input to a displacement map, and make sure that that one does not render.
> 
> Would that actually make sense? I we draw a cross-origin image, the Canvas
> would already be tainted when trying to draw something with such filter
> applied. Ho would we test if it gets tainted as the result of it then?

I'm confused - there are two different things here: A canvas becoming tainted, and draw call not rendering. I'm suggesting you test for the latter; again, using a reftest.
I'm not sure if the current patch handles this case correctly. Or actually, let's make sure - do you agree that what I'm suggesting would be the correct behavior?
Flags: needinfo?(mstange)
In talking with mstange he also recognized that it's worth testing having a tainted canvas as source in addition to a cross-origin image.
Comment on attachment 8738899 [details] [diff] [review]
Part 1: Make filter tainting and canvas tainting interact properly

Review of attachment 8738899 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +408,5 @@
>        DrawOptions(1.0f, mCompositionOp));
> +
> +    const gfx::FilterDescription& filter = mCtx->CurrentState().filter;
> +    MOZ_ASSERT(!filter.mPrimitives.IsEmpty());
> +    if (filter.mPrimitives.LastElement().IsTainted() && mCtx->mCanvasElement) {

It might be nice if FilterDescription had a IsTainted method.

::: layout/svg/nsSVGFilterInstance.cpp
@@ +366,5 @@
> +  // When the filter is applied during canvas drawing, we might be allowed to
> +  // read from the canvas.
> +  if (HTMLCanvasElement* canvas =
> +        HTMLCanvasElement::FromContentOrNull(aElement)) {
> +    return canvas->IsWriteOnly();

Can you find a way to avoid having to ensure that WriteOnly is set before this function is called? This approach feels really fragile to me.
Attachment #8738899 - Flags: review?(jmuizelaar) → review-
(Reporter)

Comment 12

3 years ago
Yeah, I remember that I wasn't happy with that part of the patch. In IsFilterInputTainted, we really want to know whether *what the current draw call is drawing* is tainted. I didn't see a good way of doing that when I worked on the patch. Tobias, maybe you can find a way of fixing this without relying on the fact that the call to SetWriteOnly occurs before the call to IsFilterInputTainted.
(Reporter)

Updated

3 years ago
Attachment #8738901 - Flags: review?(mstange)
(Assignee)

Comment 13

3 years ago
Created attachment 8746234 [details] [diff] [review]
Part 2: Tests (v2)

Added suggested tests.
Attachment #8738901 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
(In reply to Markus Stange [:mstange] from comment #12)
> Yeah, I remember that I wasn't happy with that part of the patch. In
> IsFilterInputTainted, we really want to know whether *what the current draw
> call is drawing* is tainted. I didn't see a good way of doing that when I
> worked on the patch. Tobias, maybe you can find a way of fixing this without
> relying on the fact that the call to SetWriteOnly occurs before the call to
> IsFilterInputTainted.

After experimenting more with this, I don't see what would be a cleaner solution. We don't check IsWriteOnly every time we draw, since we only call IsFilterInputTainted when setting the filter. And in that case IsWriteOnly seems to be ok. What we need to make sure of tho, is updating the filter descriptor to run IsFilterInputTainted again after a drawing call tainted a Canvas used as SourceGraphics.
(Assignee)

Comment 15

3 years ago
Created attachment 8749809 [details] [diff] [review]
Part 1 (v2): Make filter tainting and canvas tainting interact properly

Makes failing test pass by making sure the filter description is updated after the Canvas got tainted. Jeff, what do you think about this change and Comment 14?
Attachment #8738899 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Created attachment 8749813 [details] [diff] [review]
Part 1 (v2): Make filter tainting and canvas tainting interact properly

Sorry, uploaded the wrong patch.
Attachment #8749809 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Flags: needinfo?(jmuizelaar)
(In reply to Tobias Schneider [:tobytailor] from comment #14)
> (In reply to Markus Stange [:mstange] from comment #12)
> > Yeah, I remember that I wasn't happy with that part of the patch. In
> > IsFilterInputTainted, we really want to know whether *what the current draw
> > call is drawing* is tainted. I didn't see a good way of doing that when I
> > worked on the patch. Tobias, maybe you can find a way of fixing this without
> > relying on the fact that the call to SetWriteOnly occurs before the call to
> > IsFilterInputTainted.
> 
> After experimenting more with this, I don't see what would be a cleaner
> solution. We don't check IsWriteOnly every time we draw, since we only call
> IsFilterInputTainted when setting the filter. And in that case IsWriteOnly
> seems to be ok. What we need to make sure of tho, is updating the filter
> descriptor to run IsFilterInputTainted again after a drawing call tainted a
> Canvas used as SourceGraphics.

I don't quite follow this. Can you explain what you changed?
Flags: needinfo?(jmuizelaar) → needinfo?(tschneider)
(Assignee)

Comment 18

3 years ago
In the original patch, every time we set a new filter, or change a referenced SVG filter, we create a new filter description and save it in CurrentState().filter. During this, IsFilterInputTainted() is called to check if the canvas is write only. This only happens during those 2 scenarios, not every time we draw something. So using IsWriteOnly() and relying on the fact that SetWriteOnly() was called before doesn't feel fragile to me. But since the canvas can get tainted between setting the filter and drawing something, we have to make sure we update the filter description. That's what my changes are doing. And the tests I've created cover this case.
Flags: needinfo?(tschneider)
(Assignee)

Updated

3 years ago
Attachment #8749813 - Flags: review?(jmuizelaar)
Comment on attachment 8749813 [details] [diff] [review]
Part 1 (v2): Make filter tainting and canvas tainting interact properly

Review of attachment 8749813 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable.
Attachment #8749813 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Your Try push has test_filter_tainted.html failures all over it.
Assignee: mstange → tschneider
status-firefox41: affected → ---
Keywords: checkin-needed
(Assignee)

Comment 22

3 years ago
Created attachment 8754676 [details] [diff] [review]
Part 2: Tests (v3)

Fixes failing test.
Attachment #8746234 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Was totally looking at the wrong try run when asking for checkin, sorry for that. I updated the tests and started a new run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=280eef39b125
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
In the future, I'd strongly recommend basing your Try pushes off mozilla-central so you don't pick up the bustage of the minute from inbound/fx-team.
Comment on attachment 8749813 [details] [diff] [review]
Part 1 (v2): Make filter tainting and canvas tainting interact properly

layout/svg/nsSVGFilterInstance.cpp.rej          | 64 +++++++++++++++++++++++++

That seems bad.
Keywords: checkin-needed
(Assignee)

Comment 26

2 years ago
Created attachment 8756195 [details] [diff] [review]
Part 1 (v3): Make filter tainting and canvas tainting interact properly

Cleaned up patch.
Attachment #8749813 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
There are a lot of private browsing failures in that try run. I don't think they're from your patches, but I'd feel more comfortable landing this with a cleaner try run. Could you rebase this onto the tip of mozilla-central and re-push it to try, please?
Flags: needinfo?(tschneider)
Keywords: checkin-needed

Updated

2 years ago
Keywords: checkin-needed

Comment 31

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f43c78d7cc13
https://hg.mozilla.org/mozilla-central/rev/a7d611896f38
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Correctly implement the interaction of filters and canvas with respect to CORS.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
(Reporter)

Comment 33

2 years ago
I think bug 1173545 is the one that deserves a release note, not this one.
Based on Comment 33 I will minus this bug for the release notes.
relnote-firefox: ? → -
Depends on: 1287316

Comment 35

2 years ago
I think it's still not working probably. Why should the filter tainted the canvas in this example.

See example here: https://jsfiddle.net/tbbqa3uq/1/

Working in Chrome

Not working in Firefox. Security error.

Comment 36

2 years ago
(In reply to tiran133 from comment #35)
> I think it's still not working probably. Why should the filter tainted the
> canvas in this example.
> 
> See example here: https://jsfiddle.net/tbbqa3uq/1/
> 
> Working in Chrome
> 
> Not working in Firefox. Security error.

I believe this issue has been fixed by Bug 1307740. You can try the latest Nightly for sure. Reopen it if you still see it.
Resolution: FIXED → DUPLICATE
Duplicate of bug: 1307740
(Reporter)

Updated

2 years ago
Resolution: DUPLICATE → FIXED

Comment 37

2 years ago
(In reply to Vincent Liu[:vliu] from comment #36)
> (In reply to tiran133 from comment #35)
> > I think it's still not working probably. Why should the filter tainted the
> > canvas in this example.
> > 
> > See example here: https://jsfiddle.net/tbbqa3uq/1/
> > 
> > Working in Chrome
> > 
> > Not working in Firefox. Security error.
> 
> I believe this issue has been fixed by Bug 1307740. You can try the latest
> Nightly for sure. Reopen it if you still see it.
> 
> *** This bug has been marked as a duplicate of bug 1307740 ***

Thanks for the reply. I haven't seen this one. Yes, I tried the nightly build and it works just fine.

Thanks
You need to log in before you can comment on or make changes to this bug.