391043 - (CVE-2007-5334) Content can hide the window's titlebar

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Eli Friedman]

Attachment: 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...

Comment 1

[Samuel Sidler (old account; do not CC)]

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.)

Comment 2

[Eli Friedman]

Yeah; it doesn't apply to Mac because HideWindowChrome isn't implemented on Mac.

Comment 3

[Martijn Wargers (dead)]

Attachment: patch

Comment 4

[Eli Friedman]

I don't think we can trust the caller principal here; this isn't getting called by content directly. bz, any comments?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Eli Friedman]

(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.

Comment 7

[Martijn Wargers (dead)]

Attachment: patch?

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

Comment 8

[Martijn Wargers (dead)]

Attachment: Tests I did

Here are some testcase I made. They need to be run from chrome.

Comment 9

[Eli Friedman]

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 10

[Martijn Wargers (dead)]

Comment on attachment 276977 [details] [diff] [review]
patch?

Ok, thanks, I'll ask roc then.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 276977 [details] [diff] [review]
patch?

I'm not a content peer, actually, either.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 276977 [details] [diff] [review]
patch?

r=jst

Comment 13

[Martijn Wargers (dead)]

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.

Comment 14

[Boris Zbarsky [: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().

Comment 15

[Martijn Wargers (dead)]

Attachment: patchv2

Ok, like this, right?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277405 [details] [diff] [review]
patchv2

Yeah, that looks great!

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 277405 [details] [diff] [review]
patchv2

a1.9=dbaron

Comment 18

[Martijn Wargers (dead)]

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.

Comment 19

[Martijn Wargers (dead)]

Attachment: branch patch

Comment 20

[Boris Zbarsky [:bzbarsky]]

I'm pretty sure nsPresContext::IsChrome is trunk-only...

Comment 21

[Boris Zbarsky [:bzbarsky]]

As in, you'd just need to copy that code on branch.

Comment 22

[Martijn Wargers (dead)]

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.

Comment 23

[Martijn Wargers (dead)]

Attachment: branch patch

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277531 [details] [diff] [review]
branch patch

Sure.

Comment 25

[Daniel Veditz [:dveditz]]

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

Comment 26

[Martijn Wargers (dead)]

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.

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[John M]

This looks like the likely cause of Bug 400744.

Comment 29

[Martijn Wargers (dead)]

(In reply to comment #28)
> This looks like the likely cause of Bug 400744.

That seems unlikely to me. What makes you think that?

Comment 30

[John M]

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.

Comment 31

[Stephen Donner [:stephend] Not actively reading bugmail]

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