Closed Bug 414540 Opened 14 years ago Closed 12 years ago
Source should reject cross-domain redirects
9.10 KB, patch
|Details | Diff | Splinter Review|
7.28 KB, patch
|Details | Diff | Splinter Review|
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
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
Component: Security → RDF
Product: Firefox → Core
QA Contact: firefox → rdf
Version: 2.0 Branch → unspecified
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)?
Severity: normal → critical
(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
Whiteboard: [sg:investigate] → [sg:investigate] [sg:moderate?]
(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.
> 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.
ok, the XMLDocument.load API does a redirect check successfully: http://mxr.mozilla.org/mozilla/source/content/xml/document/src/nsXMLDocument.cpp#292
[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.
Whiteboard: [sg:investigate] [sg:moderate?] → [sg:investigate] [sg:high?]
Summary: stealing RDF via http redirects → stealing RDF and XML via http redirects
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?
We'll block on this when the trunk fix is done. Might be '.14 rather than '.13
Whiteboard: [sg:investigate] [sg:high?] → [sg:high?]
Flags: tracking1.9+ → wanted1.9.0.x+
i consider reading arbitrary xml (including well formed html) "blocking ?" so requesting blocking
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.
>Sigh, remote XUL. Let's just disable it with a per-site pref for 3.next, OK? Quite agree.
who would/could work on a patch?
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?
Assignee: nobody → enndeakin
(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?
Flags: blocking18.104.22.168? → blocking22.214.171.124-
Status: NEW → ASSIGNED
Summary: stealing RDF and XML via http redirects → RDFXMLDataSource should reject cross-domain redirects
I can't get the test to actually run though, so I haven't tested this, but something like this should work.
Nominating this bug for blocking1.9.1: security researchers are looking for redirect bugs, so this is very likely to be discovered independently.
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.
Flags: blocking1.9.1? → blocking1.9.1+
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.
Assignee: enndeakin → benjamin
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 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...)
Attachment #359329 - Flags: superreview?(bzbarsky) → superreview-
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.
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 on attachment 359507 [details] [diff] [review] Copied nsSameOriginChecker code, fixed tests, rev. 2 Looks good. Gods, our RDF API is ugly. :(
Attachment #359507 - Flags: review?(bzbarsky) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Attachment #359507 - Flags: review?(jwalden+bmo) → review+
>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
Attachment #359507 - Flags: approval126.96.36.199? → approval188.8.131.52+
Comment on attachment 359507 [details] [diff] [review] Copied nsSameOriginChecker code, fixed tests, rev. 2 Approved for 184.108.40.206, a=dveditz for release-drivers.
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
This was a pain in the butt to set up on my private webservers. :-) Verified fixed in 220.127.116.11 with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:18.104.22.168pre) Gecko/2009021304 GranParadiso/3.0.7pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:22.214.171.124pre) Gecko/2009021304 GranParadiso/3.0.7pre
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.
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?
CheckSameOriginURI is probably fine.
Comment on attachment 363399 [details] [diff] [review] 1.8.0 version (looks like 1.8.1 too) Approved for 126.96.36.199, a=dveditz for release-drivers
Attachment #363399 - Flags: approval1.8.1.next? → approval1.8.1.next+
Fix checked into the 1.8 branch. I did not check in the tests since 1.8 doesn't support XPCSHELL_TESTS
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
Attachment #363399 - Flags: approval1.8.0.next? → approval1.8.0.next+
You need to log in before you can comment on or make changes to this bug.