Closed
Bug 201547
(zen7)
Opened 21 years ago
Closed 21 years ago
Buffer overrun in mimetric.cpp
Categories
(MailNews Core :: Security, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.4beta
People
(Reporter: security-bugs, Assigned: security-bugs)
Details
Attachments
(2 files)
4.98 KB,
text/plain
|
Details | |
599 bytes,
patch
|
security-bugs
:
review+
hjtoi-bugzilla
:
superreview+
|
Details | Diff | Splinter Review |
from zen-parse@gmx.net: 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.
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Flags: blocking1.4b?
Comment 2•21 years ago
|
||
Does "view message body as plain text" stop this?
Assignee | ||
Comment 3•21 years ago
|
||
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)
Assignee | ||
Updated•21 years ago
|
Alias: zen7
Assignee | ||
Comment 4•21 years ago
|
||
Assignee | ||
Comment 5•21 years ago
|
||
Comment on attachment 121225 [details] [diff] [review] Patch - zen's suggested fix r=mstoltz, needs an sr
Attachment #121225 -
Flags: superreview?(heikki)
Attachment #121225 -
Flags: review+
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
Attachment #121225 -
Flags: superreview?(heikki) → superreview+
Comment 7•21 years ago
|
||
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.
Assignee | ||
Comment 8•21 years ago
|
||
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.
Comment 9•21 years ago
|
||
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?
Comment 11•21 years ago
|
||
Good point. We should be fine with *5.
Assignee | ||
Comment 12•21 years ago
|
||
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•21 years ago
|
||
mstolts, can you land this on the MOZILLA_1_3_BRANCH as well?
Comment 14•21 years ago
|
||
I've landed the fix on the 1.3 branch, per asa's request.
Comment 15•21 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 21 years ago
Flags: blocking1.4b?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.4beta
Comment 18•20 years ago
|
||
Bugs published on the Known-vulnerabilities page long ago, removing confidential flag.
Group: security
Updated•20 years ago
|
Product: MailNews → Core
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•