Last Comment Bug 402649 - [FIX]window.location race condition can be used to spoof referer header
: [FIX]window.location race condition can be used to spoof referer header
Status: RESOLVED FIXED
[sg:high] CSRF against many sites
: fixed1.8.0.15, verified1.8.1.10
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: P2 major (vote)
: mozilla1.9beta2
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://pseudo-flaw.net/mfrs/referer-s...
Depends on: 404627
Blocks: 402713
  Show dependency treegraph
 
Reported: 2007-11-05 20:20 PST by Gregory Fleischer
Modified: 2008-03-20 12:56 PDT (History)
13 users (show)
jst: blocking1.9+
dveditz: blocking1.8.1.10+
asac: blocking1.8.0.next+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
oritinal testcase (edited into a single file) (13.31 KB, text/html)
2007-11-06 07:44 PST, Daniel Veditz [:dveditz]
no flags Details
Possible branch patch that illustrates the problem (3.20 KB, patch)
2007-11-06 21:28 PST, Boris Zbarsky [:bz]
jst: review+
dveditz: superreview+
dveditz: approval1.8.1.10+
caillon: approval1.8.0.next-
Details | Diff | Splinter Review
Trunk fix. (1.25 KB, patch)
2007-11-10 01:32 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
The right trunk fix (2.64 KB, patch)
2007-11-10 10:04 PST, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Combined 1.8.0 branch patch (1.75 KB, patch)
2008-03-20 12:53 PDT, Christopher Aillon (sabbatical, not receiving bugmail)
caillon: approval1.8.0.next+
Details | Diff | Splinter Review

Description Gregory Fleischer 2007-11-05 20:20:41 PST
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9

Firefox is vulnerable to a race-condition when setting 'window.location' values using JavaScript. In some circumstances, the race-condition can be exploited to spoof referer headers. Specially crafted iframe content can be used to perform cross-site request forgery (CSRF) against sites that accept GET requests and use referer checking for protection.

Reproducible: Always




There is an online demo and additional write-up at: http://pseudo-flaw.net/mfrs/referer-spoofing.html
Comment 1 Daniel Veditz [:dveditz] 2007-11-06 07:44:09 PST
Created attachment 287538 [details]
oritinal testcase (edited into a single file)

The original testcase in case the server becomes unreachable at some point, lightly edited to include the joke.js and winding_paths.js files in-line (makes attaching a working copy easier).
Comment 2 Daniel Veditz [:dveditz] 2007-11-06 08:11:18 PST
Filed bug 402713 on the Minefield crash just in case it's unrelated and will be unfixed by whatever fixes this one.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2007-11-06 16:15:51 PST
Boris, got cycles for this sometime soon?
Comment 4 Boris Zbarsky [:bz] 2007-11-06 21:28:42 PST
Created attachment 287647 [details] [diff] [review]
Possible branch patch that illustrates the problem

So the issue is that we base our referrer URI on the JSContext.  In this case, the new page has loaded, so the JSContext's stuff corresponds to the new window.  In particular, we get the "current document" from the JSContext and then use its URI.

However, we do have something pointing to where the actual code that's running comes from: the principal.

Given that, it seems to me that we have a number of options:

1) Only send the document URI if callerPrincipal == doc->GetPrincipal()
2) Only send the document URI if callerPrincipal->Equals(doc->GetPrincipal())
3) Only send the document URI if callerPrincipal->Subsumes(doc->GetPrincipal())
4) Send the callerPrincipal URI no matter what

This particular patch implements #3 (which means we don't send a referrer at all on the testcase in this bug).  I've verified that it fixes the bug in a branch build.

One could make an argument for doing #4 throughout (that is, do it in docshell too, for link clicks and the like).  That would only change behavior for cases like this, and for javascript: and data: URIs.  Which I think is not a bad idea at all.  I'd favor option #4 for trunk.

jst, thoughts?
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2007-11-08 00:51:52 PST
Comment on attachment 287647 [details] [diff] [review]
Possible branch patch that illustrates the problem

Yes, this makes sense, and doing #4 sounds like the right thing to do for trunk. We should get that in for beta2 ideally to get as much testing on it as possible (not that I think it's a particularly risk thing to change, but still).
Comment 6 Boris Zbarsky [:bz] 2007-11-10 01:17:03 PST
Comment on attachment 287647 [details] [diff] [review]
Possible branch patch that illustrates the problem

Let's get this into the branch.  I'll work on a trunk fix.
Comment 7 Boris Zbarsky [:bz] 2007-11-10 01:32:39 PST
Created attachment 288116 [details] [diff] [review]
Trunk fix.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2007-11-10 09:51:56 PST
Comment on attachment 288116 [details] [diff] [review]
Trunk fix.

Um, this doesn't look related to this bug at all, wrong diff?
Comment 9 Boris Zbarsky [:bz] 2007-11-10 10:04:13 PST
Created attachment 288147 [details] [diff] [review]
The right trunk fix
Comment 10 Boris Zbarsky [:bz] 2007-11-11 11:16:56 PST
Checked in on trunk.
Comment 11 Daniel Veditz [:dveditz] 2007-11-13 19:04:14 PST
Comment on attachment 287647 [details] [diff] [review]
Possible branch patch that illustrates the problem

sr=dveditz

approved for 1.8.1.10, a=dveditz
Comment 12 Daniel Veditz [:dveditz] 2007-11-14 03:59:33 PST
Checked into 1.8 branch
Comment 13 Carsten Book [:Tomcat] 2007-11-15 08:31:57 PST
verified fixed using the testcase from dveditz (comment #1) on:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.10) Gecko/2007111504 Firefox/2.0.0.10

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.10)
Gecko/2007111504 Firefox/2.0.0.10
Comment 14 Alexander Sack 2008-03-12 08:41:43 PDT
blocking1.8.0.15, as this is a distro security patch.
Comment 15 Alexander Sack 2008-03-12 08:42:31 PDT
Comment on attachment 287647 [details] [diff] [review]
Possible branch patch that illustrates the problem

caillon, please approve this for 1.8.0.15.
Comment 16 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 12:53:33 PDT
Created attachment 310817 [details] [diff] [review]
Combined 1.8.0 branch patch

Attachment 287647 [details] [diff] was committed and then merged with the trunk fix.  This patch just combines them to match what was committed to 1.8.1.x.

Patch by bzbarsky
r/sr=jst/dveditz
a1.8.0.15=caillon
Comment 17 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 12:56:43 PDT
Committed attachment 310817 [details] [diff] [review] to the 1.8.0 branch

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