362213 - heap overflow in MimeExternalBody_parse_eof

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[georgi - hopefully not receiving bugspam]

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]

Attachment: overflow

Comment 2

[georgi - hopefully not receiving bugspam]

buggy code:
http://lxr.mozilla.org/seamonkey/source/mailnews/mime/src/mimeebod.cpp#333

Comment 3

[David :Bienvenu]

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]

Attachment: proposed fix

just add ct to the size calculation - I think all other fields are accounted for.

Comment 5

[David :Bienvenu]

Comment on attachment 246962 [details] [diff] [review]
proposed fix

simple fix.

Comment 6

[David :Bienvenu]

fixed on trunk and 1.8.1 branch

Comment 7

[Daniel Veditz [:dveditz] out until Nov 10]

Comment on attachment 246962 [details] [diff] [review]
proposed fix

approved for 1.8.0 branch, a=dveditz

Comment 8

[georgi - hopefully not receiving bugspam]

(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]

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]

Attachment: 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]

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]

(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]

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]

clearing blocking flag on a fix already landed on the branch.