Last Comment Bug 412363 - (CVE-2008-0304) Buffer overflow in external MIME bodies
(CVE-2008-0304)
: Buffer overflow in external MIME bodies
Status: RESOLVED FIXED
[sg:critical?]
: fixed1.8.0.15, fixed1.8.1.12
Product: MailNews Core
Classification: Components
Component: MIME (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla1.9
Assigned To: Emre Birol
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-14 15:01 PST by Daniel Veditz [:dveditz]
Modified: 2008-07-31 04:30 PDT (History)
15 users (show)
mkmelin+mozilla: blocking‑thunderbird3.0a1+
mtschrep: blocking1.9-
dveditz: blocking1.8.1.12+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (1.10 KB, patch)
2008-01-18 09:42 PST, David :Bienvenu
dmose: review-
Details | Diff | Splinter Review
zipped mail folder with testcase (1.01 KB, application/octet-stream)
2008-01-24 10:44 PST, Daniel Veditz [:dveditz]
no flags Details
alternate patch (1.8 branch version) (7.96 KB, patch)
2008-01-26 00:11 PST, Daniel Veditz [:dveditz]
mozilla: review+
dmose: superreview+
samuel.sidler+old: approval1.8.1.12+
asac: approval1.8.0.next+
Details | Diff | Splinter Review
alternate path (trunk version) (6.95 KB, patch)
2008-04-11 15:26 PDT, Emre Birol
dveditz: review+
dmose: superreview+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2008-01-14 15:01:19 PST
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
Comment 1 David :Bienvenu 2008-01-17 08:07:49 PST
Dan, are you working on this? I can look at it if you're not.
Comment 2 Dan Mosedale (:dmose) 2008-01-17 13:33:27 PST
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 David :Bienvenu 2008-01-18 09:42:23 PST
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 :-)
Comment 4 Daniel Veditz [:dveditz] 2008-01-19 09:22:40 PST
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).
Comment 5 Daniel Veditz [:dveditz] 2008-01-24 10:44:36 PST
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 Dan Mosedale (:dmose) 2008-01-24 10:47:12 PST
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.
Comment 7 Daniel Veditz [:dveditz] 2008-01-26 00:11:40 PST
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.
Comment 8 David :Bienvenu 2008-01-26 11:49:12 PST
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
Comment 9 Dan Mosedale (:dmose) 2008-01-26 16:26:07 PST
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.
Comment 10 Dan Mosedale (:dmose) 2008-01-26 16:26:46 PST
Er, the "here" comment refers to the slen line above it, not below it.
Comment 11 Daniel Veditz [:dveditz] 2008-01-26 16:54:11 PST
> 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.
Comment 12 Dan Mosedale (:dmose) 2008-01-26 16:57:11 PST
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 13 Samuel Sidler (old account; do not CC) 2008-01-27 21:24:17 PST
Comment on attachment 299387 [details] [diff] [review]
alternate patch (1.8 branch version)

Approved for 1.8.1.12. a=ss for release drivers.
Comment 14 Daniel Veditz [:dveditz] 2008-01-27 22:21:04 PST
will fix the tab wonkiness on the lines I touched.
Comment 15 Daniel Veditz [:dveditz] 2008-01-28 02:18:42 PST
Fixed on 1.8 branch.
Comment 16 Al Billings [:abillings] 2008-02-05 13:12:15 PST
Do we have a good way to verify the fix for QA?
Comment 17 Al Billings [:abillings] 2008-02-25 17:06:55 PST
The answer to this question seems to be "No."
Comment 18 David :Bienvenu 2008-02-25 17:11:31 PST
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?)
Comment 19 Al Billings [:abillings] 2008-02-25 17:33:15 PST
QA has no access to a TB debug build built from the 1.8.1.12 release branch, unfortunately.
Comment 20 Daniel Veditz [:dveditz] 2008-02-26 11:55:46 PST
Missed this bit in the initial message to security:

> IX. CREDIT
> 
> This vulnerability was reported to iDefense by regenrecht.
Comment 21 Dan Mosedale (:dmose) 2008-03-04 17:55:34 PST
Dan/David: what do you think about landing this on the trunk too?
Comment 22 David :Bienvenu 2008-03-04 18:01:13 PST
It should be on the trunk.
Comment 23 Emre Birol 2008-03-06 08:57:25 PST
I don't see the patch applied to the file on the trunk yet. Can somebody confirm otherwise?
Comment 24 Magnus Melin 2008-03-06 09:48:44 PST
No, it's not on trunk yet.
Comment 25 Alexander Sack 2008-03-18 05:00:43 PDT
Comment on attachment 299387 [details] [diff] [review]
alternate patch (1.8 branch version)

a=asac for 1.8.0.15
Comment 26 Mike Schroepfer 2008-03-19 12:51:34 PDT
Let's get this on trunk asap..
Comment 27 Christopher Aillon (sabbatical, not receiving bugmail) 2008-03-20 13:54:57 PDT
Landed on the 1.8.0.15 branch
Comment 28 Joe Sabash [:JoeS1] 2008-03-20 16:29:54 PDT
(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.


Comment 31 Mike Schroepfer 2008-03-21 12:10:15 PDT
Is there a trunk patch for this?
Comment 32 Mike Schroepfer 2008-03-23 23:54:19 PDT
Taking off the 1.9 blocking list since this appears to be mailnews only - please re-nom if this is wrong...
Comment 33 Magnus Melin 2008-04-03 01:31:04 PDT
So, according to comment #22 we should get the patch on trunk. (All hunks FAIL currently though.)

Is anyone working on it?
Comment 34 Emre Birol 2008-04-03 15:48:10 PDT
Is there any specific reason not to land it to trunk as it is?
Comment 35 Samuel Sidler (old account; do not CC) 2008-04-03 16:02:46 PDT
Because all hunks currently fail, according to comment 33.
Comment 36 Emre Birol 2008-04-11 15:26:30 PDT
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
Comment 37 Daniel Veditz [:dveditz] 2008-04-17 13:09:04 PDT
Comment on attachment 315196 [details] [diff] [review]
alternate path (trunk version)

r=dveditz
Comment 38 Dan Mosedale (:dmose) 2008-04-18 10:37:07 PDT
Comment on attachment 315196 [details] [diff] [review]
alternate path (trunk version)

sr=dmose
Comment 39 Phil Ringnalda (:philor) 2008-04-18 23:26:56 PDT
mailnews/mime/src/mimeebod.cpp 1.32

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