414540 - RDFXMLDataSource should reject cross-domain redirects

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect, P2)

Description

[georgi - hopefully not receiving bugspam]

Attachment: rdf3a.xul - won't work from bugzilla, needs redirect

it is possible to still RDF from any http hosts via:
datasources="/cgi-bin/re?http://SOMEWHERE/steal.rdf"

if the RDF structure is known
/cgi-bin/re does a http redirect
testcase won't work from bugzilla

not clear if this allows stealing arbitrary XML.
both trunk and 2.0 are vulnerable

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: steal.rdf

Comment 2

[Daniel Veditz [:dveditz]]

Is it specific to RDF? More likely it would apply to any non-local template data. There is a movement to remove RDF from the trunk, but obviously this kind of XUL widget needs some kind of datasource. Could whatever replaces it also have the same problem?

How does the "Mozilla Amazon Browser" work? I'd be surprised if the Amazon interface were RDF specifically rather than plain XML, but maybe it's not using a tree widget with a datasource.
http://faser.net/mab/chrome/content/mab.xul

Comment 3

[Benjamin Smedberg]

The same-origin check is here: http://mxr.mozilla.org/mozilla/source/content/xul/templates/src/nsXULTemplateBuilder.cpp#1275

So yes, it appears that you could steal XML using this method as well (on trunk).

Is there a way to tell necko not to follow redirects across origins (or a redirect observer mechanism which could do the check)?

Comment 4

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
> 
> How does the "Mozilla Amazon Browser" work? I'd be surprised if the Amazon
> interface were RDF specifically rather than plain XML, but maybe it's not using
> a tree widget with a datasource.
> http://faser.net/mab/chrome/content/mab.xul
> 

don't know how it works, but it seems to parse plain XML.
sniffer catched this request which returns plain XML:

http://faser.net/mab/chrome/content/server/proxy.cfm?t=faser-20&dev-t=D3MAIAYX2Q6JLY&KeywordSearch=math&mode=books&sort=+salesrank&type=lite&page=2&f=xml

so it seems this bug allows stealing XML and well formed (X)HTML, allowing harvesting luser's webmail.

this makes it at least [sg:moderate] in my opinion

Comment 5

[georgi - hopefully not receiving bugspam]

(In reply to comment #3)
> 
> Is there a way to tell necko not to follow redirects across origins (or a
> redirect observer mechanism which could do the check)?
> 

don't know, but XMLHttpRequest provides similar functionality and it is not vulnerable to this attack, so probably you may reuse code from it.

Comment 6

[Neil Deakin]

> Is there a way to tell necko not to follow redirects across origins (or a
> redirect observer mechanism which could do the check)?
> 

XMLHttpRequest does this by implementing nsIChannelEventSink and adding itself as an observer on the channel. RDF datasources could implement this but there isn't currently a means for the template builder to indicate this. XML datasources are loaded via the dom document.load api so don't have access to a channel, as far as I know.

Comment 7

[Benjamin Smedberg]

ok, the XMLDocument.load API does a redirect check successfully: http://mxr.mozilla.org/mozilla/source/content/xml/document/src/nsXMLDocument.cpp#292

Comment 8

[georgi - hopefully not receiving bugspam]

[sg:high?] per defintion
High: Vulnerability can be used to gather sensitive data from
         sites in other windows or inject data or code into those
         sites, requiring no more than normal browsing actions.

Comment 9

[georgi - hopefully not receiving bugspam]

on second thought, http redirects are recurring attack vector.
xmlhttprequest is safe now, yet SOAP* and WSDL* should be hit with redirects hard.
are there other ways to process xml except the above?

Comment 10

[Daniel Veditz [:dveditz]]

We'll block on this when the trunk fix is done. Might be '.14 rather than '.13

Comment 11

[georgi - hopefully not receiving bugspam]

i consider reading arbitrary xml (including well formed html) "blocking ?" so requesting blocking

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sigh, remote XUL.  Let's just disable it with a per-site pref for 3.next, OK?  Pretty please?

I don't think we should push back FF3 for this -- which is what "blocking" means on a bug with no patch at this point -- but we could and should take a well-tested fix into 3.0.1.

Comment 13

[georgi - hopefully not receiving bugspam]

>Sigh, remote XUL.  Let's just disable it with a per-site pref for 3.next, OK? 

Quite agree.

Comment 14

[chris hofmann]

who would/could work on a patch?

Comment 15

[Mike Schroepfer]

Neil can you help us out here.  Also can you work up the disable but for per-site remote xul pref for 3.1?  As an aside could we do that in a 3.0.x?

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #12)
> Sigh, remote XUL.  Let's just disable it with a per-site pref for 3.next, OK? 
> Pretty please?

File that bug and mark it blocking1.9.1?

Comment 17

[Neil Deakin]

Attachment: fix with test

I can't get the test to actually run though, so I haven't tested this, but something like this should work.

Comment 18

[Jesse Ruderman]

Nominating this bug for blocking1.9.1: security researchers are looking for redirect bugs, so this is very likely to be discovered independently.

Comment 19

[Benjamin Smedberg]

Neil, is this patch ready for review? When you say you couldn't get the test to run, what do you mean?

If you don't have time to drive this, assign it to me.

Comment 20

[Neil Deakin]

It should be ready, but wasn't sure how to run unit tests. I'll be on vacation tomorrow so reassigning would be the best option.

Comment 21

[Benjamin Smedberg]

Attachment: Neils patch with the test fixed

This is Neil's patch with the test fixed (you needed to have "server" in the global scope). r=me, requesting sr from bz

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 359329 [details] [diff] [review]
Neils patch with the test fixed

Does that test really fail without this patch?  Shouldn't the port in the url be 4444 instead of 8080, since you start the server on 4444?

Also, the OnChannelRedirect impl needs to look more like nsSameOriginChecker::OnChannelRedirect (and we really need to expose that on the security manager or some such...)

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 359329 [details] [diff] [review]
Neils patch with the test fixed

>diff --git a/rdf/tests/unit/test_rdfredirect.js b/rdf/tests/unit/test_rdfredirect.js

>+  server.start(4444);
>+  var ds = rdfService.GetDataSource("http://localhost:8080/sample.rdf");

This will fail with or without the patch, I'll bet, because the use of an unexpected hostport will trigger a HTTP 400 response.  (Or rather, it will if the request is HTTP/1.1, which I have no reason to assume wouldn't be the case.)  Probably need to check for that specific case using the ignored arguments in the sink observer methods, probably also check that the request handler was actually called as well to be safe...

>+var gErrorOccured = false;

typo


>diff --git a/testing/xpcshell/test_all.sh b/testing/xpcshell/test_all.sh

> # Make assertions fatal
>-XPCOM_DEBUG_BREAK=stack-and-abort; export XPCOM_DEBUG_BREAK
>+# XPCOM_DEBUG_BREAK=stack-and-abort; export XPCOM_DEBUG_BREAK

Don't even think about it.  :-P

If you really have to, flip the environment variable inside the test itself, but don't take away assertion-checking from everyone else's tests.

Comment 24

[Benjamin Smedberg]

Attachment: Copied nsSameOriginChecker code, fixed tests, rev. 2

This copies nsSameOriginChecker. I filed bug 475940 on providing this code in a shared location.

The test for this is significantly reworked. It loads two HTTP servers on different ports. It verifies that it can load RDF from each, as well as that a same-origin redirect is not blocked and a cross-origin redirect is blocked. Asking Waldo to review the testing code because I'm just learning my way around there.

As noted on IRC, there's inconsistent behavior when loading data from a rejected redirect. In some cases we'll get an error code and the load will fail. In other cases, the load will succeed but give us data from the redirecting page. So instead of relying on the onError callback, we actually check to see whether we loaded the RDF data.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 359507 [details] [diff] [review]
Copied nsSameOriginChecker code, fixed tests, rev. 2

Looks good.  Gods, our RDF API is ugly.  :(

Comment 26

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/01c42e286b4c

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 359507 [details] [diff] [review]
Copied nsSameOriginChecker code, fixed tests, rev. 2

Only thing worth mentioning is that if this ever fails, you're likely to not decrement the pending count correctly and thus hang, but that seems relatively unlikely to happen; best would be to put the decrement and is-done test in a finally block (rest of method in a try) to be safe.

Comment 28

[Benjamin Smedberg]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/de14d3aaefdf

Comment 29

[georgi - hopefully not receiving bugspam]

>Nominating this bug for blocking1.9.1: security researchers are looking for
>redirect bugs, so this is very likely to be discovered independently.

similar redirect stuff is:
Bug 466989 stealing xul overlays via http redirects

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 359507 [details] [diff] [review]
Copied nsSameOriginChecker code, fixed tests, rev. 2

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 31

[Benjamin Smedberg]

Checking in rdf/base/src/Makefile.in;
new revision: 1.38; previous revision: 1.37
Checking in rdf/base/src/nsRDFXMLDataSource.cpp;
new revision: 1.159; previous revision: 1.158
Checking in rdf/tests/Makefile.in;
new revision: 1.11; previous revision: 1.10
Checking in rdf/tests/unit/sample.rdf;
initial revision: 1.1
Checking in rdf/tests/unit/test_rdfredirect.js;
initial revision: 1.1

Comment 32

[Al Billings [:abillings - ex-MoCo]]

This was a pain in the butt to set up on my private webservers. :-)

Verified fixed in 1.9.0.7 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021304 GranParadiso/3.0.7pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.7pre) Gecko/2009021304 GranParadiso/3.0.7pre

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090213 Shiretoko/3.1b3pre.

Comment 34

[Martin Stránský [:stransky] (ni? me)]

I wonder how the fix should look like for 1.8. Is the test by CheckSameOriginURI() enough here or do we need to involve (and backport) CheckMayLoad() from 1.9?

Comment 35

[Boris Zbarsky [:bzbarsky]]

CheckSameOriginURI is probably fine.

Comment 36

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 version (looks like 1.8.1 too)

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 363399 [details] [diff] [review]
1.8.0 version (looks like 1.8.1 too)

Approved for 1.8.1.21, a=dveditz for release-drivers

Comment 38

[Daniel Veditz [:dveditz]]

Fix checked into the 1.8 branch. I did not check in the tests since 1.8 doesn't support XPCSHELL_TESTS

Comment 39

[Alexander Sack]

Comment on attachment 363399 [details] [diff] [review]
1.8.0 version (looks like 1.8.1 too)

a=asac for 1.8.0 - thanks requesting approval1.8.0