Closed Bug 391043 (CVE-2007-5334) Opened 13 years ago Closed 13 years ago

Content can hide the window's titlebar

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sharparrow1, Assigned: martijn.martijn)

References

Details

(Keywords: testcase, verified1.8.0.14, verified1.8.1.8, Whiteboard: [sg:low spoof])

Attachments

(4 files, 3 obsolete files)

Attached file Testcase
Per the description: any arbitrary XUL document can hide the window's titlebar without any security checks.

Needless to say, this is very disorienting for a user, and therefore could trick the user into thinking the page is trusted.

The testcase is very simple; it just sets the hidechrome attribute onload.  I assume it wasn't intentional that this is accessible to content...
Flags: blocking1.8.1.7?
Whiteboard: [sg:low spoof]
OS: Windows XP → All
Hardware: PC → All
And by "All", I'm sure Reed meant this bug applies to both trunk and branch on Windows and Linux but not on Mac.

(Tested 2.0.0.6 on all three platforms.)
Yeah; it doesn't apply to Mac because HideWindowChrome isn't implemented on Mac.
Attached patch patch (obsolete) — Splinter Review
Attachment #276638 - Flags: review?(sharparrow1)
I don't think we can trust the caller principal here; this isn't getting called by content directly. bz, any comments?
In the testcase it's being called directly, actually.

But maybe it would be simpler to only do HideChrome handling if our NodePrincipal() has UniversalXPConnect or something?

Alternately, perhaps only the hidechrome attribute on the root document in the window should apply?  That is, XUL documents loaded in the content area, even if they're system, should not affect the window chrome.  That would make the most sense to me.
Attachment #276638 - Flags: review?(sharparrow1)
(In reply to comment #5)
> In the testcase it's being called directly, actually.

Well, yes, but there are other ways to hit this code... it's not reliable.

I agree that it makes sense to check whether the document is the root document in the window.
Attached patch patch? (obsolete) — Splinter Review
So I think this is what is wanted, right?

As a sidenote, this kind of check is on initial load here:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsContainerFrame.cpp#436
Attachment #276638 - Attachment is obsolete: true
Attachment #276977 - Flags: review?(sharparrow1)
Attached file Tests I did
Here are some testcase I made. They need to be run from chrome.
Comment on attachment 276977 [details] [diff] [review]
patch?

This looks correct; I'm not sure if I should be r+ing patches in content, though.
Comment on attachment 276977 [details] [diff] [review]
patch?

Ok, thanks, I'll ask roc then.
Attachment #276977 - Flags: review?(sharparrow1) → review?(roc)
Comment on attachment 276977 [details] [diff] [review]
patch?

I'm not a content peer, actually, either.
Attachment #276977 - Flags: review?(roc) → review?(jst)
Comment on attachment 276977 [details] [diff] [review]
patch?

r=jst
Attachment #276977 - Flags: review?(jst) → review+
Flags: blocking1.9?
Comment on attachment 276977 [details] [diff] [review]
patch?

I am trying to make some mochitests for this bug, but it's bit hard.
I don't think there is a direct way of knowing whether hidechrome succeeded or not, is there?
Otherwise, I guess I need to check with window.innerWidth/outerWidth comparing.
But I'm seeing some oddness (bug 392509) which might make it kind of difficult/unsatisfactory.
Attachment #276977 - Flags: superreview?(bzbarsky)
Comment on attachment 276977 [details] [diff] [review]
patch?

So out of curiousity.... how does this affect non-XUL-UI Gecko browsers (e.g. Camino)?  There every non-frame document has no parent document...  I have to admit to not being able to follow the widget code well enough to tell what happens there, and I would think we want to avoid doing the hiding in those cases.  That is, it might be better to check for a chrome-type docshell in addition to checking that we're the root document.  See nsPresContext::IsChrome().
Attached patch patchv2Splinter Review
Ok, like this, right?
Attachment #276977 - Attachment is obsolete: true
Attachment #277405 - Flags: superreview?(bzbarsky)
Attachment #276977 - Flags: superreview?(bzbarsky)
Attachment #277405 - Attachment is patch: true
Attachment #277405 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 277405 [details] [diff] [review]
patchv2

Yeah, that looks great!
Attachment #277405 - Flags: superreview?(bzbarsky) → superreview+
Attachment #277405 - Flags: approval1.9?
Comment on attachment 277405 [details] [diff] [review]
patchv2

a1.9=dbaron
Attachment #277405 - Flags: approval1.9? → approval1.9+
Checking in nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.c
pp
new revision: 1.727; previous revision: 1.726
done

Checked in.
Assignee: nobody → martijn.martijn
Flags: blocking1.9?
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attached patch branch patch (obsolete) — Splinter Review
Attachment #277504 - Flags: approval1.8.1.7?
Attachment #277504 - Flags: approval1.8.0.14?
I'm pretty sure nsPresContext::IsChrome is trunk-only...
As in, you'd just need to copy that code on branch.
Comment on attachment 277504 [details] [diff] [review]
branch patch

Ah, yeah, I still needed to test the patch.
But I was in the middle of building something else.
Attachment #277504 - Attachment is obsolete: true
Attachment #277504 - Flags: approval1.8.1.7?
Attachment #277504 - Flags: approval1.8.0.14?
Attached patch branch patchSplinter Review
Ok, this works, I tested it on a 1.8.1 tree.
I'm just asking review, to be completely sure.

Btw, fwiw, the non-dynamic testcase doesn't seem to work at all on branch, for some reason.
Attachment #277531 - Flags: review?(bzbarsky)
Attachment #277531 - Flags: approval1.8.1.7?
Attachment #277531 - Flags: approval1.8.0.14?
Comment on attachment 277531 [details] [diff] [review]
branch patch

Sure.
Attachment #277531 - Flags: review?(bzbarsky) → review+
Blocks: 393775
Flags: blocking1.8.1.7?
Flags: blocking1.8.1.7+
Flags: blocking1.8.0.14?
Comment on attachment 277531 [details] [diff] [review]
branch patch

approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers
Attachment #277531 - Flags: approval1.8.1.7?
Attachment #277531 - Flags: approval1.8.1.7+
Attachment #277531 - Flags: approval1.8.0.14?
Attachment #277531 - Flags: approval1.8.0.14+
Checking in content/xul/content/src/nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.c
pp
new revision: 1.578.2.1.2.12; previous revision: 1.578.2.1.2.11
done
Checking in layout/base/nsPresContext.h;
/cvsroot/mozilla/layout/base/nsPresContext.h,v  <--  nsPresContext.h
new revision: 3.150.4.2.4.1; previous revision: 3.150.4.2
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.852.2.11.2.9; previous revision: 3.852.2.11.2.8
done

Fixed on 1.8.0.x branch.

Checking in content/xul/content/src/nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.c
pp
new revision: 1.578.2.25; previous revision: 1.578.2.24
done
Checking in layout/base/nsPresContext.h;
/cvsroot/mozilla/layout/base/nsPresContext.h,v  <--  nsPresContext.h
new revision: 3.150.4.3; previous revision: 3.150.4.2
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.852.2.20; previous revision: 3.852.2.19
done

Fixed on 1.8.1 branch.
Flags: blocking1.8.0.14?
verified fixed 1.8.1.7 using the testcase and:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre) Gecko/2007090308 BonEcho/2.0.0.7pre 

and: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/20070903 BonEcho/2.0.0.7pre ID:2007090304

Titlebar is not hidden in this 1.8.1 builds -> adding verified keyword
Alias: CVE-2007-5334
Group: security
Flags: in-litmus?
This looks like the likely cause of Bug 400744.
(In reply to comment #28)
> This looks like the likely cause of Bug 400744.

That seems unlikely to me. What makes you think that?
Let me retract the previous comment. I thought that the demo URL for 400744 uses XUL, which led me to believe that 391043 was the cause, but on second thought I need to do more investigation before pointing the finger at 391043.
Verified FIXED on 1.8.0.x, using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre

The testcase https://bugzilla.mozilla.org/attachment.cgi?id=275391 doesn't hide my titlebar at all using this build.

Replacing fixed1.8.0.14 keyword with verified1.8.0.14
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.