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

VERIFIED FIXED

Status

()

Core
Security
--
major
VERIFIED FIXED
15 years ago
14 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: jag (Peter Annema))

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 1 obsolete attachment)

798 bytes, application/vnd.mozilla.xul+xml
Details
Fix
1.70 KB, patch
dbaron
: review+
dbaron
: approval+
Details | Diff | Splinter Review
1.23 KB, patch
Mitchell Stoltz (not reading bugmail)
: review+
Brian Ryner (not reading)
: superreview+
Details | Diff | Splinter Review
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?
(Reporter)

Comment 1

15 years ago
Created attachment 100923 [details]
Example XUL page
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]
(Reporter)

Comment 4

15 years ago
caillon, jag, hyatt, can one of you take this on?
(Assignee)

Comment 5

15 years ago
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
(Assignee)

Comment 6

15 years ago
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.
(Assignee)

Comment 7

15 years ago
There we go. I think I got it now.
(Assignee)

Comment 8

15 years ago
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.
Great! Let's drive this in.
Blocks: 174647

Comment 10

15 years ago
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+
(Assignee)

Comment 13

15 years ago
I thought the code spoke for itself, but ...

// if it isn't content, set it to the parent's type
Blocks: 168047
Keywords: adt1.0.2

Comment 14

15 years ago
discussed with dan and jag.  plussing for adt and adding nsbeta stuff, linking
to meta.
Keywords: adt1.0.2 → adt1.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+
Keywords: mozilla1.0.2+
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());
       }
     }
(Assignee)

Comment 18

15 years ago
Duh. You wanna make sure the parent is chrome, not the shell itself. Coming up
with a new patch.
(Assignee)

Comment 19

15 years ago
Created attachment 104037 [details] [diff] [review]
Fix
Attachment #103726 - Attachment is obsolete: true
(Assignee)

Comment 20

15 years ago
Thanks, dbaron.
Attachment #104037 - Flags: review+
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+
(Assignee)

Comment 23

15 years ago
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+
(Assignee)

Comment 25

15 years ago
That's the plan :-)

Comment 26

15 years ago
ready for the branch?
(Assignee)

Comment 27

15 years ago
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.
(Assignee)

Comment 30

15 years ago
It got checked in on the trunk. I just checked it in on the branch. Thanks.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Keywords: mozilla1.0.2+ → fixed1.0.2

Comment 31

15 years ago
Verified on 2002-11-06-branch on Win 2000
Keywords: fixed1.0.2 → verified1.0.2

Comment 32

15 years ago
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
(Reporter)

Updated

15 years ago
Group: security
(Reporter)

Comment 33

15 years ago
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 → ---
(Assignee)

Comment 34

15 years ago
Created attachment 111108 [details] [diff] [review]
Branch fix
(Assignee)

Updated

15 years ago
Attachment #111108 - Flags: superreview?(jst)
Attachment #111108 - Flags: review?(dbaron)
Attachment #111108 - Flags: approval1.0.x?
(Reporter)

Comment 35

15 years ago
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+
(Assignee)

Comment 38

15 years ago
Checked in.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 39

14 years ago
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.