656277 - Prevent loading WebGL textures from cross-domain images

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

In order to fix bug 655987 i.e. respond to this exploit,
  http://www.contextis.co.uk/resources/blog/webgl/poc/index.html

We need to prevent using cross-domain images as WebGL textures. We also need to prevent using a canvas/2d as proxy for doing the same thing. Then we may consider relaxing this with CORS.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

MDC page:
https://developer.mozilla.org/en/WebGL/Cross-Domain_Textures

I'm planning to link to it from a JS warning message.

Sheppy, is there a way to give an "international" link as opposed to an "en" link? In case this page gets localized? Should I bother?

Comment 2

[Janet Swisher]

There's no way to do a language-neutral link, but if there are translations, users can select a different language from a menu within the page.

This topic needs version or date information to explain what "currently" and "no longer" refer to. Because it's in MDN, it may be found by readers doing web searches, who will have no context.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: block cross-domain textures, v1

Here's a first version that seems to be working.

TODO:
 * CORS relaxation
 * Discussion is going on the WebGL list to decide if on illegal cross-domain textures we should throw exceptions (as this patch does) or only generate WebGL errors (which are only JS warnings).
 * adjust the WebGL test suite/mochitest to avoid permaoranges. Will do a tryserver build ASAP. At least the origin-clean-conformance test should be rendered obsolete by these changes.

Also, I have one following worry. I tried a page that tried loading a texture from a <img> with a local relative path as URL, like "picture.jpg". This is working when the page is hosted on http, but when the page URL is itself a local relative path like "page.html", this is rejected unless I set the preference security.fileuri.strict_origin_policy=false.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

@ Janet: sorry, I didn't realize. I will try to clarify that ASAP.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

* The proof-of-concept attack is no longer working with this patch.
* Most of the WebGL demos I've tried are still working with this patch. In particular the following are working:
   -- Mozilla demos: No Comply; Flight of the Navigator;
   -- SpiderGL's demos
   -- Google's WebGL Aquarium
* The following demos are no longer working:
   -- Mozilla's 360 Degree Video
   -- Cooliris' Photowall (this one was fully expected)

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Angry birds is still working. Ship it!

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to comment #3)
> This is working when the page is hosted on http, but when the page URL is
> itself a local relative path like "page.html", this is rejected unless I set
> the preference security.fileuri.strict_origin_policy=false.

To prevent saved-then-opened-from-file web pages from being able to read everything on your local disk we consider each directory to be the root of a separate domain. Although if picture.jpg is in the same directory as page.html that shouldn't be the issue and I wouldn't expect the pref change to fix it.

Set the pref back to the default when you're done testing.

Comment 8

[Asa Dotzler [:asa]]

Benoit, time is getting short for Firefox 5. How much more work is required here to get something that could ship there?

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: block cross-domain textures, ready to be landed, but ONLY land it in case of emergency!

Ready to land, applies on top of the patch in bug 657190 which I'm going to land very soon (but can be manually applied without it).

This passes the mochitest on my machinel; tryserver build here:
http://tbpl.mozilla.org/?tree=Try&rev=f50e0f5e86eb

Compared to v1, the changes are:
* only c++ change: when the image principal is null, consider the origin check to be OK. This mimics what is already being done in Canvas DoDrawImageSecurityCheck. I hit this case with images served from mochi.test:8888
* disabled a test and half of another test, that are rendered obsolete by the underlying spec change here.
* edited tests to load images from mochi.test:8888 instead of example.com so that they aren't blocked as cross-domain textures.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Asa, this patch breaks compatibility with a non-negligible amount of web content, more so than we ultimately have to (when the CORS solution is implemented). So we don't necessarily want to land it. Only land if the security flaw at hand here is deemed too severe to stay unpatched.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #2)
> This topic needs version or date information to explain what "currently" and
> "no longer" refer to. Because it's in MDN, it may be found by readers doing
> web searches, who will have no context.

I've updated https://developer.mozilla.org/en/WebGL/Cross-Domain_Textures , hopefully it's more clear now.

Comment 12

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

Comment on attachment 533787 [details] [diff] [review]
block cross-domain textures, ready to be landed, but ONLY land it in case of emergency!

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

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #9)
> This passes the mochitest on my machinel; tryserver build here:
> http://tbpl.mozilla.org/?tree=Try&rev=f50e0f5e86eb

All green on tryserver.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533787 [details] [diff] [review]
block cross-domain textures, ready to be landed, but ONLY land it in case of emergency!

> +        nsHTMLCanvasElement *canvas = static_cast<nsHTMLCanvasElement*>(domCanvas.get());

This is bad.  You have no guarantee that domCanvas is an actual canvas element here; it could be some arbitrary jsobject.

The right way to do this is probably:

  nsCOMPtr<nsIContent> maybeDOMCanvas = do_QueryInterface(imageOrCanvas);
  if (maybeDOMCanvas && maybeDOMCanvas->IsHTML(nsGkAtoms::canvas)) {
    // Now it's safe to cast.
  }

Another question: should these cross-origin draw attempts throw, or just silently do nothing?  I'd sort of lean toward the latter, maybe.

