445117 - integer overflow in MsgEscapeHTML

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[georgi - hopefully not receiving bugspam]

mozilla/mailnews/base/util/nsMsgUtils.cpp
NS_MSG_BASE char *MsgEscapeHTML(const char *string)
{
  /* XXX Hardcoded max entity len. The +1 is for the trailing null. */
  char *rv = (char *) nsMemory::Alloc(strlen(string) * 6 + 1);


|* 6| overflows at about 683M on 32 bit platform.

easily hit at least from mimeebod.cpp.

similar bug, same file,
NS_MSG_BASE PRUnichar *MsgEscapeHTML(const PRUnichar *aSourceBuffer, 
                                      PRInt32 aSourceBufferLen)

  /* XXX Hardcoded max entity len. */
  PRUnichar *resultBuffer = (PRUnichar *)nsMemory::Alloc(aSourceBufferLen *
                            6 * sizeof(PRUnichar) + sizeof(PRUnichar('\0')));


to reproduce send message with content-type external body and body about 683M of '<'.

peak memory usage is about 2G with debug build.

testcase to follow.


#6  0x09236d8b in MsgEscapeHTML (string=0x38c5c409 '<' <repeats 200 times>...)
    at /opt/joro/thunderbird-cvs/mozilla/mailnews/base/util/nsMsgUtils.cpp:1689
#7  0x08ff60df in MimeExternalBody_parse_eof (obj=0xa3d94a8, abort_p=0)
    at /opt/joro/thunderbird-cvs/mozilla/mailnews/mime/src/mimeebod.cpp:422
#8  0x08fd01a2 in MimeContainer_parse_eof (object=0x9bb4e38, abort_p=0)
    at /opt/joro/thunderbird-cvs/mozilla/mailnews/mime/src/mimecont.cpp:140
#9  0x08fdc0ee in MimeMessage_parse_eof (obj=0x9bb4e38, abort_p=0)
    at /opt/joro/thunderbird-cvs/mozilla/mailnews/mime/src/mimemsg.cpp:550
#10 0x08fef12a in mime_display_stream_complete (stream=0xa3ab898)
    at /opt/joro/thunderbird-cvs/mozilla/mailnews/mime/src/mimemoz2.cpp:962
#11 0x08fc53d6 in nsStreamConverter::OnStopRequest (this=0xa3ccce0, 
    request=0xa38be2c, ctxt=0xa3abeb0, status=0)
    at /opt/joro/thunderbird-cvs/mozilla/mailnews/mime/src/nsStreamConverter.cpp:1027
#12 0x08bd5928 in nsDocumentOpenInfo::OnStopRequest (this=0xa415290,


(gdb) frame 6
#6  0x09236d8b in MsgEscapeHTML (string=0x38c5c409 '<' <repeats 200 times>...)
    at /opt/joro/thunderbird-cvs/mozilla/mailnews/base/util/nsMsgUtils.cpp:1689
1689            *ptr++ = ';';

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: perl script to generate local folder - usage: ./file > foldername

Comment 2

[georgi - hopefully not receiving bugspam]

note:

1. the perl script produces 683MB file
2. after the crash foldername.msf must be deleted to reproduce the crash again

Comment 3

[Daniel Veditz [:dveditz]]

Reassigning to dmose to find an appropriate TBird dev

This does not appear to affect Thunderbird 2.0

Filed bug 455987 on a similar issue that does affect the 1.8 and 1.9 branches.

Comment 4

[David :Bienvenu]

putting in b1

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Emre, you've helped out on the MIME front before, can you have a look at this?

Comment 6

[Daniel Veditz [:dveditz]]

We're fixing bug 455987 on Firefox branches so you'll definitely want this fixed before you ship Tbird 3. The changes made in nsEscape should work here as well.

Comment 7

[Emre Birol]

Attachment: Proposed patch

Changes have been applied to nsMsgUtils.cpp

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 347590 [details] [diff] [review]
Proposed patch

>+  if (aSourceBufferLen >=
>+    ((PR_UINT32_MAX / (6 * sizeof(PRUnichar))) + sizeof(PRUnichar)))
>+      return nsnull;
>+
>   PRUnichar *resultBuffer = (PRUnichar *)nsMemory::Alloc(aSourceBufferLen *
>                             6 * sizeof(PRUnichar) + sizeof(PRUnichar('\0')));

This is embarrassing because it's essentially my patch from bug 455987, but r- because I should've subtracted the size of the terminator, not added it :-(

This should be

   if (aSourceBufferLen >= ((PR_UINT32_MAX - sizeof(PRUnichar))/(6 * sizeof(PRUnichar))) )
      return nsnull;

Comment 9

[Daniel Veditz [:dveditz]]

Filed bug 464998 to fix nsEscape.cpp

Comment 10

[David :Bienvenu]

Emre, can you put up a new patch for review here so we can get this fixed and off the beta1 blocker list? thx!

Comment 11

[Emre Birol]

Attachment: Sign is fixed.

I share the half of the embarrassment :-)

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 348629 [details] [diff] [review]
Sign is fixed.

r/sr=dveditz

Comment 13

[David :Bienvenu]

I'll land this, since it's a private bug and at least some of the folks that troll for checkin-needed won't see it.

Comment 14

[David :Bienvenu]

fix checked in (I tweaked the formatting which somehow got messed up a bit...)