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

RESOLVED FIXED

Status

()

Core
DOM: Events
--
enhancement
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: jst)

Tracking

({csectype-disclosure, fixed1.8.1.21, verified1.8.1.19})

Trunk
csectype-disclosure, fixed1.8.1.21, verified1.8.1.19
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.19 +
blocking1.8.1.next +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want P4])

Attachments

(5 attachments)

(Reporter)

Description

11 years ago
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

11 years ago
Whiteboard: [sg:want P4]

Comment 1

11 years ago
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+
(Assignee)

Comment 3

10 years ago
Created attachment 271141 [details] [diff] [review]
Don't give error handlers error info for xorigin errors.
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+
(Assignee)

Comment 6

10 years ago
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).
(Assignee)

Comment 7

10 years ago
I filed followup bug 387476 and bug 387477, marking bug FIXED.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

10 years ago
Created attachment 271587 [details] [diff] [review]
Fix for orange on tinderbox (checked in).

Updated

10 years ago
Attachment #271587 - Attachment is patch: true
Attachment #271587 - Attachment mime type: application/octet-stream → text/plain

Comment 9

10 years ago
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+
(Reporter)

Updated

9 years ago
Blocks: 412525
Blocks: 461735
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.
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.
Attachment #351093 - Flags: review?(jst)
(Assignee)

Updated

9 years ago
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?
Keywords: fixed1.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
Keywords: fixed1.8.1.19 → fixed1.8.1.20
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.
Keywords: fixed1.8.1.19 → verified1.8.1.19
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+

Updated

9 years ago
Flags: blocking1.8.0.next+

Comment 19

9 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

9 years ago
Created attachment 353178 [details] [diff] [review]
clean patch for 1.8.0
(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

9 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?
(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

6 years ago
Depends on: 696301
(Reporter)

Updated

4 years ago
Keywords: csec-disclosure
You need to log in before you can comment on or make changes to this bug.