Last Comment Bug 391043 - (CVE-2007-5334) Content can hide the window's titlebar
(CVE-2007-5334)
: Content can hide the window's titlebar
Status: RESOLVED FIXED
[sg:low spoof]
: testcase, verified1.8.0.14, verified1.8.1.8
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Martijn Wargers [:mwargers] (not working for Mozilla)
:
: Neil Deakin
Mentors:
Depends on:
Blocks: 393775
  Show dependency treegraph
 
Reported: 2007-08-05 23:32 PDT by Eli Friedman
Modified: 2008-07-31 03:24 PDT (History)
12 users (show)
dveditz: blocking1.8.1.8+
jwalden+bmo: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (149 bytes, application/vnd.mozilla.xul+xml)
2007-08-05 23:32 PDT, Eli Friedman
no flags Details
patch (2.21 KB, patch)
2007-08-14 08:22 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
patch? (1.02 KB, patch)
2007-08-16 11:12 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
jst: review+
Details | Diff | Splinter Review
Tests I did (2.15 KB, application/zip)
2007-08-16 11:24 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patchv2 (1.36 KB, patch)
2007-08-20 09:21 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
bzbarsky: superreview+
dbaron: approval1.9+
Details | Diff | Splinter Review
branch patch (1.36 KB, patch)
2007-08-21 00:15 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
branch patch (3.50 KB, patch)
2007-08-21 07:35 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
bzbarsky: review+
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.14+
Details | Diff | Splinter Review

Description Eli Friedman 2007-08-05 23:32:04 PDT
Created attachment 275391 [details]
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) 2007-08-05 23:45:37 PDT
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 2007-08-05 23:47:09 PDT
Yeah; it doesn't apply to Mac because HideWindowChrome isn't implemented on Mac.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-14 08:22:39 PDT
Created attachment 276638 [details] [diff] [review]
patch
Comment 4 Eli Friedman 2007-08-14 09:18:38 PDT
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 [:bz] (still a bit busy) 2007-08-14 10:30:15 PDT
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 2007-08-14 10:47:25 PDT
(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 [:mwargers] (not working for Mozilla) 2007-08-16 11:12:01 PDT
Created attachment 276977 [details] [diff] [review]
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 [:mwargers] (not working for Mozilla) 2007-08-16 11:24:47 PDT
Created attachment 276980 [details]
Tests I did

Here are some testcase I made. They need to be run from chrome.
Comment 9 Eli Friedman 2007-08-16 11:29:19 PDT
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 [:mwargers] (not working for Mozilla) 2007-08-16 11:33:12 PDT
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) 2007-08-16 22:27:46 PDT
Comment on attachment 276977 [details] [diff] [review]
patch?

I'm not a content peer, actually, either.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2007-08-17 15:19:12 PDT
Comment on attachment 276977 [details] [diff] [review]
patch?

r=jst
Comment 13 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-17 23:15:45 PDT
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 [:bz] (still a bit busy) 2007-08-19 12:27:03 PDT
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 [:mwargers] (not working for Mozilla) 2007-08-20 09:21:21 PDT
Created attachment 277405 [details] [diff] [review]
patchv2

Ok, like this, right?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2007-08-20 09:33:34 PDT
Comment on attachment 277405 [details] [diff] [review]
patchv2

Yeah, that looks great!
Comment 17 David Baron :dbaron: ⌚️UTC-10 2007-08-20 11:58:08 PDT
Comment on attachment 277405 [details] [diff] [review]
patchv2

a1.9=dbaron
Comment 18 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-20 23:45:37 PDT
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 [:mwargers] (not working for Mozilla) 2007-08-21 00:15:37 PDT
Created attachment 277504 [details] [diff] [review]
branch patch
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2007-08-21 00:21:54 PDT
I'm pretty sure nsPresContext::IsChrome is trunk-only...
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2007-08-21 00:22:17 PDT
As in, you'd just need to copy that code on branch.
Comment 22 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-21 01:00:50 PDT
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 [:mwargers] (not working for Mozilla) 2007-08-21 07:35:53 PDT
Created attachment 277531 [details] [diff] [review]
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 [:bz] (still a bit busy) 2007-08-21 12:15:43 PDT
Comment on attachment 277531 [details] [diff] [review]
branch patch

Sure.
Comment 25 Daniel Veditz [:dveditz] 2007-08-29 14:52:28 PDT
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 [:mwargers] (not working for Mozilla) 2007-08-29 16:08:34 PDT
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] 2007-09-03 15:18:24 PDT
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 2007-10-23 13:53:44 PDT
This looks like the likely cause of Bug 400744.
Comment 29 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-23 13:58:55 PDT
(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 2007-10-23 14:10:34 PDT
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] 2007-12-10 16:26:49 PST
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

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