Closed Bug 167315 Opened 22 years ago Closed 19 years ago

[FIX] TITLE string should be "sandboxed"

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: L.Wood, Assigned: MatsPalmgren_bugz)

References

Details

(4 keywords, Whiteboard: [has patch])

Attachments

(2 files, 3 obsolete files)

WARNING - this testcase could CRASH your OS or Window Manager
146.50 KB, text/html
Details
Patch rev. 3
8.45 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
asa
: approval1.8b4+
Details | Diff | Splinter Review 
Consider the minimum page:

<html>
<head>
<title>this is the title
</head>
<body>
<strong>this is the body and should be shown separately</strong>
</body>
</html>

The title close tag is missing, but this is implicitly closed by the closing
head tag, or even by the opening body tag. (No doctype, so we're in quirks mode
here).

The result in mozilla 1.1 is display of nothing - no body content, no title either.

A more malformed variation on this is the following, with the unclosed title
appearing in malformed body tags.


<html>
<head>
<body>
<title>this is the title line
</body>
this is the run-on body that also appears in the title line
<hr noshade>
</body>
</html>

In this variation, everything that should appear in the page appears instead in
the title bar line - a possible overflow situation? (I don't know how robust
various windowing environments are to being given arbitrarily long window title
strings.) Since the title is in the body (and the opening body tag implicitly
closes the head tag, I think) I was expecting everything to be displayed in the
page, not in the titlebar, and for the title tag to just be ignored.

Mozilla should cope with lack of closing title tags more robustly - and should
display these malformed pages properly in quirks mode. Could tags embedded in
the title also be handled better?
This is either bug 42945 or an evang. issue...
Yes it is an evang issue, but Moz has to handle the error a little better than
that.  :)

*** This bug has been marked as a duplicate of 42945 ***
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → DUPLICATE
v
Status: RESOLVED → VERIFIED
bug 4295 doesn't cover the second case described in bug 167315 - a malformed
page with <title> in body where everything gets to appear as text in the window
title bar - which may be a problem for large pages (leading to an overflow?).

See example in:
http://www.ee.surrey.ac.uk/Personal/L.Wood/mozilla/bug-5a.html

<html>
<head>
<body>
<title>this is the title line
</body>
this is the run-on body that also appears in the title line
<hr noshade>
</body>
</html>

I think this bug should track this case.
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
So this bug is now about the second case *only* (since the first case is clearly
a dupe of bug 42945). Pages that has an unclosed <TITLE> in the their <BODY>
*deserve* not to be rendered (as also noted in bug 42945 comment 22).

--> INVALID
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → INVALID
In the second case, the entire page gets rendered as ASCII text in the title bar.
That is bad. A possible overflow condition, if you dump megabytes of page into
the window manager for the window title bar, and have a very long title?
Something mozilla should protect against.

I've reopened this bug. Again, the problem is not that the page isn't rendered
in the second case. It's that the text of the page (including tags) appears in
full in the window manager's title bar. Seems like a handy way to attack and
crash a window manager.
URL: http://www.ee.surrey.ac.uk/Personal/L... → http://www.ee.surrey.ac.uk/Personal/L...
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Keywords: compat
Lloyd, are you still seeing this bug?  The testcase at
http://www.ee.surrey.ac.uk/Personal/L.Wood/mozilla/bug-5a.html worksforme with a
current Linux trunk build (2003-03-09-05).
This testcase consists of:
<title>
... 150 000 ASCII characters ...
</title>
The URL is WFM because bug 42945 was fixed.

The issue here is that extremly long TITLEs should be truncated to a reasonable
length before passing them on to the Window System (or Printer?).

The attached testcase file for example crashes my X server (I'm running KDE2/
SuSE 7.3) and along with it the whole desktop and all applications of course.
I must say you have me convinced after that experience :-)

Here is the relevant code for GTK2 (widget/src/gtk2/nsWindow.cpp):

NS_IMETHODIMP
nsWindow::SetTitle(const nsString& aTitle)
{
    if (!mShell)
        return NS_OK;

    // convert the string into utf8 and set the title.
    NS_ConvertUCS2toUTF8 utf8title(aTitle);
    gtk_window_set_title(GTK_WINDOW(mShell), (const char *)utf8title.get());

    return NS_OK;
}


Maybe just a truncating |aTitle| before the gdk call would work?
URL: http://www.ee.surrey.ac.uk/Personal/L...
Keywords: crash, testcase
Summary: lack of closing title tag prevents any display of page → TITLE string should be "sandboxed"
GNU software is supposed to handle arbitrarily-long inputs. That should include
GTK. I'd argue title name termination should happen inside GTK, before passing
to the incredibly lame window manager.
I respectfully disagree - we cannot depend on GTK/X to do this (and especially
now that we know that there is at least one major distro that is broken).

It wouldn't surprise me if this could be used to mount a buffer overflow attack
on the X server (which normally runs as 'root') so this could potentially
be very bad. (see for example bug 121885 for a similar issue).

Asking for a security review please.
Attachment 124838 [details] causes no issues for me with a GTK2 Mozilla, XFree86 4.3, and
Metacity 2.4.55. I think the issue you are seeing is a bug in something outside
of Mozilla.
Defending against possibly overflowing X is a good thing if the fix isn't too
complicated. Let's do it.
-> XP Toolkit/Widgets
Assignee: asa → jaggernaut
Status: REOPENED → NEW
Component: Browser-General → XP Toolkit/Widgets
QA Contact: asa → jrgm
Summary: TITLE string should be "sandboxed" → [FIX] TITLE string should be "sandboxed"
Whiteboard: [patch]
Attached patch Patch for gtk (obsolete) — — Splinter Review 
Patch for widget/src/gtk/nsWindow.cpp: nsWindow::SetTitle() that limits the
title string to a maximum of 4095 characters.

Testing showed that my X server (XFree86 Version 4.1.0) crashes for titles with

~5600 characters or longer.
This is a suggestion for a corresponding change for the other X toolkits.
I haven't tested this patch though...
It would be cool to get security fixes in before they are exploited.
Flags: blocking1.8b2?
Attachment #126156 - Flags: superreview?(blizzard)
Attachment #126156 - Flags: review?(bzbarsky)
Attachment #126156 - Flags: approval1.8b2?
Flags: blocking-aviary1.1?
Comment on attachment 126156 [details] [diff] [review]
Patch for gtk2 and xlib (untested)

The code paths seem fine, even if they are untested. Well behaved window
managers should handle this case, but I agree that we should be doing
protection against this kind of thing as well.
Attachment #126156 - Flags: superreview?(blizzard) → superreview+
First of all, I'm not really qualified to give r+ for the patch -- I don't know
whether any other code paths need changing, for example.

Second, it seems odd to be truncating a UTF8 string in some random place,
possibly in mid-char.  That seems like it could lead to an attack on the window
manager (causing it to read past end of buffer) just as well as the current
behavior.  The Xlib code actually looks OK on this count, since our converters
shouldn't be placing partial chars in the output if the output buffer is too short.
Attachment #126156 - Flags: review?(bzbarsky) → review-
Comment on attachment 126156 [details] [diff] [review]
Patch for gtk2 and xlib (untested)

a=asa
Attachment #126156 - Flags: approval1.8b2? → approval1.8b2+
Is this fixed? 
caillon, can you shepard this in early in 1.1b3?
Assignee: jag → caillon
Flags: blocking1.8b3+
Flags: blocking1.8b2?
Flags: blocking1.8b2-
Will do so when I get back from LinuxTag.
Status: NEW → ASSIGNED
Flags: blocking1.8b4+
Flags: blocking1.8b3-
Flags: blocking1.8b3+
Flags: blocking-aviary1.1?
Whiteboard: [patch] → [has patch, has reviews, ready to land?]
changing cbeard's whiteboard as seems to be based on a misreading - the patch 
has sr, but hasn't got a review AFAICS
Whiteboard: [has patch, has reviews, ready to land?] → [has patch]
Assignee: caillon → mats.palmgren
Status: ASSIGNED → NEW
Attachment #126155 - Attachment is obsolete: true
Attachment #126156 - Attachment is obsolete: true
Attached patch Patch rev. 2 (obsolete) — — Splinter Review 
This patch addresses Boris' comment 19 about truncating UTF8 mid-char.
I couldn't find any suitable string utility functions so the code ended
up being quite inefficient, but normal pages will never hit that block.
(tested on GTK1 and GTK2, not tested on XLIB)
Attachment #192095 - Flags: superreview+
Attachment #192095 - Flags: review?(bzbarsky)
Attachment #192095 - Flags: superreview+
Comment on attachment 192095 [details] [diff] [review]
Patch rev. 2

>+    aTitle.BeginReading(iter);
>+    aTitle.EndReading(end);
>+    titleUTF8.Truncate();
>+    PRUnichar ch[2] = { 0, 0 };
>+    while (iter != end && titleUTF8.Length() <= NS_WINDOW_TITLE_MAX_LENGTH - 4) {
>+      ch[0] = *iter;
>+      AppendUTF16toUTF8(ch, titleUTF8);
>+      ++iter;
>+    }
>+  }

I think something more like your first patch was better, just check for utf8
"first chars" and chop there.

#define UTF8_FOLLOWBYTE(ch) (((ch) & 0xC0) == 0x80)
    NS_ConvertUTF16toUTF8 utf8_title(aTitle);
    if (utf8_title.Length() > NS_WINDOW_TITLE_MAX_LENGTH) {
       // truncate overlong titles (bug 167315). Make sure we chop after a
       // complete sequence by making sure the next char isn't a follow-byte
       PRUint32 len = NS_WINDOW_TITLE_MAX_LENGTH;
       while(UTF8_FOLLOWBYTE(utf8_title[len]))
	 --len;
       utf8_title.Truncate(len);
    }
Attached patch Patch rev. 3 — — Splinter Review 
Thanks, that's much better. I didn't know the bytes were encoded like that.
Attachment #192095 - Attachment is obsolete: true
Attachment #192165 - Flags: review?(bzbarsky)
Attachment #192095 - Flags: review?(bzbarsky)
This crash still occurs actually:

----------------------------------------------------------------
The application 'Gecko' lost its connection to the display :0.0;
most likely the X server was shut down or you killed/destroyed
the application.
----------------------------------------------------------------

It didn't crash my X server this time, just SeaMonkey (2005-08-09-03/gtk2).
I have attached a testcase at
http://landfill.bugzilla.org/bugzilla-tip/show_bug.cgi?id=444

it triggers the "follow-byte" case (warning it crashes your browser, or more).

My system:
vendor string:    The X.Org Foundation
vendor release number:    60802000
X.Org version: 6.8.2

Linux 2.6.11.4-21.8-default (i686)
Severity: normal → critical
Attachment #192165 - Flags: superreview+
Attachment #192165 - Flags: review?(bzbarsky)
Attachment #192165 - Flags: review+
Attachment #192165 - Flags: approval1.8b4?
Attachment #192165 - Flags: approval1.8b4? → approval1.8b4+
Checked in on trunk and 1.8 branch at 2005-08-18 01:10 PDT

-> FIXED
Status: NEW → RESOLVED
Closed: 22 years ago → 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051003 
Firefox/1.4.1

verified fixed
Keywords: fixed1.8 → verified1.8
*** Bug 314308 has been marked as a duplicate of this bug. ***
Blocks: 311052
*** Bug 311052 has been marked as a duplicate of this bug. ***
No longer blocks: 311052
Sandboxing the title string properly should stop the announced exploit where a long title string is written to the history.dat file, crashing firefox on next launch. See:

http://packetstormsecurity.org/0512-exploits/firefox-1.5-buffer-overflow.txt
(which messes with document.title.)

http://isc.sans.org/diary.php?storyid=920
First Vulnerability for Firefox 1.5 (released version) Announced - PoC available (NEW), Published: 2005-12-08, Last Updated: 2005-12-08 16:37:43 UTC by John Bambenek (Version: 1) 

http://news.com.com/Unpatched+Firefox+1.5+exploit+made+public/2100-1002_3-5987401.html

So, now we can crash Firefox itself -- as well as crashing the window manager. Rather lame, and not new... but worth asking for a security review to catch any other related crashes?
Priority: -- → P1
All of that sounds like a separate bug (one that needs to be fixed in the Firefox-specific history code, not in Core code).
Priority: P1 → --
Boris,

The obvious fix would seem to be at a chokepoint/bottleneck when the title string is defined or modified. If you say that the title can be any length and go and fix this in the history code, where else would you need to fix this? On printer output? On View Page Info? Everywhere that looks at the title string?
Yes.  I see no reason for us to break DOM handling of the title, for example, because some title consumers have constraints that they don't bother to actually apply to their title handling.
In particular, the history code and window managers (this bug) have different constraints on the title string, so there is simply no sane way to handle both in the core.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: