Closed Bug 202546 Opened 17 years ago Closed 17 years ago

Crash, possible buffer overrun in comi18n.cpp

Categories

(Core :: Security, defect)

x86
Windows 2000
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.4beta

People

(Reporter: security-bugs, Assigned: cavin)

Details

Attachments

(3 files)

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
Attached file Testcase
Seth, another possible buffer overrun in MailNews. Can you investigate, or get
someone else from the mailnews people to check this out?
Flags: blocking1.4b?
re-assign to cavin, to investigate.
Assignee: mstoltz → cavin
Keywords: nsbeta1
Target Milestone: --- → mozilla1.4beta
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?
Attached file Stack trace
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.
Attached patch PatchSplinter Review
Attachment #121587 - Flags: superreview?(jaggernaut)
Attachment #121587 - Flags: review?(cavin)
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 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)?
Attachment #121587 - Flags: review?(cavin) → review+
Without the patch, currentCharset is pointing at uninitialized memory when it
crashes.
Adding jag to cc in case that was blocking the review request.
Comment on attachment 121587 [details] [diff] [review]
Patch

sr=jag
Attachment #121587 - Flags: superreview?(jaggernaut) → superreview+
Comment on attachment 121587 [details] [diff] [review]
Patch

Requesting 1.4 approval for this simple crash fix.
Attachment #121587 - Flags: approval1.4b?
Comment on attachment 121587 [details] [diff] [review]
Patch

a=sspitzer
Attachment #121587 - Flags: approval1.4b? → approval1.4b+
Can someone please land this on the 1.3.1 branch as well (ASAP)? Thanks.
Fix checked in to trunk.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: blocking1.4b?
verified
Status: RESOLVED → VERIFIED
Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.
Group: security
You need to log in before you can comment on or make changes to this bug.