412363 - (CVE-2008-0304) Buffer overflow in external MIME bodies

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect, P1)

Description

[Daniel Veditz [:dveditz]]

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]

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

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

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]

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

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

Attachment: 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 (:dmosedale, :dmose)]

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

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

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 (:dmosedale, :dmose)]

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 (:dmosedale, :dmose)]

Er, the "here" comment refers to the slen line above it, not below it.

Comment 11

[Daniel Veditz [:dveditz]]

> 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 (:dmosedale, :dmose)]

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

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

will fix the tab wonkiness on the lines I touched.

Comment 15

[Daniel Veditz [:dveditz]]

Fixed on 1.8 branch.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Do we have a good way to verify the fix for QA?

Comment 17

[Al Billings [:abillings - ex-MoCo]]

The answer to this question seems to be "No."

Comment 18

[David :Bienvenu]

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 - ex-MoCo]]

QA has no access to a TB debug build built from the 1.8.1.12 release branch, unfortunately.

Comment 20

[Daniel Veditz [:dveditz]]

Missed this bit in the initial message to security:

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

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Dan/David: what do you think about landing this on the trunk too?

Comment 22

[David :Bienvenu]

It should be on the trunk.

Comment 23

[Emre Birol]

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

Comment 24

[Magnus Melin [:mkmelin]]

No, it's not on trunk yet.

Comment 25

[Alexander Sack]

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

a=asac for 1.8.0.15

Comment 26

[Mike Schroepfer]

Let's get this on trunk asap..

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Landed on the 1.8.0.15 branch

Comment 28

[Joe Sabash [:JoeS1]]

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

[Samuel Sidler (old account; do not CC)]

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

[Joe Sabash [:JoeS1]]

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

[Mike Schroepfer]

Is there a trunk patch for this?

Comment 32

[Mike Schroepfer]

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 [:mkmelin]]

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]

Is there any specific reason not to land it to trunk as it is?

Comment 35

[Samuel Sidler (old account; do not CC)]

Because all hunks currently fail, according to comment 33.

Comment 36

[Emre Birol]

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

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

r=dveditz

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

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

sr=dmose

Comment 39

[Phil Ringnalda (:philor)]

mailnews/mime/src/mimeebod.cpp 1.32