If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

integer overflow in MsgEscapeHTML

RESOLVED FIXED in Thunderbird 3.0b1

Status

MailNews Core
Security
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: Emre Birol)

Tracking

Trunk
Thunderbird 3.0b1
x86
Linux
Bug Flags:
blocking-thunderbird3 +
wanted1.8.1.x -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] )

Attachments

(2 attachments, 1 obsolete attachment)

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++ = ';';
Created attachment 329418 [details]
perl script to generate local folder - usage: ./file > foldername
Whiteboard: [sg:critical?]
note:

1. the perl script produces 683MB file
2. after the crash foldername.msf must be deleted to reproduce the crash again
Product: Core → MailNews Core
Component: Security → MailNews: Security
Product: Thunderbird → Core
QA Contact: thunderbird → security

Updated

9 years ago
Summary: integer overlfow in MsgEscapeHTML → integer overflow in MsgEscapeHTML

Updated

9 years ago
Flags: blocking-thunderbird3?
Blocks: 455987
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.
Assignee: nobody → dmose
Flags: wanted1.8.1.x-
Version: unspecified → Trunk

Comment 4

9 years ago
putting in b1
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b1
Emre, you've helped out on the MIME front before, can you have a look at this?
Assignee: dmose → bugmil.ebirol
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.

Updated

9 years ago
Whiteboard: [sg:critical?] → [sg:critical?] [needs patch]
(Assignee)

Comment 7

9 years ago
Created attachment 347590 [details] [diff] [review]
Proposed patch

Changes have been applied to nsMsgUtils.cpp
Attachment #347590 - Flags: superreview?(dveditz)
Attachment #347590 - Flags: review?(dveditz)
Attachment #347590 - Flags: superreview?(dveditz)
Attachment #347590 - Flags: review?(dveditz)
Attachment #347590 - Flags: review-
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;
Filed bug 464998 to fix nsEscape.cpp

Comment 10

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

Comment 11

9 years ago
Created attachment 348629 [details] [diff] [review]
Sign is fixed.

I share the half of the embarrassment :-)
Attachment #347590 - Attachment is obsolete: true
Attachment #348629 - Flags: superreview?(dveditz)
Attachment #348629 - Flags: review?(dveditz)

Updated

9 years ago
Whiteboard: [sg:critical?] [needs patch] → [sg:critical?] [needs review dveditz]
Comment on attachment 348629 [details] [diff] [review]
Sign is fixed.

r/sr=dveditz
Attachment #348629 - Flags: superreview?(dveditz)
Attachment #348629 - Flags: superreview+
Attachment #348629 - Flags: review?(dveditz)
Attachment #348629 - Flags: review+

Comment 13

9 years ago
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.
Whiteboard: [sg:critical?] [needs review dveditz] → [sg:critical?]

Comment 14

9 years ago
fix checked in (I tweaked the formatting which somehow got messed up a bit...)
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.