Closed Bug 363897 Opened 18 years ago Closed 17 years ago

Don't give onerror handlers detailed information about syntax errors in off-site "scripts"

Categories

(Core :: DOM: Events, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: jst)

References

Details

(Keywords: csectype-disclosure, fixed1.8.1.21, verified1.8.1.19, Whiteboard: [sg:want P4])

Attachments

(5 files)

http://jeremiahgrossman.blogspot.com/2006/12/i-know-if-youre-logged-in-anywhere.html describes how a malicious site can use onerror and <script src="..."> to determine whether you're logged into another site. Loading an HTML page using <script src="..."> usually triggers a syntax error, but the exact error and line number depend on the HTML. Firefox could prevent this kind of attack on most sites by giving less information to the onerror handler when a script loaded from another site has a syntax error. I doubt this would break any sites other than ones trying to determine whether you're logged into another site. I imagine that this could be done either by throwing same-origin exceptions when the site tries to access certain fields of the object passed to the onerror handler. But it would probably be simpler to give the onerror handler an object that contains fewer details about the error. The full syntax error could still be shown in the Error Console.
Whiteboard: [sg:want P4]
another one which can do almost the same. bug 147777
Flags: blocking1.9?
jst will have a go at this
Assignee: events → jst
Flags: blocking1.9? → blocking1.9+
Attachment #271141 - Flags: superreview?(brendan)
Attachment #271141 - Flags: review?(mrbkap)
Comment on attachment 271141 [details] [diff] [review] Don't give error handlers error info for xorigin errors. I asked Johnny if the filename could potentially leak information as well (due to redirects if logged in) and it looks like we'll compile the script with whatever URI was passed in the src= attribute as the filename.
Attachment #271141 - Flags: review?(mrbkap) → review+
Comment on attachment 271141 [details] [diff] [review] Don't give error handlers error info for xorigin errors. >@@ -333,8 +335,34 @@ NS_ScriptErrorReporter(JSContext *cx, > nsScriptErrorEvent errorevent(PR_TRUE, NS_LOAD_ERROR); > > errorevent.fileName = fileName.get(); >- errorevent.errorMsg = msg.get(); >- errorevent.lineNr = report ? report->lineno : 0; >+ >+ nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(win)); >+ nsIPrincipal *p = sop->GetPrincipal(); >+ >+ PRBool sameOrigin = PR_FALSE; >+ >+ if (p) { >+ nsCOMPtr<nsIURI> errorURI; >+ NS_NewURI(getter_AddRefs(errorURI), >+ NS_ConvertUTF16toUTF8(fileName).get()); Couldn't you use report->filename directly? It's either ASCII or (if JS_C_STRINGS_ARE_UTF8) UTF-8 already. >+ >+ nsCOMPtr<nsIURI> codebase; >+ p->GetURI(getter_AddRefs(codebase)); >+ >+ if (errorURI && codebase) { >+ sameOrigin = >+ NS_SUCCEEDED(sSecurityManager-> >+ CheckSameOriginURI(errorURI, codebase)); Ideally we'd have a followup bug to provide error principal, which ought to be the subject principal, to the error reporter -- then you could use CheckSameOriginPrincipal here. But it seems you can't just get the subject principal here, because it may be too late (we may be reporting an uncaught exception and the script that failed to catch has unwound). If this seems sane, please file that followup and cite it here with a FIXME: comment. sr=me with the above taken into account. /be
Attachment #271141 - Flags: superreview?(brendan) → superreview+
Attached patch Fix that was checked in. (deleted) — Splinter Review
This deals with Brendan's comments and also fixes a problem with not null checking report like the surrounding code does (bug 387478 filed to clean that up).
I filed followup bug 387476 and bug 387477, marking bug FIXED.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Attachment #271587 - Attachment is patch: true
Attachment #271587 - Attachment mime type: application/octet-stream → text/plain
Any chance of a Firefox 2.0.0.x backport ?
Hmm. This patch makes me pretty unhappy, with the "URI == principal" thing. Is there any way to store the principal of the script that throws in exceptions?
Flags: in-testsuite?
Flags: wanted1.8.1.x+
Blocks: historyleak
Depends on: 467439
Note that this did cause trouble for at least one production site (or at least for the debugging thereof). See bug 467439.
Attached patch 1.8-branch version (deleted) — Splinter Review
Version for 1.8 branch. The only minor change (besides context) is getting the nsIScriptObjectPrincipal from globalObject directly instead of from "win", a nsPIDOMWindow QI'd from globalObject that isn't used on the 1.8 branch.
Attachment #351093 - Flags: review?(jst)
Attachment #351093 - Flags: superreview+
Attachment #351093 - Flags: review?(jst)
Attachment #351093 - Flags: review+
Comment on attachment 351093 [details] [diff] [review] 1.8-branch version Checked into 1.8 branch
Attachment #351093 - Flags: approval1.8.1.19?
I guess technically we've branched so this is fixed for the next one, and I have to check into the relbranch to fix it for 1.8.1.19
Checked into 1.8.1.19 relbranch after verifying the fix in a tinderbox run (mac).
Keywords: fixed1.8.1.19
Using the proof of concept at http://ha.ckers.org/weird/javascript-website-login-checker.html, I'm still seeing a difference in the error console log if I'm logged into a site and if I'm not. For Google, if I'm logged in and run the check in the PoC, I get the following in the error console: Error: syntax error Source File: https://mail.google.com/mail/ Line: 1 Source Code: <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" If I'm *not* logged in, I get: Error: invalid XML attribute value Source File: https://www.google.com/accounts/ServiceLogin?service=mail&passive=true&rm=false&continue=http%3A%2F%2Fmail.google.com%2Fmail%2F%3Fui%3Dhtml%26zy%3Dl&bsv=1k96igf4806cy&ltmpl=default&ltmplcache=2 Line: 5, Column: 12 Source Code: <style type=text/css> Unless I'm misunderstanding things, we should not be giving this information.
The error console reports the full error. The scripted onerror handler in the webpage is what gets no information cross-site.
Ah! I've verified it with the PoC then using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.19) Gecko/2008120316 Firefox/2.0.0.19. It is fixed.
Attachment #351093 - Flags: approval1.8.1.next+
Attachment #351093 - Flags: approval1.8.1.19?
Attachment #351093 - Flags: approval1.8.1.19+
Flags: blocking1.8.1.next+
Flags: blocking1.8.1.19+
Flags: blocking1.8.0.next+
Comment on attachment 351093 [details] [diff] [review] 1.8-branch version a=asac for 1.8.0 branch
Attachment #351093 - Flags: approval1.8.0.next+
Attached patch clean patch for 1.8.0 (deleted) — Splinter Review
(In reply to comment #15) > Checked into 1.8.1.19 relbranch after verifying the fix in a tinderbox run > (mac). The win32 builds that shipped today for Firefox 2.0.0.19 do not contain this fix. We accidentally shipped the first build we did, rather than the respin which took this fix. Not sure what to do with the keywords to capture this.
Nick, did you ship the FIREFOX_2_0_0_19_BUILD1 tag or has the FIREFOX_2_0_0_19_RELEASE tag not properly pushed forward?
(In reply to comment #22) > Nick, did you ship the FIREFOX_2_0_0_19_BUILD1 tag or has the > FIREFOX_2_0_0_19_RELEASE tag not properly pushed forward? The FIREFOX_2_0_0_19_RELEASE tag was moved forward to match FIREFOX_2_0_0_19_BUILD2
Depends on: 696301
Keywords: csec-disclosure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: