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)
Core
DOM: Events
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)
(deleted),
patch
|
mrbkap
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jst
:
review+
jst
:
superreview+
samuel.sidler+old
:
approval1.8.1.19+
samuel.sidler+old
:
approval1.8.1.next+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•18 years ago
|
Whiteboard: [sg:want P4]
another one which can do almost the same.
bug 147777
Updated•18 years ago
|
Flags: blocking1.9?
jst will have a go at this
Assignee: events → jst
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Comment 3•17 years ago
|
||
Attachment #271141 -
Flags: superreview?(brendan)
Attachment #271141 -
Flags: review?(mrbkap)
Comment 4•17 years ago
|
||
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 5•17 years ago
|
||
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+
Assignee | ||
Comment 6•17 years ago
|
||
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).
Assignee | ||
Comment 7•17 years ago
|
||
I filed followup bug 387476 and bug 387477, marking bug FIXED.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 8•17 years ago
|
||
Updated•17 years ago
|
Attachment #271587 -
Attachment is patch: true
Attachment #271587 -
Attachment mime type: application/octet-stream → text/plain
Comment 9•17 years ago
|
||
Any chance of a Firefox 2.0.0.x backport ?
Comment 10•17 years ago
|
||
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?
Updated•17 years ago
|
Flags: in-testsuite?
Updated•17 years ago
|
Flags: wanted1.8.1.x+
Reporter | ||
Updated•17 years ago
|
Blocks: historyleak
Updated•16 years ago
|
Blocks: CVE-2008-5507
Comment 11•16 years ago
|
||
Note that this did cause trouble for at least one production site (or at least for the debugging thereof). See bug 467439.
Comment 12•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #351093 -
Flags: superreview+
Attachment #351093 -
Flags: review?(jst)
Attachment #351093 -
Flags: review+
Comment 13•16 years ago
|
||
Comment on attachment 351093 [details] [diff] [review]
1.8-branch version
Checked into 1.8 branch
Attachment #351093 -
Flags: approval1.8.1.19?
Updated•16 years ago
|
Keywords: fixed1.8.1.19
Comment 14•16 years ago
|
||
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
Keywords: fixed1.8.1.19 → fixed1.8.1.20
Comment 15•16 years ago
|
||
Checked into 1.8.1.19 relbranch after verifying the fix in a tinderbox run (mac).
Keywords: fixed1.8.1.19
Comment 16•16 years ago
|
||
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<mpl=default<mplcache=2
Line: 5, Column: 12
Source Code:
<style type=text/css>
Unless I'm misunderstanding things, we should not be giving this information.
Comment 17•16 years ago
|
||
The error console reports the full error. The scripted onerror handler in the webpage is what gets no information cross-site.
Comment 18•16 years ago
|
||
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.
Keywords: fixed1.8.1.19 → verified1.8.1.19
Updated•16 years ago
|
Attachment #351093 -
Flags: approval1.8.1.next+
Attachment #351093 -
Flags: approval1.8.1.19?
Attachment #351093 -
Flags: approval1.8.1.19+
Updated•16 years ago
|
Flags: blocking1.8.1.next+
Flags: blocking1.8.1.19+
Updated•16 years ago
|
Flags: blocking1.8.0.next+
Comment 19•16 years ago
|
||
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+
Comment 20•16 years ago
|
||
Comment 21•16 years ago
|
||
(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.
Comment 22•16 years ago
|
||
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?
Comment 23•16 years ago
|
||
(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
Reporter | ||
Updated•11 years ago
|
Keywords: csec-disclosure
You need to log in
before you can comment on or make changes to this bug.
Description
•