Closed
Bug 171274
Opened 22 years ago
Closed 22 years ago
URL bar spoofing using XUL <browser type="content-primary">
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
VERIFIED
FIXED
People
(Reporter: security-bugs, Assigned: jag+mozilla)
References
Details
(Whiteboard: [sg:blocker],nsbeta1+)
Attachments
(3 files, 1 obsolete file)
798 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
1.70 KB,
patch
|
dbaron
:
review+
jst
:
superreview+
dbaron
:
approval+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
security-bugs
:
review+
bryner
:
superreview+
dbaron
:
approval1.0.x+
|
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•22 years ago
|
||
Comment 2•22 years ago
|
||
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•22 years ago
|
||
caillon, jag, hyatt, can one of you take this on?
Assignee | ||
Comment 5•22 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•22 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•22 years ago
|
||
There we go. I think I got it now.
Assignee | ||
Comment 8•22 years ago
|
||
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: 1.2
Comment 10•22 years ago
|
||
sr=hyatt
Comment 11•22 years ago
|
||
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 12•22 years ago
|
||
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•22 years ago
|
||
I thought the code spoke for itself, but ... // if it isn't content, set it to the parent's type
Comment 14•22 years ago
|
||
discussed with dan and jag. plussing for adt and adding nsbeta stuff, linking to meta.
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+
Comment 16•22 years ago
|
||
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•22 years ago
|
||
Duh. You wanna make sure the parent is chrome, not the shell itself. Coming up with a new patch.
Assignee | ||
Comment 19•22 years ago
|
||
Attachment #103726 -
Attachment is obsolete: true
Assignee | ||
Comment 20•22 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 22•22 years ago
|
||
Comment on attachment 104037 [details] [diff] [review] Fix sr=jst
Attachment #104037 -
Flags: superreview+
Assignee | ||
Comment 23•22 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•22 years ago
|
||
That's the plan :-)
Comment 26•22 years ago
|
||
ready for the branch?
Assignee | ||
Comment 27•22 years ago
|
||
Yes, very ready :-) Mail was sent to drivers a couple of days ago, hopefully they'll reach it soon.
Comment 28•22 years ago
|
||
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•22 years ago
|
||
Did this get checked in on the trunk or not? I can't tell from the comments in the bug.
Assignee | ||
Comment 30•22 years ago
|
||
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
Updated•22 years ago
|
Keywords: mozilla1.0.2+ → fixed1.0.2
Comment 31•22 years ago
|
||
Verified on 2002-11-06-branch on Win 2000
Keywords: fixed1.0.2 → verified1.0.2
Comment 32•22 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•22 years ago
|
Group: security
Reporter | ||
Comment 33•22 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•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #111108 -
Flags: superreview?(jst)
Attachment #111108 -
Flags: review?(dbaron)
Attachment #111108 -
Flags: approval1.0.x?
Reporter | ||
Comment 35•22 years ago
|
||
Comment on attachment 111108 [details] [diff] [review] Branch fix r=mstoltz
Attachment #111108 -
Flags: review?(dbaron) → review+
Comment 36•22 years ago
|
||
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•22 years ago
|
||
Checked in.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 39•21 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.
Description
•