Last Comment Bug 171274 - URL bar spoofing using XUL <browser type="content-primary">
: URL bar spoofing using XUL <browser type="content-primary">
Status: VERIFIED FIXED
[sg:blocker],nsbeta1+
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: jag (Peter Annema)
: bsharma
Mentors:
Depends on:
Blocks: blackbird 1.2
  Show dependency treegraph
 
Reported: 2002-09-27 14:02 PDT by Mitchell Stoltz (not reading bugmail)
Modified: 2003-02-10 17:08 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Example XUL page (798 bytes, application/vnd.mozilla.xul+xml)
2002-09-27 14:02 PDT, Mitchell Stoltz (not reading bugmail)
no flags Details
Only accept content-primary for chrome (2.25 KB, patch)
2002-10-22 08:58 PDT, jag (Peter Annema)
caillon: review+
jst: superreview+
dbaron: approval+
Details | Diff | Review
Fix (1.70 KB, patch)
2002-10-24 14:52 PDT, jag (Peter Annema)
dbaron: review+
jst: superreview+
dbaron: approval+
Details | Diff | Review
Branch fix (1.23 KB, patch)
2003-01-09 15:25 PST, jag (Peter Annema)
security-bugs: review+
bryner: superreview+
dbaron: approval1.0.x+
Details | Diff | Review

Description Mitchell Stoltz (not reading bugmail) 2002-09-27 14:02:01 PDT
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) 2002-09-27 14:02:43 PDT
Created attachment 100923 [details]
Example XUL page
Comment 2 Daniel Veditz [:dveditz] 2002-09-27 14:47:56 PDT
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) (Exited; email my personal email if necessary) 2002-10-15 09:47:37 PDT
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) 2002-10-18 16:01:53 PDT
caillon, jag, hyatt, can one of you take this on?
Comment 5 jag (Peter Annema) 2002-10-22 06:22:11 PDT
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) 2002-10-22 08:30:03 PDT
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) 2002-10-22 08:50:47 PDT
There we go. I think I got it now.
Comment 8 jag (Peter Annema) 2002-10-22 08:58:35 PDT
Created attachment 103726 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2002-10-22 09:07:09 PDT
Great! Let's drive this in.
Comment 10 David Hyatt 2002-10-22 13:17:18 PDT
sr=hyatt
Comment 11 Christopher Aillon (sabbatical, not receiving bugmail) 2002-10-22 17:18:21 PDT
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, jst@mozilla.com) 2002-10-23 10:35:17 PDT
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) 2002-10-23 12:43:42 PDT
I thought the code spoke for itself, but ...

// if it isn't content, set it to the parent's type
Comment 14 Michael Buckland 2002-10-23 15:56:01 PDT
discussed with dan and jag.  plussing for adt and adding nsbeta stuff, linking
to meta.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-10-23 19:00:24 PDT
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) 2002-10-24 13:07:32 PDT
This caused a regression - bug #176505 - and was backed out.
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-10-24 13:42:26 PDT
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) 2002-10-24 14:39:59 PDT
Duh. You wanna make sure the parent is chrome, not the shell itself. Coming up
with a new patch.
Comment 19 jag (Peter Annema) 2002-10-24 14:52:02 PDT
Created attachment 104037 [details] [diff] [review]
Fix
Comment 20 jag (Peter Annema) 2002-10-24 14:52:39 PDT
Thanks, dbaron.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-10-24 14:54:51 PDT
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, jst@mozilla.com) 2002-10-24 15:29:44 PDT
Comment on attachment 104037 [details] [diff] [review]
Fix

sr=jst
Comment 23 jag (Peter Annema) 2002-10-24 15:45:05 PDT
dbaron: changed. Does a= still stand?
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-10-24 15:56:17 PDT
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) 2002-10-24 16:11:57 PDT
That's the plan :-)
Comment 26 chris hofmann 2002-10-31 13:53:20 PST
ready for the branch?
Comment 27 jag (Peter Annema) 2002-10-31 17:19:23 PST
Yes, very ready :-) Mail was sent to drivers a couple of days ago, hopefully
they'll reach it soon.
Comment 28 Christopher Blizzard (:blizzard) 2002-11-01 09:02:33 PST
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) 2002-11-01 14:49:31 PST
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) 2002-11-04 04:37:31 PST
It got checked in on the trunk. I just checked it in on the branch. Thanks.
Comment 31 bsharma 2002-11-06 13:41:56 PST
Verified on 2002-11-06-branch on Win 2000
Comment 32 bsharma 2002-11-06 13:43:31 PST
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) 2003-01-08 17:50:41 PST
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) 2003-01-09 15:25:36 PST
Created attachment 111108 [details] [diff] [review]
Branch fix
Comment 35 Mitchell Stoltz (not reading bugmail) 2003-01-09 16:22:05 PST
Comment on attachment 111108 [details] [diff] [review]
Branch fix

r=mstoltz
Comment 36 Brian Ryner (not reading) 2003-01-09 19:35:23 PST
Comment on attachment 111108 [details] [diff] [review]
Branch fix

sr=bryner
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-01-10 15:26:44 PST
Comment on attachment 111108 [details] [diff] [review]
Branch fix

a=dbaron for 1.0.x branch checkin
Comment 38 jag (Peter Annema) 2003-01-10 20:03:32 PST
Checked in.
Comment 39 bmartin 2003-02-10 17:08:06 PST
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

Note You need to log in before you can comment on or make changes to this bug.