321013 - Line breaks in document.title cause entire title to disappear

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[unreal]

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

Comment 1

[unreal]

Attachment: Example

I added an example as an attachment above.

Comment 2

[unreal]

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

Comment 3

[Daniel Veditz [:dveditz]]

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?

Comment 4

[Jesse Ruderman]

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.

Comment 5

[Tim Riley [:timr]]

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

Comment 6

[Tim Riley [:timr]]

We need more investigation and a fex before we can plus it.

Comment 7

[Daniel Veditz [:dveditz]]

Attachment: strip CR/LF from titles on windows

Comment 8

[Daniel Veditz [:dveditz]]

*** Bug 320937 has been marked as a duplicate of this bug. ***

Comment 9

[Doug Turner (:dougt)]

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 10

[Johnny Stenback (:jst)]

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.

Comment 11

[Daniel Veditz [:dveditz]]

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

Comment 12

[Daniel Veditz [:dveditz]]

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

Comment 13

[Doug Turner (:dougt)]

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

[Johnny Stenback (:jst)]

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.

Comment 15

[Daniel Veditz [:dveditz]]

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

[Doug Turner (:dougt)]

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.

Comment 17

[Daniel Veditz [:dveditz]]

Fix checked into trunk and 1.8 branch

Comment 18

[Tim Riley [:timr]]

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

a=timr

Comment 19

[Daniel Veditz [:dveditz]]

fix checked into the 1.8.0 branch

Comment 20

[Tracy Walker [:tracy]]

verified on Firefox 1.5.0.2 Windows from 20060306