318 bytes, text/html
675 bytes, patch
|Details | Diff | Splinter Review|
358 bytes, text/html
1.35 KB, text/html
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
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?
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.
We will reconsider for 1.0.x when a fix is available.
We need more investigation and a fex before we can plus it.
*** Bug 320937 has been marked as a duplicate of this bug. ***
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.
> [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.
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.
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.
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 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.
Fix checked into trunk and 1.8 branch
Comment on attachment 213412 [details] [diff] [review] extend to all platforms a=timr
fix checked into the 1.8.0 branch
verified on Firefox 184.108.40.206 Windows from 20060306