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

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: dholbert, Assigned: roc)

Tracking

({dev-doc-complete})

Trunk
mozilla11
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Summary: Allow SVG-as-an-image to be drawn into a canvas → Allow SVG-as-an-image to be drawn into a canvas without marking it as write-only
(Reporter)

Comment 1

6 years ago
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.
Why don't we just change that check to require that the URI is a data: URI?
Created attachment 572412 [details] [diff] [review]
fix

This limits SVG image resource loads to data: URIs and about:blank.
Assignee: nobody → roc
Attachment #572412 - Flags: superreview?(bzbarsky)
Attachment #572412 - Flags: review?(dholbert)
> 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.

Updated

6 years ago
Flags: in-testsuite?
(Reporter)

Comment 5

6 years ago
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.)
Depends on: 693940
The fix that was just landed in bug 693940 fixes the content policy issue for purposes of this bug, I think....
(Reporter)

Comment 7

6 years ago
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.
Attachment #572412 - Flags: review?(dholbert) → review+
Right, thanks!
(Reporter)

Comment 9

6 years ago
(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. :))
This will need a fix to https://developer.mozilla.org/en/HTML/Canvas/Drawing_DOM_objects_into_a_canvas at a minimum.
Keywords: dev-doc-needed
No need to land these changes this today, I think. This is non-urgent and should wait until after the merge.

Comment 12

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

Comment 13

6 years ago
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.
See bug 693940 comment 14.
(Reporter)

Comment 15

6 years ago
(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

6 years ago
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

6 years ago
Created attachment 573617 [details]
object URL testcase

Oops, forgot MIME type.
Attachment #573615 - Attachment is obsolete: true

Comment 18

6 years ago
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?
(Reporter)

Updated

6 years ago
Attachment #573617 - Attachment mime type: application/octet-stream → application/xhtml+xml
(Reporter)

Comment 19

6 years ago
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?
(Reporter)

Updated

6 years ago
Attachment #573615 - Attachment mime type: application/octet-stream → application/xhtml+xml

Comment 20

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

Yes.
(Reporter)

Comment 21

6 years ago
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.
Created attachment 575779 [details] [diff] [review]
fix v2
Attachment #572412 - Attachment is obsolete: true
Attachment #575779 - Flags: review?(bzbarsky)
Attachment #572412 - Flags: superreview?(bzbarsky)
Comment on attachment 575779 [details] [diff] [review]
fix v2

r=me.  I'm really sorry for the lag!
Attachment #575779 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/06e8ee7aa95d
Whiteboard: [inbound]
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
Whiteboard: [inbound]
This didn't cause the failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/973938d037a9
Whiteboard: [inbound]
(Reporter)

Comment 27

6 years ago
(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)
Flags: in-testsuite? → in-testsuite+
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/89a8e26f1df0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Doc complete

https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#section_43
https://developer.mozilla.org/en/Firefox_11_for_developers#section_4
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.