Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 412363 (CVE-2008-0304)

Buffer overflow in external MIME bodies

RESOLVED FIXED in mozilla1.9

Status

MailNews Core
MIME
P1
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: dveditz, Assigned: Emre Birol)

Tracking

({fixed1.8.0.15, fixed1.8.1.12})

unspecified
mozilla1.9
fixed1.8.0.15, fixed1.8.1.12
Bug Flags:
blocking-thunderbird3.0a1 +
blocking1.9 -
blocking1.8.1.12 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(4 attachments)

(Reporter)

Description

10 years ago
iDefense reports the following mailnews bug:

II. DESCRIPTION

Remote exploitation of a heap based buffer overflow vulnerability in
Mozilla Organization's Thunderbird could allow an attacker to execute
arbitrary code with the privileges of the current user.

The vulnerability exists when parsing the external-body MIME type in an
electronic mail. When calculating the number of bytes to allocate for a
heap buffer, sufficient space is not reserved for all of the data being
copied into the buffer. This results in up to 3 bytes of the buffer
being overflowed, potentially allowing for the execution of arbitrary
code.

A partial listing of the MimeExternalBody_make_url() function (line
numbers based on the source code of version 2.0.0.9) from
mailnews/mime/src/mimeebod.cpp is shown below:

230 else if (!nsCRT::strcasecmp(at, "mail-server"))

     {

        char *s2;

        if (!svr)

           return 0;

235   s = (char *) PR_MALLOC(strlen(svr)*4 +

                               (subj ? strlen(subj)*4 : 0) +

                               (body ? strlen(body)*4 : 0) + 20);

        if (!s) return 0;

239   PL_strcpy(s, "mailto:");

        s2 = nsEscape(svr, url_XAlphas);

        if (s2)

        {

244    PL_strcat(s, s2);

           nsCRT::free(s2);

        }

        if (subj)

        {

250    s2 = nsEscape(subj, url_XAlphas);

251    PL_strcat(s, "?subject=");

           if (s2)

           {

254      PL_strcat(s, s2);

              nsCRT::free(s2);

           }

        }

        if (body)

        {

260    s2 = nsEscape(body, url_XAlphas);

261    PL_strcat(s, (subj ? "&body=" : "?body="));

           if (s2)

           {

264      PL_strcat(s, s2);

              nsCRT::free(s2);

           }

        }

        return s;

268 }

At line 235 a heap buffer is allocated. The "+ 20" is meant to reserve
enough space for the strings copied into the buffer at lines 239, 251,
and 261. However, the combined length of these strings is 23 bytes,
including the NULL byte at the end. Depending on the underlying memory
allocator, this can result in the corruption of heap memory.

III. ANALYSIS

Exploitation of this vulnerability results in the execution of arbitrary
code with the privileges of the user running Thunderbird. Exploitation
requires that an attacker social engineers a user into viewing a
malicious message in Thunderbird. If the 'View->Message Pane' option is
turned on (the "Preview" pane), which is the default, then all a
targeted user has to do is select the message in the browsing pane.
Once the message is previewed, the vulnerability will be triggered.

Due to the limited scope of the overflow, exploitation is difficult and
has not yet been proven in the Labs. However, this vulnerability still
has the potential to be exploitable, and should be treated as such.

IV. DETECTION

iDefense has confirmed the existence of this vulnerability in
Thunderbird version 2.0.0.9 on Linux and Windows. Previous versions may
also be affected.

V. WORKAROUND

Setting the "mailnews.display.disallow_mime_handlers" configuration
property to any value >= 3 will prevent the vulnerable code from being
triggered. In order to do this:

1) Go to the Tools->Options->Advanced->Config Editor in the Windows
version, or Edit->Preferences->Advanced->Config Editor in the Linux
version

2) Type the property name into the Filter box
(mailnews.display.disallow_mime_handlers)

3) Double click the property and set its value to 3
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12+
Flags: blocking-thunderbird3?
(Reporter)

Updated

10 years ago
Assignee: nobody → dmose
(Reporter)

Updated

10 years ago
Priority: -- → P1
Whiteboard: [sg:critical?]

Comment 1

10 years ago
Dan, are you working on this? I can look at it if you're not.

Comment 2

10 years ago
David: if you're up for taking look, that would rock, as I still have a bunch of Firefox stuff on my plate (nailing down a transition plan today).

Comment 3

10 years ago
Created attachment 297795 [details] [diff] [review]
proposed fix

