Last Comment Bug 362213 - heap overflow in MimeExternalBody_parse_eof
: heap overflow in MimeExternalBody_parse_eof
Status: RESOLVED FIXED
[sg:critical]
: fixed1.8.0.9, fixed1.8.1.1
Product: Thunderbird
Classification: Client Software
Component: Security (show other bugs)
: Trunk
: x86 Linux
: -- major (vote)
: ---
Assigned To: David :Bienvenu
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-11-29 04:46 PST by georgi - hopefully not receiving bugspam
Modified: 2008-04-18 02:11 PDT (History)
3 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
overflow (1.78 KB, text/plain)
2006-11-29 04:47 PST, georgi - hopefully not receiving bugspam
no flags Details
proposed fix (920 bytes, patch)
2006-11-29 11:09 PST, David :Bienvenu
mscott: superreview+
mscott: approval‑thunderbird2+
dveditz: approval1.8.0.9+
Details | Diff | Splinter Review
fix the case where all headers are present... (1.63 KB, patch)
2006-12-04 08:19 PST, David :Bienvenu
mscott: superreview+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2006-11-29 04:46:26 PST
heap overflow in MimeExternalBody_parse_eof

MimeExternalBody_parse_eof:
	  h = (char *) PR_MALLOC((at ? strlen(at) : 0) + ...
doesn't allocate space for |ct| and    
FROB("Type",			ct);
overflows.

in addition in 	|(url ? strlen(url) : 0) + 100);| the constant |100| seems
small and may be overflowed even if allocating |ct|.

overflowing mailbox to follow.
Comment 1 georgi - hopefully not receiving bugspam 2006-11-29 04:47:30 PST
Created attachment 246917 [details]
overflow
Comment 2 georgi - hopefully not receiving bugspam 2006-11-29 05:01:32 PST
buggy code:
http://lxr.mozilla.org/seamonkey/source/mailnews/mime/src/mimeebod.cpp#333
Comment 3 David :Bienvenu 2006-11-29 08:01:42 PST
geez, I love libmime. That code is crying out for rewriting with nsCStrings...Dan, I can look at this if you want.
Comment 4 David :Bienvenu 2006-11-29 11:09:05 PST
Created attachment 246962 [details] [diff] [review]
proposed fix

just add ct to the size calculation - I think all other fields are accounted for.
Comment 5 David :Bienvenu 2006-11-29 12:57:58 PST
Comment on attachment 246962 [details] [diff] [review]
proposed fix

simple fix.
Comment 6 David :Bienvenu 2006-11-29 13:01:34 PST
fixed on trunk and 1.8.1 branch
Comment 7 Daniel Veditz [:dveditz] 2006-11-29 17:25:08 PST
Comment on attachment 246962 [details] [diff] [review]
proposed fix

approved for 1.8.0 branch, a=dveditz
Comment 8 georgi - hopefully not receiving bugspam 2006-12-04 00:23:03 PST
(In reply to comment #1)


(In reply to comment #0)
> the constant |100| seems small

this comment is dumb, but the logic of adding |strlen()| is strange. shouldn't it be MAX(strlen()) instead of + strlen?

Comment 9 David :Bienvenu 2006-12-04 07:37:03 PST
all the strings get concatenated together, so we have to add all the strlens...

you're right that 100 is too small - it needs to be ~76 (the total length of the header strings) + (4 * 12) (the per header overhead times the number of possible headers). That would handle the case where all the headers are present...So 150 would be a safe number...
Comment 10 David :Bienvenu 2006-12-04 08:19:32 PST
Created attachment 247417 [details] [diff] [review]
fix the case where all headers are present...

address G30rgi's comment about 100 seeming too small, sorry I missed that before...
Comment 11 David :Bienvenu 2006-12-04 08:22:35 PST
I'm reopening this, unless we want a new bug for the issue the first patch didn't address.
Comment 12 georgi - hopefully not receiving bugspam 2006-12-05 03:53:19 PST
(In reply to comment #9)
> all the strings get concatenated together, so we have to add all the strlens...
> 

are you sure about this?

tried to exploit it with no luck.

373 # define FROB(STR,VAR) \
374       if (VAR) \
375         { \
376           PL_strcpy(h, STR ": "); \
377           PL_strcat(h, VAR); \
378           PL_strcat(h, MSG_LINEBREAK); \
379           status = MimeHeaders_parse_line(h, strlen(h), hdrs); \
380           if (status < 0) goto FAIL; \
381         }

PL_strcpy starts from the begging of |h| ?


Comment 13 David :Bienvenu 2006-12-05 07:13:21 PST
no, I think you're right - re-resolving as fixed. So I have no idea why they're adding together all the strlens...
Comment 14 Scott MacGregor 2006-12-14 15:36:44 PST
clearing blocking flag on a fix already landed on the branch.

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