672013 - Allow SVG-as-an-image to be drawn into a canvas without marking it as write-only

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

When we originally added SVG-as-an-image support for canvas, drawImage over in bug 589558, it was necessary to always mark the canvas as write-only, because same-origin SVG content could contain cross-origin resources.  (see bug 589558 comment 1)

However, since that time, we've locked down SVG-as-an-image to block all external resource loads, in bug 628747.

Hence, I think we can safely relax our canvas restrictions, to treat SVG images just like any other image for the purpose of drawImage -- no additional write-only-setting required.

This would enable the SVG Open proposal "From SVG to Canvas and Back" to actually work.*  (Last I heard, the "and back" part of their presentation doesn't work natively in any browser, since the SVG makes the canvas write-only.  See the "Security issues" slide, which mentions cross-origin issues and write-only-flagging.)

* http://www.svgopen.org/2010/presentations/62-From_SVG_to_Canvas_and_Back/index.html

Comment 1

[Daniel Holbert [:dholbert]]

Note that the URI_IS_LOCAL_RESOURCE check that bug 628747 added *does* allow local files served over file:// to load any other local file.

If we were to fix this bug right now (allowing SVG images to be painted to a canvas without tainting), it would create a data-leakage problem, because a local file could have an <img> pointing to a SVG image that included e.g. /etc/passwd or something else sensitive, and it could paint that to a canvas and read it.  Right now, the existing canvas-tainting protects us from that.

So, we need to strengthen bug 628747's URI_IS_LOCAL_RESOURCE check to prevent that sort of thing before fixing this bug.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why don't we just change that check to require that the URI is a data: URI?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This limits SVG image resource loads to data: URIs and about:blank.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Why don't we just change that check to require that the URI is a data: URI?

Because special-casing protocol names is generally a bad idea; in particular it prevents extensions from adding protocol handlers sanely.

Comment 5

[Daniel Holbert [:dholbert]]

Note that Bug 693940 is filed on adding a URI_INHERITS_SECURITY_CONTEXT check, which should cut off the using-a-local-file-as-an-image leak and allow us to safely remove the canvas-taint in this bug.

(Marking this bug as depending on that one -- sorry for not having done that already.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

The fix that was just landed in bug 693940 fixes the content policy issue for purposes of this bug, I think....

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 572412 [details] [diff] [review]
fix

r=me on the nsLayoutUtils.cpp changes and the new reftest, modulo a few reftest-nits below.

(bug 693940 should cover the rest of the patch's changes, I think -- sorry for not fixing that sooner & for not linking it more clearly to this one!)

A few nits on the new reftest:

>+      function go() {
>+        var canvas = document.getElementById("canvas");
>+        var ctx = canvas.getContext("2d");
>+        var image = document.getElementById("image");
>+
>+        // Draw the SVG image on top of our red
>+        ctx.drawImage(image, 0, 0);

I don't think there's any red drawn here, so "on top of our red" doesn't make sense.  You probably want something like this:
        ctx.fillStyle = "red";
        ctx.fillRect(0, 0, 100, 100);
before the drawImage call.

(or alternately, just remove "on top of our red" from the comment)

>+        try {
>+          ctx.toDataURL();
>+        } catch (ex) {
>+          document.body.textContent = ex;
>+        }

When I try this locally (with the patch applied), I get:
  TypeError: ctx.toDataURL is not a function

I think you want s/ctx/canvas/ there -- that seems to fix it for me.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right, thanks!

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #6)
> The fix that was just landed in bug 693940 fixes the content policy issue
> for purposes of this bug, I think....

s/just landed/just pushed to try/ (but it's essentially ready-to-land; I tested it locally, and I mostly did the try push as indemnification against having caused orange, in the inevitable day-before-migration pile-on. :))

Comment 10

[Eric Shepherd [:sheppy]]

This will need a fix to https://developer.mozilla.org/en/HTML/Canvas/Drawing_DOM_objects_into_a_canvas at a minimum.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No need to land these changes this today, I think. This is non-urgent and should wait until after the merge.

Comment 12

[Eli Grey (:sephr)]

Does this break object URLs, which are essentially the same as data: URIs security-wise, if not safer?

Comment 13

[Eli Grey (:sephr)]

roc has informed me that this does break object URLs, which will break my dom2canvas library, and encourages/requires people to use outdated and inefficient data: URIs over object URLs. It should be changed to handle all local URIs equivalently.

Comment 14

[Boris Zbarsky [:bzbarsky]]

See bug 693940 comment 14.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Eli Grey (:sephr) from comment #13)
> roc has informed me that this does break object URLs, which will break my
> dom2canvas library

Eli: Is there a demo version of this library available for testing anywhere? (I want to make sure the yet-to-come improved version of bug 693940's patch works correctly.)

Comment 16

[Eli Grey (:sephr)]

Attachment: object URL testcase

(In reply to Daniel Holbert [:dholbert] from comment #15)

No, but here's a simplified testcase for it. It should say PASS if the patch is working properly.

Comment 17

[Eli Grey (:sephr)]

Attachment: object URL testcase

Oops, forgot MIME type.

Comment 18

[Eli Grey (:sephr)]

Huh? Apparently I didn't forget the MIME type and it's just ignoring the MIME type I provided. Is there an appropriate Bugzilla bug for this?

Comment 19

[Daniel Holbert [:dholbert]]

Fixed the mimetype on your latest attachment.

(Not sure what you were experiencing with that, but if it's a real bug, you could file it at https://bugzilla.mozilla.org/enter_bug.cgi?product=Bugzilla or https://bugzilla.mozilla.org/enter_bug.cgi?product=bugzilla.mozilla.org )

So to be clear -- it's expected that all existing Firefox versions will fail this test (since it draws an SVG image into a canvas and then tries to read the canvas), but the goal is for your test to pass after this bug is fixed -- right?

Comment 20

[Eli Grey (:sephr)]

(In reply to Daniel Holbert [:dholbert] from comment #19)

Yes.

Comment 21

[Daniel Holbert [:dholbert]]

Eli: Thanks very much for the testcase -- it helped me write a test for the updated patch on bug 693940.

That bug's new patch preserves the (existing) allowance on moz-filedata URIs inside of SVG-as-an-image.  And when this bug here lands, images (with or without moz-filedata URIs in them) will be able to be drawn to canvas without tainting it.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 575779 [details] [diff] [review]
fix v2

r=me.  I'm really sorry for the lag!

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06e8ee7aa95d

Comment 25

[Matt Brubeck (:mbrubeck)]

Backed out on inbound (along with other patches in the same push) because of Mac and Win7 reftest failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff088809cd2a

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This didn't cause the failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/973938d037a9

Comment 27

[Daniel Holbert [:dholbert]]

(note that the [inbound] whiteboard-status is deprecated; see crossed-out line here:
 https://wiki.mozilla.org/Inbound_Sheriff_Duty#Please_do_the_following_after_pushing_to_inbound )

Also, for the record, the pushed cset for this bug was:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/89a8e26f1df0
(973938d037a9 was another cset in the same push)

Comment 28

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/89a8e26f1df0

Comment 29

[Jeremie Patonnier :Jeremie]

Doc complete

https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#section_43
https://developer.mozilla.org/en/Firefox_11_for_developers#section_4