I don't have a test case, but I convinced myself that you probably could get into this method with empty strings (not null pointers) for subject, server, and body, in which case the overrun would happen, and increasing the buffer size would prevent the problem.

If I had a test case, I'd rewrite the code to use a CString, but I don't. That'll have to wait for the eagerly awaited libmime rewrite :-)
Assignee: dmose → bienvenu
Status: NEW → ASSIGNED
Attachment #297795 - Flags: review?(dmose)
(Reporter)

Comment 4

10 years ago
From iDefense:
> MITRE has assigned CVE-2008-0304 to this issue.  Please reference this
> identifier in any public or customer facing documents regarding this
> vulnerability.
>
> PS.  Please *OMIT* any IDEFXXXX numbers from any such document(s).
Alias: IDEF2936 → CVE-2008-0304
(Reporter)

Comment 5

10 years ago
Created attachment 298978 [details]
zipped mail folder with testcase

The PoC we got was a ruby program that generated an Inbox folder. Here's the output folder (zipped).

Didn't crash me, but I didn't run it in a debug build or under purify/valgrind to see it overflow the couple of bytes involved.

Comment 6

10 years ago
Comment on attachment 297795 [details] [diff] [review]
proposed fix

Thanks for taking this one, David.  

This patch seems like it leaves the code in the same fragile state it was in, but you're absolutely right that a test-case would much better before tweaking the code too much.

dveditz says he has a test that he'll attach, but it doesn't cause a crash.  However, running under purify or valgrind ought to be enough to confirm that it's stomping memory.

How about working with that test and either switching to our C++ string classes or else switching strcpy/strcat to strncpy/strncat.
Attachment #297795 - Flags: review?(dmose) → review-
(Reporter)

Comment 7

10 years ago
Created attachment 299387 [details] [diff] [review]
alternate patch (1.8 branch version)

Here's a patch using safe strcpy/strcat, went ahead and did the whole file and also mimeunty.cpp in the same directory which had similar code where the buffer was simply padded by 100 to cover any slop.

The "mail-server" section multiplies strings by 4 for worse-case, but hex escapes should only require a multiple of 3. Is there something else going on?

There's a body buffer allocated later one, then copied into, and then freed without being used. That one was straightforward enough that I didn't fix up the strcats there, but it's not doing anyone any good.
Attachment #299387 - Flags: superreview?(dmose)
Attachment #299387 - Flags: review?(bienvenu)

Comment 8

10 years ago
Comment on attachment 299387 [details] [diff] [review]
alternate patch (1.8 branch version)

thx, Dan. Re the 4x instead of 3x, my guess is fudging for safety, but it's strictly a guess
Attachment #299387 - Flags: review?(bienvenu) → review+

Comment 9

10 years ago
Comment on attachment 299387 [details] [diff] [review]
alternate patch (1.8 branch version)

Looks good; thanks for the patch, Dan!

Nits:

>+      slen = strlen(name) + strlen(site) + (dir ? strlen(dir) : 0) + 20;
>+	  s = (char *) PR_MALLOC(slen);

Here, and in a few other similar places, there seems to be an indentation issue:

>+             // dpv xxx: why 4x? %xx escaping should be 3x

Maybe use "dveditz", since that's how everyone knows you around these parts?

I'd suggest landing this on the trunk too, since it's significantly better than the status quo.
Attachment #299387 - Flags: superreview?(dmose) → superreview+
Er, the "here" comment refers to the slen line above it, not below it.
(Reporter)

Updated

10 years ago
Attachment #299387 - Flags: approval1.8.1.12?
(Reporter)

Comment 11

10 years ago
> there seems to be an indentation issue:

The original file is full of 4-space tabs. It looks OK if your editor obeys the modeline and I don't feel like being on the hook for replacing all the whitespace in the file. I guess I could use tabs on the lines I added just to fit in.
I wasn't complaining that the lines you added were out of sync with the surrounding lines; that's generally something we live with.

I was complaining that in several places two lines in a row were replaced and yet still ended up with different indentations from each other.  Fixing that shouldn't require taking liberties with cvsblame.
Comment on attachment 299387 [details] [diff] [review]
alternate patch (1.8 branch version)

Approved for 1.8.1.12. a=ss for release drivers.
Attachment #299387 - Flags: approval1.8.1.12? → approval1.8.1.12+
(Reporter)

Comment 14

10 years ago
will fix the tab wonkiness on the lines I touched.
Status: ASSIGNED → NEW
(Reporter)

Comment 15

