303181 - urlSecurityCheck and getReferrer incorrectly use focusedWindow

Status: RESOLVED FIXED

(Toolkit Graveyard :: Security, defect)

Description

[Jesse Ruderman]

http://lxr.mozilla.org/mozilla/source/toolkit/content/contentAreaUtils.js#131

The focused window could easily be different than the one trying to load the
URL.  Perhaps urlSecurityCheck should take as a parameter the document trying to
do the load, or the URL / URI of that document.  Why does it take as a parameter
a XUL document?

Comment 1

[Jesse Ruderman]

This is a Toolkit bug, but I'm putting it in the Firefox product for now due to
bug 303183.

Comment 2

[Jesse Ruderman]

Back in the Toolkit product thanks to justdave in bug 303183 comment 1.

Comment 3

[Jesse Ruderman]

Searching contentAreaUtils for "focusedWindow" reveals that the focused window
(or frame?) is used for many other things, including figuring out what referrer
to send.  Scary.

Comment 4

[Jesse Ruderman]

Attachment: Testcase: Cmd+click the link in this demo to see a spoofed referrer. (Load from a local server because Bugzilla is https, and make sure "Allow scripts to raise and lower windows" is checked.)

Comment 5

[Jesse Ruderman]

I'm thinking of changing openNewTabWith and openNewWindowWith to take a new
parameter, either |content.document| or a URL/URI to use for the referrer and
security checks.  I could use some help figuring out what this parameter should
be.  Requirements:

* The URL used for the referrer and security checks should be a "real URL".  If
a page has a javascript: URL, |content.document.location.href| is not a "real
URL", so doc.location.href isn't the right way to do this.

* Whatever I use as a parameter should survive navigating away from the page. 
If |content.document| evaporates when I navigate away from the page, that means
I have to extract the "real URL" early and pass that around instead of passing
|content.document| around.

Hopefully I'm not duplicating work happening in bug 293363.

Comment 6

[Jesse Ruderman]

Attachment: patch

This patch fixes bug 284868 and makes the testcase in this bug behave properly.
 I'd need answers to my questions in comment 5 to be sure that this patch fixes
the security hole completely.

Comment 7

[Jesse Ruderman]

Attachment: patch 2

Addresses some comments Gavin made on IRC.

Comment 8

[Jesse Ruderman]

It would be nice to have a fix for this bug in before beta because the fix might
break extensions.

Comment 9

[Mike Schroepfer]

Jesse is this patch ready to go?  If so let's set the review flags. 

Comment 10

[Daniel Veditz [:dveditz]]

the XPFE version has the same pattern. Need mconnor and neil looking at this. Is
this change going to break extensions? or is openNewTabWith() a purely internal
function?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> the XPFE version has the same pattern. Need mconnor and neil looking at this. Is
> this change going to break extensions? or is openNewTabWith() a purely internal
> function?

openNewTabWith is used at the very least by ChatZilla, and I'd imagine by many
other extensions wanting to open tabs in the browser, so I'd guess that the risk
of breaking extensions is rather high.

Comment 12

[Mike Connor [:mconnor]]

I think that because we've changed URLs, extensions from 1.0.x that are using
this are already broken, and I don't really care about extensions being
backwards compat with the alphas.  We just need to announce this widely before beta.

Comment 13

[Daniel Veditz [:dveditz]]

Changing the package for contentAreaUtils.js may have broken extensions that are
explicitly including it, but has no affect on extensions overlayed into the
browser context.

If we have to back-port this to the aviary/1.7 branches then we care a lot more
about breaking extensions. A spoofed referrer isn't worth a branch re-spin, but
what bad things could you do by spoofing the urlSecurityCheck?

Comment 14

[Jesse Ruderman]

Attachment: patch 3

Changes from previous patch:
* Pass document URLs around instead of documents.
* Resolve merge conflicts with bug 78510.
* Several small changes.

Comment 15

[Jesse Ruderman]

Attachment: patch 4

Comment 16

[[e-mail]]

In order to bypass the security check a page would have to bypass the existing
security checking on frame focussing - for instance they can't focus about:

Comment 17

[Jesse Ruderman]

So the attacker would have to convince the user to tab into or click in the frame?

Comment 18

[[e-mail]]

This attack only appears to affect context menus, as they introduce a delay in
between the click and the action in which the script can shift the focus.

Comment 19

[Jesse Ruderman]

The demo in comment 4 works for me with Cmd+clicking, so it's not just context
menus.  That demo uses the time between mousedown and mouseup to move focus.

Comment 20

[[e-mail]]

I see it now; I'm just not used to clicking that slowly!

Comment 21

[Mike Shaver (:shaver emeritus)]

Blocking beta.  mconnor: your review goodness is craved.

Comment 22

[Mike Shaver (:shaver emeritus)]

Can someone file a version for xpfe as well?

Comment 23

[Jesse Ruderman]

Attachment: patch 5

Changes since last patch:
* Fix urlSecurityCheck calls for "View Image" so they don't break the feature.
* Add urlSecurityCheck calls to Save Link and Save Image.
* Add a few checks to setDesktopBackground (part of bug 302022).

Comment 24

[Jesse Ruderman]

Attachment: patch 6

Update to trunk, detab, and fix some of mconnor's nits.

Comment 25

[Jesse Ruderman]

cvs -d :ext:[e-mail]:/cvsroot commit
browser/base/content/browser.js toolkit/content/contentAreaUtils.js 

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.478; previous revision: 1.477
done

Checking in toolkit/content/contentAreaUtils.js;
/cvsroot/mozilla/toolkit/content/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.77; previous revision: 1.76
done

Comment 26

[Daniel Veditz [:dveditz]]

This patch broke the "linky" extension by removing the getReferrer() function it
was using.

Comment 27

[Jesse Ruderman]

getReferrer() returned bogus results and could not be fixed given its API, so I
don't mind forcing extensions to change.

Comment 28

[Daniel Veditz [:dveditz]]

Jesse, neil: do we need a Suite version of this patch as well?

Comment 29

[[e-mail]]

(In reply to comment #28)
>do we need a Suite version of this patch as well?
It looks as if all versions of the Suite have this bug too, yes.