Closed Bug 412363 (CVE-2008-0304) Opened 16 years ago Closed 16 years ago

Buffer overflow in external MIME bodies

Categories

(MailNews Core :: MIME, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9

People

(Reporter: dveditz, Assigned: bugmil.ebirol)

Details

(Keywords: fixed1.8.0.15, fixed1.8.1.12, Whiteboard: [sg:critical?])

Attachments

(4 files)

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?
Assignee: nobody → dmose
Priority: -- → P1
Whiteboard: [sg:critical?]
Dan, are you working on this? I can look at it if you're not.
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).
Attached patch proposed fixSplinter Review
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)
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
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 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-
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 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 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.
Attachment #299387 - Flags: approval1.8.1.12?
> 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+
will fix the tab wonkiness on the lines I touched.
Status: ASSIGNED → NEW
Fixed on 1.8 branch.
Keywords: fixed1.8.1.12
Do we have a good way to verify the fix for QA?
Flags: blocking1.9?
The answer to this question seems to be "No."
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.
Missed this bit in the initial message to security:

> IX. CREDIT
> 
> This vulnerability was reported to iDefense by regenrecht.
Flags: tracking1.9? → blocking1.9?
Dan/David: what do you think about landing this on the trunk too?
It should be on the trunk.
Group: security
I don't see the patch applied to the file on the trunk yet. Can somebody confirm otherwise?
No, it's not on trunk yet.
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+
Flags: blocking1.8.0.15+
Let's get this on trunk asap..
Flags: blocking1.9? → blocking1.9+
Flags: blocking-thunderbird3? → blocking-thunderbird3.0a1+
Target Milestone: --- → mozilla1.9beta5
Landed on the 1.8.0.15 branch
Keywords: fixed1.8.0.15
(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.


Is there a trunk patch for this?
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
So, according to comment #22 we should get the patch on trunk. (All hunks FAIL currently though.)

Is anyone working on it?
Is there any specific reason not to land it to trunk as it is?
Because all hunks currently fail, according to comment 33.
Assignee: bienvenu → ebirol
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)
Attachment #315196 - Flags: review?(dveditz)
Comment on attachment 315196 [details] [diff] [review]
alternate path (trunk version)

r=dveditz
Attachment #315196 - Flags: review?(dveditz) → review+
Attachment #315196 - Flags: review?(dveditz)
Attachment #315196 - Flags: review?(dveditz)
Comment on attachment 315196 [details] [diff] [review]
alternate path (trunk version)

sr=dmose
Attachment #315196 - Flags: superreview?(dmose) → superreview+
mailnews/mime/src/mimeebod.cpp 1.32
Status: NEW → RESOLVED
Closed: 16 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.

Attachment

General

Created:
Updated:
Size: