202546 - Crash, possible buffer overrun in comi18n.cpp

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

Attached file tmp3.pl generates email message which crashes mozilla.
Depending on the length of the overflow, some time mozilla exits, sometimes
it is possible to attach with gdb.
Not sure where the overflow is.


Found this while examinig comi18n.cpp, though the crash may not be related
to it.
In comi18n.cpp in function intl_decode_mime_part2_str there is
--------------------
 /* Assume no more than 3X expansion due to UTF-8 conversion */
 retbuff = (char *)PR_Malloc(3*strlen(header)+1);
--------------------
Then in intl_copy_uncoded_header the string is converted to UTF-8 and
copied.

Is such an attack possible:
Embed \x00 in the headers so strlen terminates and then \x00 is treated as
valid UTF-8 or some other charset character ?
Looks like it is possible to embed \x00 in From: and other headers - it
does not terminate the string when the message is opened.
Not sure whether the comi18n is hit, though.


Georgi Guninski

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Attachment: Testcase

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Seth, another possible buffer overrun in MailNews. Can you investigate, or get
someone else from the mailnews people to check this out?

Comment 3

[(not reading, please use [e-mail] instead)]

re-assign to cavin, to investigate.

Comment 4

[nhottanscp]

4 bytes per Unicode character is a maximum for UTF-8 encoding, so it is safer to
do  times four instead of three
but existing charset encoding's code point maps to maximum 3 bytes UTF-8 (so no
4 byte case from one character in the input charset code point)
Simon, any comment?

Comment 5

[Simon Montagu :smontagu]

Attachment: Stack trace

Comment 6

[Cavin Song]

Ah ok, this is where it crashes. Thanks Simon. I could not see the crash on my
machine with debug build. Using the test mail msg the mime/i18n code seems to
work fine (ie, buffer is big enough) because ConvertToUnicode() seems to fail
all the time so the data actually get copies to the buffer is very small.

Naoki's comment #4 indicates that *3 is pretty safe.

Comment 7

[Simon Montagu :smontagu]

Attachment: Patch

Comment 8

[Cavin Song]

Comment on attachment 121587 [details] [diff] [review]
Patch

r=cavin. Just curious, what's 'currentCharset' set to when it crashes (since I
can't see it on my machine)?

Comment 9

[Cavin Song]

Comment on attachment 121587 [details] [diff] [review]
Patch

r=cavin. Just curious, what's 'currentCharset' set to when it crashes (since I
can't see it on my machine)?

Comment 10

[Simon Montagu :smontagu]

Without the patch, currentCharset is pointing at uninitialized memory when it
crashes.

Comment 11

[Simon Montagu :smontagu]

Adding jag to cc in case that was blocking the review request.

Comment 12

[jag (Peter Annema)]

Comment on attachment 121587 [details] [diff] [review]
Patch

sr=jag

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 121587 [details] [diff] [review]
Patch

Requesting 1.4 approval for this simple crash fix.

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 121587 [details] [diff] [review]
Patch

a=sspitzer

Comment 15

[Asa Dotzler [:asa]]

Can someone please land this on the 1.3.1 branch as well (ASAP)? Thanks.

Comment 16

[(not reading, please use [e-mail] instead)]

http://bugzilla.mozilla.org/attachment.cgi?id=121587&action=view
fix landed on 1.3 branch

Comment 17

[Simon Montagu :smontagu]

Fix checked in to trunk.

Comment 18

[Charles Rosendahl]

verified

Comment 19

[Daniel Veditz [:dveditz]]

Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.