Last Comment Bug 414540 - RDFXMLDataSource should reject cross-domain redirects
: RDFXMLDataSource should reject cross-domain redirects
Status: VERIFIED FIXED
[sg:high]
: fixed1.8.1.21, testcase, verified1.9.0.7, verified1.9.1
Product: Core
Classification: Components
Component: RDF (show other bugs)
: unspecified
: x86 Linux
: P2 critical (vote)
: ---
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on: 477930
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-29 04:09 PST by georgi - hopefully not receiving bugspam
Modified: 2009-03-05 03:17 PST (History)
17 users (show)
benjamin: blocking1.9.1+
shaver: blocking1.9-
mbeltzner: blocking1.9.0.1-
dveditz: blocking1.9.0.7+
mbeltzner: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
shaver: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix with test (7.19 KB, patch)
2008-06-25 12:11 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Neils patch with the test fixed (7.89 KB, patch)
2009-01-28 11:45 PST, Benjamin Smedberg [:bsmedberg]
benjamin: review+
bzbarsky: superreview-
Details | Diff | Splinter Review
Copied nsSameOriginChecker code, fixed tests, rev. 2 (9.10 KB, patch)
2009-01-29 06:16 PST, Benjamin Smedberg [:bsmedberg]
bzbarsky: review+
jwalden+bmo: review+
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review
1.8.0 version (looks like 1.8.1 too) (7.28 KB, patch)
2009-02-20 14:33 PST, Martin Stránský
dveditz: approval1.8.1.next+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2008-01-29 04:09:52 PST
Created attachment 299989 [details]
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 2008-01-29 04:11:30 PST
Created attachment 299990 [details]
steal.rdf
Comment 2 Daniel Veditz [:dveditz] 2008-01-29 09:05:07 PST
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 [:bsmedberg] 2008-01-29 14:30:04 PST
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 2008-01-30 00:07:31 PST
(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 2008-01-30 03:58:53 PST
(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 2008-01-30 06:03:59 PST
> 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 [:bsmedberg] 2008-01-30 09:38:38 PST
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 2008-02-01 00:19:38 PST
[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 2008-02-01 07:05:36 PST
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] 2008-02-22 11:29:41 PST
We'll block on this when the trunk fix is done. Might be '.14 rather than '.13
Comment 11 georgi - hopefully not receiving bugspam 2008-05-28 00:49:13 PDT
i consider reading arbitrary xml (including well formed html) "blocking ?" so requesting blocking
Comment 12 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-05-28 05:22:53 PDT
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 2008-05-28 08:34:44 PDT
>Sigh, remote XUL.  Let's just disable it with a per-site pref for 3.next, OK? 

Quite agree.
Comment 14 chris hofmann 2008-06-02 10:40:12 PDT
who would/could work on a patch?
Comment 15 Mike Schroepfer 2008-06-24 20:06:16 PDT
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] 2008-06-25 08:37:45 PDT
(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 2008-06-25 12:11:56 PDT
Created attachment 326758 [details] [diff] [review]
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 2009-01-23 16:19:41 PST
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 [:bsmedberg] 2009-01-28 09:01:12 PST
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 2009-01-28 09:07:06 PST
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 [:bsmedberg] 2009-01-28 11:45:06 PST
Created attachment 359329 [details] [diff] [review]
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 [:bz] 2009-01-28 11:54:31 PST
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] (remove +bmo to email) 2009-01-28 23:44:55 PST
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 [:bsmedberg] 2009-01-29 06:16:33 PST
Created attachment 359507 [details] [diff] [review]
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 [:bz] 2009-01-29 07:40:02 PST
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 [:bsmedberg] 2009-01-29 09:24:59 PST
http://hg.mozilla.org/mozilla-central/rev/01c42e286b4c
Comment 27 Jeff Walden [:Waldo] (remove +bmo to email) 2009-01-29 10:14:19 PST
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 [:bsmedberg] 2009-01-29 10:33:00 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/de14d3aaefdf
Comment 29 georgi - hopefully not receiving bugspam 2009-01-31 01:06:49 PST
>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] 2009-02-02 14:18:15 PST
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 [:bsmedberg] 2009-02-03 07:03:01 PST
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] 2009-02-13 13:00:11 PST
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] 2009-02-13 13:01:29 PST
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ý 2009-02-18 10:45:52 PST
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 [:bz] 2009-02-18 11:00:31 PST
CheckSameOriginURI is probably fine.
Comment 36 Martin Stránský 2009-02-20 14:33:21 PST
Created attachment 363399 [details] [diff] [review]
1.8.0 version (looks like 1.8.1 too)
Comment 37 Daniel Veditz [:dveditz] 2009-02-23 11:16:17 PST
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] 2009-02-28 22:55:00 PST
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 2009-03-05 03:17:09 PST
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

Note You need to log in before you can comment on or make changes to this bug.