I sort of hope this code is not performance-sensitive.  :(

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: [UPDATED] block cross-domain textures, ready to be landed, but ONLY land it in case of emergency!

Applied bz's review comments.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #14)
> Another question: should these cross-origin draw attempts throw, or just
> silently do nothing?  I'd sort of lean toward the latter, maybe.

This is being discussed on the WebGL list. Not sure what the final decision will be. On the one hand throwing a DOM_SECURITY_ERR seems more DOM-ish and helps web developers find quickly where problems are; on the other hand not throwing feels more WebGL-ish (WebGL uses error codes for most kinds of errors) and might break a little less existing content.

> 
> I sort of hope this code is not performance-sensitive.  :(

It's not really performance-sensitive (though we don't want it to be slower than necessary). The only common case where it starts getting a little performance sensitive is when playing a <video> through WebGL.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Note that in general the 2d canvas context silently ignores erroneous input instead of throwing.

I think we shouldn't throw here, precisely because of existing content.  Better to paint wrong than to stop the script completely.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 534028 [details] [diff] [review]
[UPDATED] block cross-domain textures, ready to be landed, but ONLY land it in case of emergency!

r=me modulo that return value question.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

OK, then there is the nontrivial question of whether to
 * generate a WebGL error? which one?
 * fail completely silently?
 * not generate a WebGL error but mark the texture as bad and treat it as a 1x1 black texture, as WebGL does in other cases?

It's still being discussed among WebGL people. I will forward your comments to them. Meanwhile, please don't hold r+ for that: we need something that could conceivably landed if Security wants to, and in real-world tests I found that throwing does not break significantly more content, because it seems that the (relatively few) WebGL apps that rely on cross-domain textures are using try { ... } catch anyways.

Comment 20

[Cedric Vivier [:cedricv]]

(In reply to comment #17)
> I think we shouldn't throw here, precisely because of existing content. 
> Better to paint wrong than to stop the script completely.

I think throwing is a more helpful and consistent semantic here.

Not throwing would makes debugging new content and migration unnecessarily harder.

The few existing content, like picture viewers, that uses cross-domain images are completely useless anyways if all textures they load are black, therefore stopping the script is more useful and ecologically-friendly ;)

Also, consistency is important : texImage2D supports uploading an ImageData object, therefore texImage2D(..., crossDomainImgElement) should behave the same imho as the equivalent call texImage2D(..., canvas_I_drawn_the_crossDomainImgElement_to.getImageData()), which will throw a SECURITY_ERR as per Canvas specification.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> therefore stopping the script is more useful

That _really_ depends on the page.  There are cases where stopping the script leads to really bad effects...

Benoit, if you're happy shipping the throwing version as a security update to a stable release, fine by me.  In other DOM code we wouldn't do that, but you have a better idea of how this object is used than I do.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

The amount of web content that this patch breaks either way, is much larger than the difference in web content broken between the two approaches. Therefore, it's not a huge issue. The main thing that prevents me from giving you right away a non-throwing version, is that it's not clear to me whether we'd then want to generate a WebGL error. Generating unexpected WebGL errors can break WebGL scripts in different ways. Again, this conversation is being had at the WebGL WG at the moment.

Comment 23

[JP Rosevear [:jpr]]

Comment on attachment 534028 [details] [diff] [review]
[UPDATED] block cross-domain textures, ready to be landed, but ONLY land it in case of emergency!

Please land.

Comment 24

[Benoit Jacob [:bjacob] (mostly away)]

How about adding a pref to get back the old behavior? I think that would ease acceptance.

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Forget comment 24 --- I'm told by demo people that it doesn't help much.

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

This is a sad day :-( :-( :-(

Landed on beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/c4d4c278c462

Landed on aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/e227e4d50900

Landed on central:
http://hg.mozilla.org/mozilla-central/rev/4a87e9b7cc96

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

Filed follow-up for CORS: bug 662599.

Comment 28

[Asa Dotzler [:asa]]

I presume we want to note this for developers. Adding keyword to that affect. Feel free to remove if I'm wrong.

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

We have already written:
https://developer.mozilla.org/en/WebGL/Cross-Domain_Textures

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

(Affected JS code generates JS warnings pointing to this page)

Comment 31

[Paul Rouget [:paul]]

I'll make sure the WebGL community knows about this.

Comment 32

[Daniel Veditz [:dveditz]]

updating flags to reflect comment 26

Comment 34

[Eric Shepherd [:sheppy]]

This was already documented.

Comment 35

[AndreiD[QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

I was trying to see if this is fixed for Fx6 since the flag "status-firefox6" is set on "fixed", but I couldn't.
Can anyone say what is expected to be seen for the test example in comment 1, or is there another testcase that can be used to verify the fix? Thanks

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

AndreiD, this is fixed since Firefox 5. All pages loading cross-domain images without CORS approval as WebGL textures now fail to. For example, in the original proof of concept,

   http://www.contextis.co.uk/resources/blog/webgl/poc/index.html

as soon as you load a cross-domain image (click the 'Go' button under 'Steal image URL'), you should now only see some noise instead of seeing the actual image. In Firefox 4, before this got fixed, you could see the image or at least an approximation of it.