Line breaks in document.title cause entire title to disappear

RESOLVED FIXED

Status

()

Core
Security
--
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: unreal, Assigned: dveditz)

Tracking

({fixed1.8.1, verified1.8.0.2})

1.0 Branch
x86
Windows XP
fixed1.8.1, verified1.8.0.2
Points:
---
Bug Flags:
blocking-aviary1.0.8 -
blocking-aviary1.0.9 ?
blocking1.8.0.1 -
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low spoof][rft-dl])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)

Simular to the bug I posted here( https://bugzilla.mozilla.org/show_bug.cgi?id=320937 ) this also hides the titlebar text completely. Using javascript characters such as \n for a new line or \r for a carriage return everything gets cut off. 
EXAPLE:
document.title = 'Titlebar is not visable \n \n \n \r \r \r'

Reproducible: Always

Steps to Reproduce:
1.Make a new document ( the protocal "javascript:" won't work) and test uising example above

Actual Results:  
Everything, evan the " - Mozilla Firefox" addon is removed
(Reporter)

Comment 1

12 years ago
Created attachment 206447 [details]
Example

I added an example as an attachment above.
(Reporter)

Comment 2

12 years ago
sorry, used wrong user agent. Browser used was: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Component: General → Security
(Assignee)

Comment 3

12 years ago
This scrolls the entire contents out of the title bar, at least on windows. Although the attacker couldn't supply their own text (the whole thing scrolls out, regardless of where any attacker-supplied text is in relation to the newlines), this *could* be used to hide our true-site prefix on popup windows without a location bar.

This would be a minor aide to spoofing a site by removing a contradictory indicator. A secure site would still show the real location in the status bar so it wouldn't be totally effective for financial sites.

I suspect this is platform-specific, will have to test. Josh or Jesse, can you test on Mac?
Assignee: nobody → dveditz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8.1+
Flags: blocking1.8.0.1?
Flags: blocking-aviary1.0.8?
Whiteboard: [sg:spoof]

Comment 4

12 years ago
On Mac, the title is part of the body of the dialog.  Both the hostname and the site-supplied title are bold.  Line breaks in the site-supplied title are treated as spaces.  A really long title pushes off the "OK" and "Cancel" buttons, and might distract from or contradict the hostname, but the real hostname is always shown first and is always visible.
(Assignee)

Updated

12 years ago
Blocks: 320937
(Assignee)

Updated

12 years ago
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-

Comment 5

12 years ago
We will reconsider for 1.0.x when a fix is available.

Comment 6

12 years ago
We need more investigation and a fex before we can plus it.
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2-
Flags: blocking-aviary1.0.9?
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8-
(Assignee)

Updated

12 years ago
Flags: blocking1.8.0.2- → blocking1.8.0.2?
(Assignee)

Updated

12 years ago
Flags: blocking1.8.0.2? → blocking1.8.0.2+
(Assignee)

Comment 7

12 years ago
Created attachment 213298 [details] [diff] [review]
strip CR/LF from titles on windows
(Assignee)

Comment 8

12 years ago
*** Bug 320937 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Attachment #213298 - Flags: superreview?(jst)
Attachment #213298 - Flags: review?(dougt)
Attachment #213298 - Flags: approval1.8.0.2?
Attachment #213298 - Flags: approval-branch-1.8.1?(jst)

Comment 9

12 years ago
Is this just we so don't use the title bar of someone mistakenly adds a \n or \r?  is so, this patch is fine.

I am not sure why we don't also test for cases like:

document.title = <1000's of spaces> The titlebar is now invisable';
Comment on attachment 213298 [details] [diff] [review]
strip CR/LF from titles on windows

Linux needs a similar fix as well (the testcase makes the " - Firefox" part of the title not show up), so that brings me to think we'd want to do this in a central place so all platforms get this, even if they don't strictly need it.
Attachment #213298 - Flags: superreview?(jst)
Attachment #213298 - Flags: superreview-
Attachment #213298 - Flags: approval-branch-1.8.1?(jst)
Attachment #213298 - Flags: approval-branch-1.8.1-
(Assignee)

Comment 11

12 years ago
> [on Linux] the testcase makes the " - Firefox" part of the title not show up

Can't anyone do that just by making a really long title of NBSP? The worrying bit on windows was that the whole title goes away, including the site prefix on alert dialogs and locationbar-less popups.

(In reply to comment #9)
> Is this just we so don't use the title bar of someone mistakenly adds a \n or
> \r?  is so, this patch is fine.

Maliciously, because on windows XP Luna theme each \n anywhere in the title seems to make the whole thing scroll up about 1/3 of a line.

> I am not sure why we don't also test for cases like:
> document.title = <1000's of spaces> The titlebar is now invisable';

If the page author wants an empty title that's fine. Adding spaces won't get rid of any hostname prefix we've added for spoof prevention. The carriage returns did.
(Assignee)

Comment 12

12 years ago
Created attachment 213412 [details] [diff] [review]
extend to all platforms

I'm happier making this cross-platform. Probably it does nothing, but should there ever be another platform sensitive to this (and note that windows is or isn't dependent on the system theme) then this will catch it.

This also prevents having to make another copy of the title unlike the windows-specific patch. We're already making a copy in nsXULWindow.
Attachment #213298 - Attachment is obsolete: true
Attachment #213412 - Flags: superreview?(jst)
Attachment #213412 - Flags: review?
Attachment #213412 - Flags: approval1.8.0.2?
Attachment #213412 - Flags: approval-branch-1.8.1?(jst)
Attachment #213298 - Flags: review?(dougt)
Attachment #213298 - Flags: approval1.8.0.2?
(Assignee)

Updated

12 years ago
Component: Security → Security
Flags: review?
Product: Firefox → Core
Version: unspecified → 1.0 Branch
(Assignee)

Updated

12 years ago
Attachment #213412 - Flags: review?(dougt)

Comment 13

12 years ago
Created attachment 213428 [details]
spaces testcase

this is another example based on the first attachment.  Instead of using lf/cr's, we use a title string that starts with 1000 spaces.  On windows, this results in a title bar that is mostly blank and has a trailing elipse "..." on the far right side.

This seams to be a similar probably as was reported.  I think that we might want to make the widget code know about this problem and prevent setting the window title to something that would result in an empty title.
Comment on attachment 213412 [details] [diff] [review]
extend to all platforms

sr=jst for this piece of the change.

But couldn't we simply make the same piece of code that now strips out \n and \r also strip leading and trailing whitespace, and collapse whitespace into a single space (i.e. s/\s+/ /g on the string)?

Also, is this someting we'd want to do for embedders too? If so, we'd probably need to do this elsewhere as well.
Attachment #213412 - Flags: superreview?(jst) → superreview+
(Assignee)

Updated

12 years ago
Attachment #213428 - Attachment is patch: false
Attachment #213428 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 15

12 years ago
Created attachment 213545 [details]
testcase showing the difference

There is a big difference between a blank title (don't care, <title></title> is not a crime) and a title that is able to remove our anti-spoofing markers. Removing the "- Firefox" suffix isn't important (I think we don't even show that on Mac) it's just there for branding.

Comment 16

12 years ago
Comment on attachment 213412 [details] [diff] [review]
extend to all platforms

this change is fine if its intent is soley to fix the prompt() problem.  I think windows w/o location bars can still have their title bars hacked off.

jst -- when I said that change should be in gfx, i take that back.  the thinking there was that only the GFX knows how much text it can stuff into a title bar without running content out of view.
Attachment #213412 - Flags: review?(dougt) → review+
(Assignee)

Comment 17

12 years ago
Fix checked into trunk and 1.8 branch
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Whiteboard: [sg:spoof] → [sg:low spoof]

Comment 18

12 years ago
Comment on attachment 213412 [details] [diff] [review]
extend to all platforms

a=timr
Attachment #213412 - Flags: approval1.8.0.2? → approval1.8.0.2+
(Assignee)

Comment 19

12 years ago
fix checked into the 1.8.0 branch
Keywords: fixed1.8.0.2

Updated

12 years ago
Whiteboard: [sg:low spoof] → [sg:low spoof][rft-dl]
verified on Firefox 1.5.0.2 Windows from 20060306
Keywords: fixed1.8.0.2 → verified1.8.0.2

Updated

12 years ago
Attachment #213412 - Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
(Assignee)

Updated

12 years ago
Group: security

Updated

12 years ago
Summary: Hides titlebar text completely using javscript new line or carriage return → Line break in document.title causes entire title to disappear
(Assignee)

Updated

12 years ago
Summary: Line break in document.title causes entire title to disappear → Line breaks in document.title cause entire title to disappear
You need to log in before you can comment on or make changes to this bug.