1174278 - Make filter tainting and canvas tainting interact properly

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Markus Stange [:mstange]]

Attachment: 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.

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: Part 1: Make filter tainting and canvas tainting interact properly

Rebased old wip patch.

Comment 2

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Tests

Tests.

Comment 3

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Tests

Sorry, uploaded the wrong patch earlier.

Comment 4

[Markus Stange [:mstange]]

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?

Comment 5

[Markus Stange [:mstange]]

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.

Comment 6

[Markus Stange [:mstange]]

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.

Comment 7

[Tobias Schneider [:tobytailor]]

> 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?

Comment 8

[Tobias Schneider [:tobytailor]]

> 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?

Comment 9

[Markus Stange [:mstange]]

(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?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

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 11

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 12

[Markus Stange [:mstange]]

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.

Comment 13

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Tests (v2)

Added suggested tests.

Comment 14

[Tobias Schneider [:tobytailor]]

(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.

Comment 15

[Tobias Schneider [:tobytailor]]

Attachment: 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?

Comment 16

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v2): Make filter tainting and canvas tainting interact properly

Sorry, uploaded the wrong patch.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(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?

Comment 18

[Tobias Schneider [:tobytailor]]

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.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 20

[Tobias Schneider [:tobytailor]]

Thanks for the review Jeff!

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19ee910b53a4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Your Try push has test_filter_tainted.html failures all over it.

Comment 22

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Tests (v3)

Fixes failing test.

Comment 23

[Tobias Schneider [:tobytailor]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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 25

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 26

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v3): Make filter tainting and canvas tainting interact properly

Cleaned up patch.

Comment 27

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b68100cf06a2

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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?

Comment 29

[Tobias Schneider [:tobytailor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=548297cc239c

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f43c78d7cc13
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d611896f38

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f43c78d7cc13
https://hg.mozilla.org/mozilla-central/rev/a7d611896f38

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

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)]:

Comment 33

[Markus Stange [:mstange]]

I think bug 1173545 is the one that deserves a release note, not this one.

Comment 34

[Marcia Knous [:marcia]]

Based on Comment 33 I will minus this bug for the release notes.

Comment 35

[tiran133]

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

[Vincent Liu[:vliu]]

(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.

Comment 37

[tiran133]

(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