Last Comment Bug 363897 - Don't give onerror handlers detailed information about syntax errors in off-site "scripts"
: Don't give onerror handlers detailed information about syntax errors in off-s...
Status: RESOLVED FIXED
[sg:want P4]
: csectype-disclosure, fixed1.8.1.21, verified1.8.1.19
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Hixie (not reading bugmail)
Mentors:
Depends on: 467439 696301
Blocks: historyleak CVE-2008-5507
  Show dependency treegraph
 
Reported: 2006-12-14 17:04 PST by Jesse Ruderman
Modified: 2013-06-09 20:17 PDT (History)
23 users (show)
jonas: blocking1.9+
samuel.sidler+old: blocking1.8.1.19+
samuel.sidler+old: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't give error handlers error info for xorigin errors. (1.95 KB, patch)
2007-07-05 15:55 PDT, Johnny Stenback (:jst, jst@mozilla.com)
mrbkap: review+
brendan: superreview+
Details | Diff | Splinter Review
Fix that was checked in. (2.18 KB, patch)
2007-07-09 15:11 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Fix for orange on tinderbox (checked in). (1.30 KB, patch)
2007-07-09 17:24 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
1.8-branch version (3.00 KB, patch)
2008-12-02 17:17 PST, Daniel Veditz [:dveditz]
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
clean patch for 1.8.0 (2.19 KB, patch)
2008-12-16 00:33 PST, Alexander Sack
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2006-12-14 17:04:02 PST
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.
Comment 1 Biju 2006-12-16 19:50:08 PST
another one which can do almost the same.
bug 147777
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2007-01-29 17:26:53 PST
jst will have a go at this
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2007-07-05 15:55:37 PDT
Created attachment 271141 [details] [diff] [review]
Don't give error handlers error info for xorigin errors.
Comment 4 Blake Kaplan (:mrbkap) 2007-07-05 16:20:21 PDT
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.
Comment 5 Brendan Eich [:brendan] 2007-07-07 14:53:47 PDT
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
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2007-07-09 15:11:32 PDT
Created attachment 271577 [details] [diff] [review]
Fix that was checked in.

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).
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2007-07-09 15:12:39 PDT
I filed followup bug 387476 and bug 387477, marking bug FIXED.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2007-07-09 17:24:56 PDT
Created attachment 271587 [details] [diff] [review]
Fix for orange on tinderbox (checked in).
Comment 9 İsmail Dönmez 2007-07-10 05:09:13 PDT
Any chance of a Firefox 2.0.0.x backport ?
Comment 10 Boris Zbarsky [:bz] 2007-07-14 13:32:49 PDT
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?
Comment 11 Boris Zbarsky [:bz] 2008-12-02 09:25:46 PST
Note that this did cause trouble for at least one production site (or at least for the debugging thereof).  See bug 467439.
Comment 12 Daniel Veditz [:dveditz] 2008-12-02 17:17:30 PST
Created attachment 351093 [details] [diff] [review]
1.8-branch version

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.
Comment 13 Daniel Veditz [:dveditz] 2008-12-02 17:57:15 PST
Comment on attachment 351093 [details] [diff] [review]
1.8-branch version

Checked into 1.8 branch
Comment 14 Daniel Veditz [:dveditz] 2008-12-02 19:49:16 PST
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
Comment 15 Daniel Veditz [:dveditz] 2008-12-02 20:34:10 PST
Checked into 1.8.1.19 relbranch after verifying the fix in a tinderbox run (mac).
Comment 16 Al Billings [:abillings] 2008-12-04 10:24:20 PST
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.
Comment 17 Boris Zbarsky [:bz] 2008-12-04 10:44:22 PST
The error console reports the full error.  The scripted onerror handler in the webpage is what gets no information cross-site.
Comment 18 Al Billings [:abillings] 2008-12-04 11:17:35 PST
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.
Comment 19 Alexander Sack 2008-12-16 00:32:55 PST
Comment on attachment 351093 [details] [diff] [review]
1.8-branch version

a=asac for 1.8.0 branch
Comment 20 Alexander Sack 2008-12-16 00:33:46 PST
Created attachment 353178 [details] [diff] [review]
clean patch for 1.8.0
Comment 21 Nick Thomas [:nthomas] 2008-12-17 02:19:08 PST
(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 Alexander Sack 2008-12-17 03:36:10 PST
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 Ben Hearsum (:bhearsum) 2008-12-17 09:41:45 PST
(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

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