Closed Bug 171274 Opened 22 years ago Closed 22 years ago

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

Categories

(Core :: Security, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: security-bugs, Assigned: jag+mozilla)

References

Details

(Whiteboard: [sg:blocker],nsbeta1+)

Attachments

(3 files, 1 obsolete file)

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?
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.
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.
Whiteboard: [sg:blocker]
caillon, jag, hyatt, can one of you take this on?
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).
Assignee: mstoltz → jaggernaut
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.
There we go. I think I got it now.
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.
sr=hyatt
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.
Attachment #103726 - Flags: review+
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
Attachment #103726 - Flags: superreview+
I thought the code spoke for itself, but ...

// if it isn't content, set it to the parent's type
Keywords: adt1.0.2
discussed with dan and jag.  plussing for adt and adding nsbeta stuff, linking
to meta.
Keywords: adt1.0.2adt1.0.2+
Whiteboard: [sg:blocker] → [sg:blocker],nsbeta1+
Comment on attachment 103726 [details] [diff] [review]
Only accept content-primary for chrome

a=dbaron for trunk and 1.0 branch
Attachment #103726 - Flags: approval+
This caused a regression - bug #176505 - and was backed out.
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());
       }
     }
Duh. You wanna make sure the parent is chrome, not the shell itself. Coming up
with a new patch.
Attached patch FixSplinter Review
Attachment #103726 - Attachment is obsolete: true
Thanks, dbaron.
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 on attachment 104037 [details] [diff] [review]
Fix

sr=jst
Attachment #104037 - Flags: superreview+
dbaron: changed. Does a= still stand?
Comment on attachment 104037 [details] [diff] [review]
Fix

a=dbaron for trunk checkin.  (Ask again for branch in a day or two, perhaps?)
Attachment #104037 - Flags: approval+
That's the plan :-)
ready for the branch?
Yes, very ready :-) Mail was sent to drivers a couple of days ago, hopefully
they'll reach it soon.
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.
Did this get checked in on the trunk or not?  I can't tell from the comments in
the bug.
It got checked in on the trunk. I just checked it in on the branch. Thanks.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Verified on 2002-11-06-branch on Win 2000
Verified on 2002-11-06-branch on Win 2000.

Attached test case throws an exception in JS console. This is the correct behavior.
Status: RESOLVED → VERIFIED
Group: security
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Attached patch Branch fixSplinter Review
Attachment #111108 - Flags: superreview?(jst)
Attachment #111108 - Flags: review?(dbaron)
Attachment #111108 - Flags: approval1.0.x?
Comment on attachment 111108 [details] [diff] [review]
Branch fix

r=mstoltz
Attachment #111108 - Flags: review?(dbaron) → review+
Comment on attachment 111108 [details] [diff] [review]
Branch fix

sr=bryner
Attachment #111108 - Flags: superreview?(jst) → superreview+
Comment on attachment 111108 [details] [diff] [review]
Branch fix

a=dbaron for 1.0.x branch checkin
Attachment #111108 - Flags: approval1.0.x? → approval1.0.x+
Checked in.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: