from firstname.lastname@example.org: Issue details: MimeRichtextConvert() in mailnews/mime/src/mimetric.cpp allocates a buffer that is smaller than is needed. http://lxr.mozilla.org/mozilla/source/mailnews/mime/src/mimetric.cpp#106 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; into 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.
Does "view message body as plain text" stop this?
Stack trace when the overrun happens (thanks Darin): ==3531== at 0x4046E384: PL_strcpy (/builds/trunk/mozilla/nsprpub/lib/libc/src/strcpy.c:45) ==3531== by 0x49D3528C: MimeRichtextConvert(char*, int, int (*)(char*, int, void*), void*, char**, int*, int) (/builds/trunk/mozilla/mailnews/mime/src/mimetric.cpp:165) ==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 (/builds/trunk/mozilla/nsprpub/pr/src/malloc/prmem.c:433) ==3531== by 0x49D36B77: mime_GrowBuffer (/builds/trunk/mozilla/mailnews/mime/src/mimebuf.cpp:49) ==3531== by 0x49D350C1: MimeRichtextConvert(char*, int, int (*)(char*, int, void*), void*, char**, int*, int) (/builds/trunk/mozilla/mailnews/mime/src/mimetric.cpp:108)
Comment on attachment 121225 [details] [diff] [review] Patch - zen's suggested fix r=mstoltz, needs an sr
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 example: // The code below must never expand anything by more than the factor specified here; if it does this value must be changed too
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 strcat(buf,"AAAABBBBCCCCDDDDEEEEFFFFGGGG"); In this case, we are replacing: <BIGGER> xx </BIGGER> with <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.
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 off.
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.
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 "&" -> "&", 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 something?
Good point. We should be fine with *5.
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?
mstolts, can you land this on the MOZILLA_1_3_BRANCH as well?
I've landed the fix on the 1.3 branch, per asa's request.
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?
I'm marking this fixed, please reopen if you find a case where the landed fix was not sufficient.
Bugs published on the Known-vulnerabilities page long ago, removing confidential flag.