171274 - URL bar spoofing using XUL <browser type="content-primary">

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

From Georgi:
It is possible to spoof the window location with XUL with the browser widget.
Check attached file xulspoof.xul which spoofs www.mozilla.org
The browser XUL widget with content-primary may introduce other bugs in the
future, is it really needed from userland XUL?

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Attachment: Example XUL page

Comment 2

[Daniel Veditz [:dveditz]]

FYI whoever tests this: does not appear to work if you open the page in a new
tab, but I'm spoofed if I open it in a new window.

Comment 3

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

Yeah, we should not respect content-primary if it's inside a content webshell.
This is quite serious and easy to exploit --- marking blocker for 1.2.

Comment 4

[Mitchell Stoltz (not reading bugmail)]

caillon, jag, hyatt, can one of you take this on?

Comment 5

[jag (Peter Annema)]

So somewhere around here we need to add an isChrome check:

http://lxr.mozilla.org/seamonkey/source/layout/html/document/src/nsFrameFrame.cpp#686

Patch coming up (I hope).

Comment 6

[jag (Peter Annema)]

Hmm, so that didn't work. I figured this would work:

@@ -683,7 +683,10 @@
       nsCOMPtr<nsIDocShellTreeOwner> parentTreeOwner;
       parentAsItem->GetTreeOwner(getter_AddRefs(parentTreeOwner));
       if (parentTreeOwner) {
+        PRInt32 shellType;
+        docShellAsItem->GetItemType(&shellType);
         PRBool is_primary_content =
+          shellType == nsIDocShellTreeItem::typeChrome &&
           value.EqualsIgnoreCase("content-primary");
 
         parentTreeOwner->ContentShellAdded(docShellAsItem, is_primary_content,

I'll have to get some more info on what's going on here I guess.

Comment 7

[jag (Peter Annema)]

There we go. I think I got it now.

Comment 8

[jag (Peter Annema)]

Attachment: Only accept content-primary for chrome

So it's the patch to nsFrameLoader.cpp that fixed this. I think we should also
have the check in nsFrameFrame.cpp, though I'm not quite sure (read: tired and
don't wanna work it out right now) how we could trigger the same problem over
there.

Comment 9

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

Great! Let's drive this in.

Comment 10

[David Hyatt]

sr=hyatt

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 103726 [details] [diff] [review]
Only accept content-primary for chrome

r=caillon provided you tested this.  I had a brief look at this a couple weeks
ago and tried similar patches, but got a bunch of JS errors and warnings about
window.content when loading pages.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 103726 [details] [diff] [review]
Only accept content-primary for chrome

- In nsFrameLoader.cpp:

-    if (isContent) {
-      // The web shell's type is content.
-
-      docShellAsItem->SetItemType(nsIDocShellTreeItem::typeContent);
-    } else {
-      // Inherit our type from our parent webshell.  If it is
-      // chrome, we'll be chrome.  If it is content, we'll be
-      // content.
-
-      docShellAsItem->SetItemType(parentType);
-    }
+    PRInt32 shellType = isContent ? nsIDocShellTreeItem::typeContent :
parentType;
+    docShellAsItem->SetItemType(shellType);

Leave the comment, in one form or another.

sr=jst

Comment 13

[jag (Peter Annema)]

I thought the code spoke for itself, but ...

// if it isn't content, set it to the parent's type

Comment 14

[Michael Buckland]

discussed with dan and jag.  plussing for adt and adding nsbeta stuff, linking
to meta.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 103726 [details] [diff] [review]
Only accept content-primary for chrome

a=dbaron for trunk and 1.0 branch

Comment 16

[Christopher Blizzard (:blizzard)]

This caused a regression - bug #176505 - and was backed out.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This fixes the regression for me:

Index: nsFrameLoader.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsFrameLoader.cpp,v
retrieving revision 1.13
diff -u -6 -d -r1.13 nsFrameLoader.cpp
--- nsFrameLoader.cpp   24 Oct 2002 04:37:33 -0000      1.13
+++ nsFrameLoader.cpp   24 Oct 2002 20:28:53 -0000
@@ -458,13 +458,13 @@

     if (isContent) {
       nsCOMPtr<nsIDocShellTreeOwner> parentTreeOwner;
       parentAsItem->GetTreeOwner(getter_AddRefs(parentTreeOwner));

       if(parentTreeOwner) {
-        PRBool is_primary = shellType == nsIDocShellTreeItem::typeChrome &&
+        PRBool is_primary = parentType == nsIDocShellTreeItem::typeChrome &&
                             value.Equals(NS_LITERAL_STRING("content-primary"));
         parentTreeOwner->ContentShellAdded(docShellAsItem, is_primary,
                                            value.get());
       }
     }

Comment 18

[jag (Peter Annema)]

Duh. You wanna make sure the parent is chrome, not the shell itself. Coming up
with a new patch.

Comment 19

[jag (Peter Annema)]

Attachment: Fix

Comment 20

[jag (Peter Annema)]

Thanks, dbaron.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 104037 [details] [diff] [review]
Fix

r=dbaron, except it might be clearer for people modifying both functions in the
future if you called the new variable in nsFrameFrame.cpp |parentType| rather
than |shellType|.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 104037 [details] [diff] [review]
Fix

sr=jst

Comment 23

[jag (Peter Annema)]

dbaron: changed. Does a= still stand?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 104037 [details] [diff] [review]
Fix

a=dbaron for trunk checkin.  (Ask again for branch in a day or two, perhaps?)

Comment 25

[jag (Peter Annema)]

That's the plan :-)

Comment 26

[chris hofmann]

ready for the branch?

Comment 27

[jag (Peter Annema)]

Yes, very ready :-) Mail was sent to drivers a couple of days ago, hopefully
they'll reach it soon.

Comment 28

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 1.0.2.  Please mark with fixed1.0.2 when you
have checked it in and verified1.0.2 once it's been verified.

Comment 29

[Christopher Blizzard (:blizzard)]

Did this get checked in on the trunk or not?  I can't tell from the comments in
the bug.

Comment 30

[jag (Peter Annema)]

It got checked in on the trunk. I just checked it in on the branch. Thanks.

Comment 31

[bsharma]

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

Comment 32

[bsharma]

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

Attached test case throws an exception in JS console. This is the correct behavior.

Comment 33

[Mitchell Stoltz (not reading bugmail)]

Reopening - the fix for the 1.0 branch appears to be ineffective. We probably
need a new patch for the 1.0 branch, and jag is working on that.

Comment 34

[jag (Peter Annema)]

Attachment: Branch fix

Comment 35

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 111108 [details] [diff] [review]
Branch fix

r=mstoltz

Comment 36

[Brian Ryner (not reading)]

Comment on attachment 111108 [details] [diff] [review]
Branch fix

sr=bryner

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 111108 [details] [diff] [review]
Branch fix

a=dbaron for 1.0.x branch checkin

Comment 38

[jag (Peter Annema)]

Checked in.

Comment 39

[bmartin]

Marking this Verified Fixed

Testing against latest branch 2002-02-10-09 on Win2000

After running XULspoof testcase in comment #1, the URL location field will not
display http://www.mozilla.org