Last Comment Bug 672013 - Allow SVG-as-an-image to be drawn into a canvas without marking it as write-only
: Allow SVG-as-an-image to be drawn into a canvas without marking it as write-only
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla11
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 693940
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-15 19:27 PDT by Daniel Holbert [:dholbert]
Modified: 2014-11-28 14:42 PST (History)
16 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (7.89 KB, patch)
2011-11-07 02:20 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dholbert: review+
Details | Diff | Splinter Review
object URL testcase (976 bytes, application/xhtml+xml)
2011-11-10 13:03 PST, Eli Grey (:sephr)
no flags Details
object URL testcase (976 bytes, application/xhtml+xml)
2011-11-10 13:05 PST, Eli Grey (:sephr)
no flags Details
fix v2 (3.18 KB, patch)
2011-11-20 15:17 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
bzbarsky: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-07-15 19:27:45 PDT
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] 2011-10-07 10:29:41 PDT
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) 2011-11-07 01:27:06 PST
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) 2011-11-07 02:20:09 PST
Created attachment 572412 [details] [diff] [review]
fix

This limits SVG image resource loads to data: URIs and about:blank.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-11-07 05:16:25 PST
> 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] 2011-11-07 10:44:23 PST
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 [:bz] (still a bit busy) 2011-11-07 12:06:57 PST
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] 2011-11-07 12:17:41 PST
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) 2011-11-07 12:25:50 PST
Right, thanks!
Comment 9 Daniel Holbert [:dholbert] 2011-11-07 12:34:33 PST
(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] 2011-11-07 13:56:05 PST
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) 2011-11-07 15:45:16 PST
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) 2011-11-07 18:00:56 PST
Does this break object URLs, which are essentially the same as data: URIs security-wise, if not safer?
Comment 13 Eli Grey (:sephr) 2011-11-07 18:04:31 PST
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 [:bz] (still a bit busy) 2011-11-07 18:44:19 PST
See bug 693940 comment 14.
Comment 15 Daniel Holbert [:dholbert] 2011-11-09 21:10:28 PST
(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) 2011-11-10 13:03:48 PST
Created attachment 573615 [details]
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) 2011-11-10 13:05:24 PST
Created attachment 573617 [details]
object URL testcase

Oops, forgot MIME type.
Comment 18 Eli Grey (:sephr) 2011-11-10 13:07:53 PST
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] 2011-11-10 13:26:26 PST
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) 2011-11-10 14:09:18 PST
(In reply to Daniel Holbert [:dholbert] from comment #19)

Yes.
Comment 21 Daniel Holbert [:dholbert] 2011-11-12 21:50:10 PST
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) 2011-11-20 15:17:54 PST
Created attachment 575779 [details] [diff] [review]
fix v2
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2011-12-15 12:02:44 PST
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) 2011-12-18 00:57:43 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/06e8ee7aa95d
Comment 25 Matt Brubeck (:mbrubeck) 2011-12-18 08:18:23 PST
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) 2011-12-18 13:59:17 PST
This didn't cause the failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/973938d037a9
Comment 27 Daniel Holbert [:dholbert] 2011-12-18 15:02:34 PST
(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] 2011-12-19 05:39:47 PST
https://hg.mozilla.org/mozilla-central/rev/89a8e26f1df0

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