402649 - [FIX]window.location race condition can be used to spoof referer header

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Gregory Fleischer]

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]]

Attachment: 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]]

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)]

Boris, got cycles for this sometime soon?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: 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)]

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Trunk fix.

Comment 8

[Johnny Stenback (:jst)]

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 [:bzbarsky]]

Attachment: The right trunk fix

Comment 10

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.

Comment 11

[Daniel Veditz [:dveditz]]

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]]

Checked into 1.8 branch

Comment 13

[Carsten Book [:Tomcat]]

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]

blocking1.8.0.15, as this is a distro security patch.

Comment 15

[Alexander Sack]

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)]

Attachment: 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)]

Committed attachment 310817 [details] [diff] [review] to the 1.8.0 branch