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

RESOLVED FIXED in mozilla1.9beta2

Status

()

Core
DOM
P2
major
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Gregory Fleischer, Assigned: bz)

Tracking

({fixed1.8.0.15, verified1.8.1.10})

unspecified
mozilla1.9beta2
fixed1.8.0.15, verified1.8.1.10
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.10 +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high] CSRF against many sites, URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
Component: Security → DOM
Product: Firefox → Core
QA Contact: firefox → general

Updated

10 years ago
Flags: blocking1.9?
Flags: blocking1.8.1.10?
Whiteboard: [sg:moderate] CSRF against many sites
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 402713
Filed bug 402713 on the Minefield crash just in case it's unrelated and will be unfixed by whatever fixes this one.
Boris, got cycles for this sometime soon?
Assignee: nobody → bzbarsky
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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?
Attachment #287647 - Flags: review?(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).
Attachment #287647 - Flags: review?(jst) → review+
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.
Attachment #287647 - Flags: superreview?(peterv)
Attachment #287647 - Flags: approval1.8.1.10?
Created attachment 288116 [details] [diff] [review]
Trunk fix.
Attachment #288116 - Flags: superreview?(jst)
Attachment #288116 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Summary: window.location race condition can be used to spoof referer header → [FIX]window.location race condition can be used to spoof referer header
Target Milestone: --- → mozilla1.9 M10
Comment on attachment 288116 [details] [diff] [review]
Trunk fix.

Um, this doesn't look related to this bug at all, wrong diff?
Created attachment 288147 [details] [diff] [review]
The right trunk fix
Attachment #288116 - Attachment is obsolete: true
Attachment #288147 - Flags: superreview?(jst)
Attachment #288147 - Flags: review?(jst)
Attachment #288116 - Flags: superreview?(jst)
Attachment #288116 - Flags: review?(jst)

Updated

10 years ago
Attachment #288147 - Flags: superreview?(jst)
Attachment #288147 - Flags: superreview+
Attachment #288147 - Flags: review?(jst)
Attachment #288147 - Flags: review+
Checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Flags: in-testsuite?
Flags: blocking1.8.1.10?
Flags: blocking1.8.1.11?
Flags: blocking1.8.1.10?
Flags: blocking1.8.1.10+
Whiteboard: [sg:moderate] CSRF against many sites → [sg:high] [need sr] CSRF against many sites
Comment on attachment 287647 [details] [diff] [review]
Possible branch patch that illustrates the problem

sr=dveditz

approved for 1.8.1.10, a=dveditz
Attachment #287647 - Flags: superreview?(peterv)
Attachment #287647 - Flags: superreview+
Attachment #287647 - Flags: approval1.8.1.10?
Attachment #287647 - Flags: approval1.8.1.10+
Whiteboard: [sg:high] [need sr] CSRF against many sites → [sg:high] CSRF against many sites
Checked into 1.8 branch
Keywords: fixed1.8.1.10
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
Keywords: fixed1.8.1.10 → verified1.8.1.10
Group: security
Depends on: 404627

Comment 14

10 years ago
blocking1.8.0.15, as this is a distro security patch.
Flags: blocking1.8.0.15+

Comment 15

10 years ago
Comment on attachment 287647 [details] [diff] [review]
Possible branch patch that illustrates the problem

caillon, please approve this for 1.8.0.15.
Attachment #287647 - Flags: approval1.8.0.15?
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
Attachment #310817 - Flags: approval1.8.0.15+
Attachment #287647 - Flags: approval1.8.0.15? → approval1.8.0.15-
Committed attachment 310817 [details] [diff] [review] to the 1.8.0 branch
Keywords: fixed1.8.0.15
You need to log in before you can comment on or make changes to this bug.