Last Comment Bug 201547 - (zen7) Buffer overrun in mimetric.cpp
: Buffer overrun in mimetric.cpp
Product: MailNews Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla1.4beta
Assigned To: Mitchell Stoltz (not reading bugmail)
: John Unruh
Depends on:
  Show dependency treegraph
Reported: 2003-04-10 13:43 PDT by Mitchell Stoltz (not reading bugmail)
Modified: 2008-07-31 01:24 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

C source file for demonstration (4.98 KB, text/plain)
2003-04-10 13:46 PDT, Mitchell Stoltz (not reading bugmail)
no flags Details
Patch - zen's suggested fix (599 bytes, patch)
2003-04-21 16:33 PDT, Mitchell Stoltz (not reading bugmail)
security-bugs: review+
hjtoi-bugzilla: superreview+
Details | Diff | Splinter Review

Description Mitchell Stoltz (not reading bugmail) 2003-04-10 13:43:15 PDT

Issue details:
MimeRichtextConvert() in mailnews/mime/src/mimetric.cpp allocates a buffer that
is smaller than is needed. 

The buffer allocated is 4 times the length of the input, however the '&'
character expands to "&" (5 chars).

A fix could be to change 
106   desired_size = length * 4;
106   desired_size = (length * 5) + 1; 
which would handle the worst case of an input consisting of all '&' characters.

In order to exploit this vulnerability someone would need to seen a specially
crafted email, and it would ned to be viewed.
Comment 1 Mitchell Stoltz (not reading bugmail) 2003-04-10 13:46:24 PDT
Created attachment 120108 [details]
C source file for demonstration
Comment 2 georgi - hopefully not receiving bugspam 2003-04-11 08:50:41 PDT
Does "view message body as plain text" stop this?
Comment 3 Mitchell Stoltz (not reading bugmail) 2003-04-11 14:36:01 PDT
Stack trace when the overrun happens (thanks Darin):

==3531==    at 0x4046E384: PL_strcpy
==3531==    by 0x49D3528C: MimeRichtextConvert(char*, int, int (*)(char*, int,
void*), void*, char**, int*, int)
==3531==    by 0x49D35AA3: MimeInlineTextRichtext_parse_line(char*, int,
MimeObject*) (/builds/trunk/mozilla/mailnews/mime/src/mimetric.cpp:336)
==3531==    by 0x49D2EF5A: MimeInlineText_convert_and_parse_line(char*, int,
MimeObject*) (/builds/trunk/mozilla/mailnews/mime/src/mimetext.cpp:412)
==3531==    Address 0x4FA944C4 is 0 bytes after a block of size 3972 alloc'd
==3531==    at 0x4003BA4E: malloc (vg_clientfuncs.c:100)
==3531==    by 0x40489512: PR_Malloc
==3531==    by 0x49D36B77: mime_GrowBuffer
==3531==    by 0x49D350C1: MimeRichtextConvert(char*, int, int (*)(char*, int,
void*), void*, char**, int*, int)
Comment 4 Mitchell Stoltz (not reading bugmail) 2003-04-21 16:33:57 PDT
Created attachment 121225 [details] [diff] [review]
Patch - zen's suggested fix
Comment 5 Mitchell Stoltz (not reading bugmail) 2003-04-21 16:34:52 PDT
Comment on attachment 121225 [details] [diff] [review]
Patch - zen's suggested fix

r=mstoltz, needs an sr
Comment 6 Heikki Toivonen (remove -bugzilla when emailing directly) 2003-04-22 16:44:23 PDT
Comment on attachment 121225 [details] [diff] [review]
Patch - zen's suggested fix

Should fix the ampersand at least, so sr=heikki.

Then I got scared that the stuff below that in the function might result in
bigger expansion than 1->5, but it does not seem so.

So this brief moment of panic made me realize we really need some comments.
When setting the desired_length, how about adding a comment like this for
// The code below must never expand anything by more than the factor specified
here; if it does this value must be changed too
Comment 7 Cavin Song 2003-04-22 17:53:32 PDT
It is possible that the switch statement in the function eats up more space than
the buffer was allocated at the beginning of the function. I added the following
line to the attached C demonstration program (which must be compiled on linux
and run) and the switch statement was indeed invoked.

strcat(buf,"<BIGGER> xx </BIGGER>");  // this is what I added

In this case, we are replacing:



  <FONT SIZE="+1"> xx </FONT>

so we eat up 6 more chars than what was allocated. But in other cases we are
actually saving some room. For example,

  <BOLD> xx </BOLD>  --->  <B> xx </B> 

we are saving 6 chars in this case. So it depends on what's in the body text we
may or may not overrun the allocated buffer.
Comment 8 Mitchell Stoltz (not reading bugmail) 2003-04-22 18:02:21 PDT
Cavin and I discussed converting this function to use the string classes, so we
won't have to deal with allocation. Cavin is going to look into this.

In the meantime, I already checked in the above change, so we're a little better
Comment 9 georgi - hopefully not receiving bugspam 2003-04-23 02:24:43 PDT
Is there another testcase to check whether "view message body as plaintext"
stops this attack?
The original testcase just makes mozilla unresponsive for a long time, though
ltrace shows it does some memory relocation.
Comment 10 Heikki Toivonen (remove -bugzilla when emailing directly) 2003-04-23 10:22:18 PDT
I haven't actually debugged this, but replacing "<BIGGER> xx </BIGGER>" with
"<FONT SIZE="+1"> xx </FONT>" means an expansion factor 1.27, and that should be
safe. Compare to "&" -> "&amp;", which is expansion factor 5. Since the
calculation for the new string is now 5*original_length+1 we should be ok - none
of the tag stuff seems to expand the string bigger than that. Or am I missing
Comment 11 Cavin Song 2003-04-23 11:57:00 PDT
Good point. We should be fine with *5.
Comment 12 Mitchell Stoltz (not reading bugmail) 2003-04-23 12:00:00 PDT
The 5x+1 fix has been checked in, but I'd like to get some verification that
none of the other expansions are more than 5x. Georgi, could you take a look?
Comment 13 Asa Dotzler [:asa] 2003-04-23 14:24:17 PDT
mstolts, can you land this on the MOZILLA_1_3_BRANCH as well? 
Comment 14 (not reading, please use instead) 2003-04-23 15:41:04 PDT
I've landed the fix on the 1.3 branch, per asa's request.
Comment 15 georgi - hopefully not receiving bugspam 2003-04-24 03:04:57 PDT
I shall look into it.
I am concerned also about the following: in text/plain email *word* becomes bold
when rendered. What file/class is responsible for this conversion to check it
for similar issues?
Comment 16 Heikki Toivonen (remove -bugzilla when emailing directly) 2003-04-24 14:40:59 PDT
I'm marking this fixed, please reopen if you find a case where the landed fix
was not sufficient.
Comment 17 John Unruh 2003-05-13 14:57:34 PDT
Comment 18 Daniel Veditz [:dveditz] 2004-07-20 04:48:37 PDT
Bugs published on the Known-vulnerabilities page long ago, removing confidential

Note You need to log in before you can comment on or make changes to this bug.