162393 - [FIX]a variable called "content" confuses urlSecurityCheck in contentAreaUtils.js

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Jesse Ruderman]

http://www.uni-duesseldorf.de/ulb/db.html defines a variable called |content|,
which causes "Open Link in New Window" to break with this error message:

Error: focusedWindow._content.location has no properties
Source File: chrome://communicator/content/contentAreaUtils.js
Line: 59

Thanks to biesi for pointing out this bug on #mozillazine.

This is a security hole.  If a page defines |content| such that
content.location.href is the string "chrome://foo", middle-clicking (or
selecting Open Link in New Window) on a link to a chrome url succeeds.

I think functions in contentAreaUtils.js such as openNewWindowWith and
urlSecurityCheck should pass around the content document that contains the link
rather than relying on xuldocument.commandDispatcher.focusedWindow and
isDocumentFrame.  It seems to me that passing around the content document would
be simpler.

Comment 1

[Jesse Ruderman]

-> caillon

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yikes.  Looking.

Comment 3

[Jesse Ruderman]

Attachment: testcase

Comment 4

[Jesse Ruderman]

Attachment: testcase A: links to chrome

Comment 5

[Jesse Ruderman]

Attachment: testcase B: spoofs the referrer as a different http site

Comment 6

[Johnny Stenback (:jst)]

Um, testcase B doesn't seem to be exploitable using a trunk build, is it
exploitable using a branch build only?

Comment 7

[Bradley Baetz (:bbaetz)]

Haven't we seen this before? Bug 143369 was this type of problem, too, wasn't it?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

jst, testcase B is exploitable for me if I open the link in a new window.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

oops, forgot to mention, that was with trunk build 2002081209 on windows98.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

There's actually several different bugs that this is about.  I can fix these,
but I'd like to know whether or not I need to conjure up a "real fix" or a
"lowest risk" fix (i.e. is this wanted for MachV?).  Nominating in either case.

Comment 11

[georgi - hopefully not receiving bugspam]

The good news is that javascript: URLs don't seem to run with chrome privileges
even after this trick.

Comment 12

[Johnny Stenback (:jst)]

Oh, duh. I didn't read testcase B through before testing, it is exploitable
using trunk builds.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Can't do anything serious with this, so not a MachV stopper.  This is something
we should fix in Buffy though.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 167941 has been marked as a duplicate of this bug. ***

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed fix v.1.0

Works against both testcases.  I actually want to remove urlSecurityCheck
completely from contentAreaUtils.js.  There is still one place,
http://lxr.mozilla.org/mozilla/source/editor/ui/composer/content/editorApplicationOverlay.js#63
that calls it, but I can't find any callers of that method (editLink).	I'll
double check with the editor folks first, though.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Same as before; but completely removing urlSecurityCheck

With the code completely moved into browser.xml

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

glazou/cmanske: is this change ok with you guys?

Comment 18

[Charles Manske]

Comment on attachment 99792 [details] [diff] [review]
Same as before; but completely removing urlSecurityCheck

r=cmanske for change in
editorApplicationOverlay.js

Comment 19

[jag (Peter Annema)]

This change doesn't seem right. For a frameset we would use the frame's URL as
the referrer, now we'll always be using the outermost frameset's URL.

Also, do we always want to fall back to the document.currentURI or about:blank
for the referrer in the security check? Should clicking a bookmark in the
personal toolbar really be checked against document.currentURI?

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v2.0

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need this for 1.2 final.

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

All righty then.

Comment 23

[jag (Peter Annema)]

So comments on irc to caillon:

isDocumentFrame should probably be renamed to isContentFrame or isContentWindow

focusedWindow.location.href is unsafe and should be replaced with the
lookupMethod version.

Comment 24

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v2.1

Comment 25

[jag (Peter Annema)]

Comment on attachment 103009 [details] [diff] [review]
Patch v2.1

>Index: xpfe/browser/resources/content/navigator.js

>-  if (!_content || !_content.frames.length || !isDocumentFrame(document.commandDispatcher.focusedWindow))
>+  if (!content || !content.length || !isContentFrame(document.commandDispatcher.focusedWindow))

Even though you and I know that content.frames == content (ewww), I think the
code is clearer if you keep this as content.frames.length.

>Index: xpfe/communicator/resources/content/contentAreaUtils.js

> function urlSecurityCheck(url, doc) 
> {
>   // URL Loading Security Check
>   var focusedWindow = doc.commandDispatcher.focusedWindow;
>+  var sourceWin = getContentFrameHref(focusedWindow);

Perhaps this should be named sourceURL instead of sourceWin.

sr=jag

Comment 26

[jag (Peter Annema)]

Actually, name the function getContentFrameURI, href is the property name but
the URI is really what you're getting there.

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v2.2

jag's comments addressed.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 103106 [details] [diff] [review]
Patch v2.2

r=bzbarsky

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 103106 [details] [diff] [review]
Patch v2.2

transferring sr=jag

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 103106 [details] [diff] [review]
Patch v2.2

a=asa for checkin to 1.2 (on behalf of drivers)

Comment 31

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fixed, though this didn't make the beta.

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

caillon:

I suspect this bug applies to the 1.0 branch as well.  If so, once it gets a
little baking on trunk, I'd like you to make a 1.0 branch patch as well.

Comment 33

[Christopher Aillon (sabbatical, not receiving bugmail)]

Sure will do.  For reference, this fix depends on the fix for bug 159667 which
was not checked into the 1.0 branch.

Comment 34

[Kathleen :Brade]

this has caused regressions in composer

Comment 35

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch for 1.0 branch

I haven't actually tested this against the 1.0 branch since it appears the
branch doesn't support the compiler I'm using (gcc3.2), which seems a bit
strange to me as gcc3.2 is likely going to be sticking around for quite a
while...

Comment 36

[Christopher Blizzard (:blizzard)]

I'm kind of confused.  This made it into the mozilla trunk, right?  I see a note
about regressions in composer.

Comment 37

[Christopher Aillon (sabbatical, not receiving bugmail)]

The regression is/was bug 175304 and has already been fixed on trunk.  The branc
patch I uploaded includes that fix (sans the comment as requested by brade).

Comment 38

[Michael Buckland]

Discussed in bBird team meeting.  Minusing for bBird.

Comment 39

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 104130 [details] [diff] [review]
Patch for 1.0 branch

Adding a=asa,shaver,roc+moz via email and confirming with roc on irc that 3 oks
is an implied a=.

Comment 40

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in on the branch.

Comment 41

[bsharma]

Verified on 2002-11-06-branch build on Win 2000.

Both above test cases work as expected.

Comment 42

[bsharma]

Updating the verified1.0.2 keyword.