Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Prevent loading WebGL textures from cross-domain images

RESOLVED FIXED in Firefox 5

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

unspecified
mozilla6
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5+ fixed, firefox6+ fixed, status2.0 wanted, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:moderate])

Attachments

(1 attachment, 2 obsolete attachments)

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

Comment 1

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

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

Comment 3

6 years ago
Created attachment 532407 [details] [diff] [review]
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.
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 years ago
@ Janet: sorry, I didn't realize. I will try to clarify that ASAP.
(Assignee)

Comment 5

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

Comment 6

6 years ago
Angry birds is still working. Ship it!
(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.

Updated

6 years ago
tracking-firefox5: --- → +
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status2.0: --- → wanted
status-firefox5: --- → affected
tracking-firefox6: --- → ?
Whiteboard: [sg:moderate]

Comment 8

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

Comment 9

6 years ago
Created attachment 533787 [details] [diff] [review]
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.
Attachment #532407 - Attachment is obsolete: true
Attachment #533787 - Flags: review?(roc)
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.
(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 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]:
-----------------------------------------------------------------
Attachment #533787 - Flags: review?(roc)
Attachment #533787 - Flags: review?(bzbarsky)
Attachment #533787 - Flags: review+
(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

6 years ago
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.  :(
Attachment #533787 - Flags: review?(bzbarsky) → review-
Created attachment 534028 [details] [diff] [review]
[UPDATED] block cross-domain textures, ready to be landed, but ONLY land it in case of emergency!

Applied bz's review comments.
Attachment #533787 - Attachment is obsolete: true
Attachment #534028 - Flags: review?(bzbarsky)
(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

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

6 years ago
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.
Attachment #534028 - Flags: review?(bzbarsky) → review+
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.
(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

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

Updated

6 years ago
tracking-firefox6: ? → +

Updated

6 years ago
Whiteboard: [sg:moderate] → [sg:moderate][blocks-fx5b5]

Updated

6 years ago
Attachment #534028 - Flags: approval-mozilla-beta?
Attachment #534028 - Flags: approval-mozilla-aurora?

Comment 23

6 years ago
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.
Attachment #534028 - Flags: approval-mozilla-beta?
Attachment #534028 - Flags: approval-mozilla-beta+
Attachment #534028 - Flags: approval-mozilla-aurora?
Attachment #534028 - Flags: approval-mozilla-aurora+
How about adding a pref to get back the old behavior? I think that would ease acceptance.
Forget comment 24 --- I'm told by demo people that it doesn't help much.
(Assignee)

Updated

6 years ago
Depends on: 662570
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
(Assignee)

Updated

6 years ago
Depends on: 662599
Filed follow-up for CORS: bug 662599.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 28

6 years ago
I presume we want to note this for developers. Adding keyword to that affect. Feel free to remove if I'm wrong.
Keywords: dev-doc-needed
We have already written:
https://developer.mozilla.org/en/WebGL/Cross-Domain_Textures
(Affected JS code generates JS warnings pointing to this page)

Comment 31

6 years ago
I'll make sure the WebGL community knows about this.
updating flags to reflect comment 26
status-firefox5: affected → fixed
status-firefox6: --- → fixed

Updated

6 years ago
Whiteboard: [sg:moderate][blocks-fx5b5] → [sg:moderate]
(Assignee)

Updated

6 years ago
Duplicate of this bug: 663823
Target Milestone: --- → mozilla6
This was already documented.
Keywords: dev-doc-needed → dev-doc-complete

Updated

6 years ago
Depends on: 666505

Comment 35

6 years ago
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
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.
You need to log in before you can comment on or make changes to this bug.