Bug 391043 (CVE-2007-5334)

Content can hide the window's titlebar

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Eli Friedman, Assigned: Martijn Wargers (dead))

Tracking

({testcase, verified1.8.0.14, verified1.8.1.8})

Trunk
testcase, verified1.8.0.14, verified1.8.1.8
Points:
---
Bug Flags:
blocking1.8.1.8 +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low spoof])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 2

10 years ago
Yeah; it doesn't apply to Mac because HideWindowChrome isn't implemented on Mac.
(Assignee)

Comment 3

10 years ago
Created attachment 276638 [details] [diff] [review]
patch
Attachment #276638 - Flags: review?(sharparrow1)
(Reporter)

Comment 4

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

Updated

10 years ago
Attachment #276638 - Flags: review?(sharparrow1)
(Reporter)

Comment 6

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

Comment 7

10 years ago
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
Attachment #276638 - Attachment is obsolete: true
Attachment #276977 - Flags: review?(sharparrow1)
(Assignee)

Comment 8

10 years ago
Created attachment 276980 [details]
Tests I did

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

Comment 9

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

Comment 10

10 years ago
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+

Updated

10 years ago
Flags: blocking1.9?
(Assignee)

Comment 13

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

Comment 15

10 years ago
Created attachment 277405 [details] [diff] [review]
patchv2

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

Updated

10 years ago
Attachment #277405 - Flags: approval1.9?
Comment on attachment 277405 [details] [diff] [review]
patchv2

a1.9=dbaron
Attachment #277405 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 18

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

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

10 years ago
Created attachment 277504 [details] [diff] [review]
branch patch
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.
(Assignee)

Comment 22

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

Comment 23

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

Updated

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

Comment 26

10 years ago
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?
Keywords: fixed1.8.0.14, fixed1.8.1.7
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
Keywords: fixed1.8.1.7 → testcase, verified1.8.1.7

Updated

10 years ago
Alias: CVE-2007-5334
Group: security
Flags: in-litmus?

Comment 28

10 years ago
This looks like the likely cause of Bug 400744.
(Assignee)

Comment 29

10 years ago
(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

10 years ago
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
Keywords: fixed1.8.0.14 → verified1.8.0.14

Updated

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