10 years ago
Fixed on 1.8 branch.
Keywords: fixed1.8.1.12
Do we have a good way to verify the fix for QA?
(Reporter)

Updated

10 years ago
Flags: blocking1.9?
The answer to this question seems to be "No."

Comment 18

10 years ago
Dan V attached a test case - but to see issues, I think you'd need to run with a debug build that warned you about the small buffer overflow (did msvc's runtime warn you, Dan?)
QA has no access to a TB debug build built from the 1.8.1.12 release branch, unfortunately.
(Reporter)

Comment 20

10 years ago
Missed this bit in the initial message to security:

> IX. CREDIT
> 
> This vulnerability was reported to iDefense by regenrecht.

Updated

10 years ago
Flags: tracking1.9? → blocking1.9?
Dan/David: what do you think about landing this on the trunk too?

Comment 22

10 years ago
It should be on the trunk.
(Reporter)

Updated

10 years ago
Group: security
(Assignee)

Comment 23

10 years ago
I don't see the patch applied to the file on the trunk yet. Can somebody confirm otherwise?

Comment 24

10 years ago
No, it's not on trunk yet.

Comment 25

10 years ago
Comment on attachment 299387 [details] [diff] [review]
alternate patch (1.8 branch version)

a=asac for 1.8.0.15
Attachment #299387 - Flags: approval1.8.0.15+

Updated

10 years ago
Flags: blocking1.8.0.15+

Comment 26

10 years ago
Let's get this on trunk asap..
Flags: blocking1.9? → blocking1.9+

Updated

10 years ago
Flags: blocking-thunderbird3? → blocking-thunderbird3.0a1+
Target Milestone: --- → mozilla1.9beta5
Landed on the 1.8.0.15 branch
Keywords: fixed1.8.0.15

Comment 28

10 years ago
(In reply to comment #24)
> No, it's not on trunk yet.
> 
As I am working with plugins in trunk TB, although with trusted content,please do.

I am at a loss to see where the checkins on 2008-03-20 even landed at all.
Maybe it's just lag.


Joe: They landed on the 1.8.0 branch: 

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_0_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-20+13%3A54&maxdate=2008-03-20+13%3A54&cvsroot=%2Fcvsroot

Comment 30

10 years ago
Thanks Sam,
I guess it would help if I actually READ the info in the branch cell on my query:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=ThunderbirdTinderbox&branch=&branchtype=match&dir=mozilla%2Fmail+mozilla%2Fmailnews&file=&filetype=match&who=&whotype=match&sortby=Date&date=hours&hours=24&mindate=1970-12-31+16%3A00%3A01&maxdate=1970-12-31+16%3A00%3A01&cvsroot=%2Fcvsroot

Comment 31

10 years ago
Is there a trunk patch for this?

Comment 32

10 years ago
Taking off the 1.9 blocking list since this appears to be mailnews only - please re-nom if this is wrong...
Flags: blocking1.9+ → blocking1.9-
Target Milestone: mozilla1.9beta5 → mozilla1.9

Comment 33

9 years ago
So, according to comment #22 we should get the patch on trunk. (All hunks FAIL currently though.)

Is anyone working on it?
(Assignee)

Comment 34

9 years ago
Is there any specific reason not to land it to trunk as it is?
Because all hunks currently fail, according to comment 33.
(Assignee)

Updated

9 years ago
Assignee: bienvenu → ebirol
(Assignee)

Comment 36

9 years ago
Created attachment 315196 [details] [diff] [review]
alternate path (trunk version)

Trunk version of the same patch is added. 

* The section that patches mailnews/mime/src/mimeunty.cpp is removed since it overlaps with the path of the bug #413874
Attachment #315196 - Flags: superreview?(dmose)
(Assignee)

Updated

9 years ago
Attachment #315196 - Flags: review?(dveditz)
(Reporter)

Comment 37

9 years ago
Comment on attachment 315196 [details] [diff] [review]
alternate path (trunk version)

r=dveditz
Attachment #315196 - Flags: review?(dveditz) → review+
(Reporter)

Updated

9 years ago
Attachment #315196 - Flags: review?(dveditz)
(Reporter)

Updated

9 years ago
Attachment #315196 - Flags: review?(dveditz)
Comment on attachment 315196 [details] [diff] [review]
alternate path (trunk version)

sr=dmose
Attachment #315196 - Flags: superreview?(dmose) → superreview+

Updated

9 years ago
Keywords: checkin-needed
mailnews/mime/src/mimeebod.cpp 1.32
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.