The default bug view has changed. See this FAQ.

heap overflow in MimeExternalBody_parse_eof

RESOLVED FIXED

Status

Thunderbird
Security
--
major
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: Bienvenu)

Tracking

({fixed1.8.0.9, fixed1.8.1.1})

Trunk
x86
Linux
fixed1.8.0.9, fixed1.8.1.1
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Created attachment 246917 [details]
overflow
(Reporter)

Updated

11 years ago
Severity: normal → major
buggy code:
http://lxr.mozilla.org/seamonkey/source/mailnews/mime/src/mimeebod.cpp#333
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.1?
Flags: blocking-thunderbird2?
(Assignee)

Comment 3

11 years ago
geez, I love libmime. That code is crying out for rewriting with nsCStrings...Dan, I can look at this if you want.
Assignee: dveditz → bienvenu
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: wanted1.8.0.x+
Flags: blocking1.8.0.9?
Whiteboard: [sg:critical]
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
(Assignee)

Comment 4

11 years ago
Created attachment 246962 [details] [diff] [review]
proposed fix

just add ct to the size calculation - I think all other fields are accounted for.
Attachment #246962 - Flags: superreview?(mscott)
Whiteboard: [sg:critical] → [sg:critical] needs reviews (mscott)

Updated

11 years ago
Attachment #246962 - Flags: superreview?(mscott)
Attachment #246962 - Flags: superreview+
Attachment #246962 - Flags: approval-thunderbird2+
(Assignee)

Comment 5

11 years ago
Comment on attachment 246962 [details] [diff] [review]
proposed fix

simple fix.
Attachment #246962 - Flags: approval1.8.0.9?
(Assignee)

Comment 6

11 years ago
fixed on trunk and 1.8.1 branch
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Whiteboard: [sg:critical] needs reviews (mscott) → [sg:critical]
Comment on attachment 246962 [details] [diff] [review]
proposed fix

approved for 1.8.0 branch, a=dveditz
Attachment #246962 - Flags: approval1.8.0.9? → approval1.8.0.9+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.9
(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?

(Assignee)

Comment 9

10 years ago
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...
(Assignee)

Comment 10

10 years ago
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...
Attachment #247417 - Flags: superreview?(mscott)
Attachment #247417 - Flags: review?(dveditz)
(Assignee)

Comment 11

10 years ago
I'm reopening this, unless we want a new bug for the issue the first patch didn't address.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

10 years ago
Attachment #247417 - Flags: superreview?(mscott) → superreview+
(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| ?


(Assignee)

Comment 13

10 years ago
no, I think you're right - re-resolving as fixed. So I have no idea why they're adding together all the strlens...
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #247417 - Attachment is obsolete: true
Attachment #247417 - Flags: review?(dveditz)
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+

Comment 14

10 years ago
clearing blocking flag on a fix already landed on the branch.
Flags: blocking-thunderbird2?
Group: security
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.