257314 - stack based buffer overflow with vcards when previewing email message

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[georgi - hopefully not receiving bugspam]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040115
Build Identifier: Mozilla/5.0

There is stack based bufferoverflow in mozilla mail when handling vcards.
The stack is quite screwed.
It is triggered by just previewing a specially crafted message.
Tested on mozilla 1.7 (because 1.7.2 fails to build due to missing directories),
but the code seems present in 1.7.2 and thunderbird 0.7 also.


Obviously the following code is quite fucked:
--------
addrbook/src/nsVCardObj.cpp
static void writeGroup(OFile *fp, VObject *o)
{
    char buf1[256];
    char buf2[256];
    PL_strcpy(buf1,NAME_OF(o));
    while ((o=isAPropertyOf(o,VCGroupingProp)) != 0) {
  PL_strcpy(buf2,STRINGZ_VALUE_OF(o));
  PL_strcat(buf2,".");
  PL_strcat(buf2,buf1);
  PL_strcpy(buf1,buf2);
  }
    appendsOFile(fp,buf1);
}

--------

To reproduce send the vcard fuckmicrosft.vcf (which will be shortly attached)
as attachment and then preview the message.

--------------------------------------------------
(gdb) info stack
#9  0x45962c5b in initPropIterator (i=0xbfffe2a8, o=0x67676767)
    at nsVCardObj.cpp:383
#10 0x45962d2a in isAPropertyOf (o=0x67676767, id=0x4598bc69 "grouping")
    at nsVCardObj.cpp:418
#11 0x45964552 in writeGroup (fp=0x67676767, o=0x67676767)
    at nsVCardObj.cpp:1350
#12 0x67676767 in ?? ()
#13 0x67676767 in ?? ()
#14 0x67676767 in ?? ()
#15 0x67676767 in ?? ()
#16 0x67676767 in ?? ()
#17 0x67676767 in ?? ()
#18 0x67676767 in ?? ()
-------------------------------------------------

georgi



Reproducible: Always
Steps to Reproduce:
1. will attach a testcase


Actual Results:  
stack is screwed

Expected Results:  
stack should not be screwed

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: vcard.vcf which causes stack overflow

vcard.vcf which causes stack overflow when previewed in mail message.

Comment 2

[Gervase Markham [:gerv]]

I have the preview pane turned on and had no problems with the message George
sent to the security list.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616

Gerv

Comment 3

[georgi - hopefully not receiving bugspam]

Gerv: are you showing attachments inline?

The default option is to show attachments inline i believe.

Comment 4

[Asa Dotzler [:asa]]

Heh, the email to security@mozilla kept crashing me every time I tried to open
it. I finally read the summary and checked the bug before trying to read the
mail again. I can reproduce a crash viewing Georgi's titled "bug 257314" on a
Thunderbird branch build from yesterday.

Comment 5

[georgi - hopefully not receiving bugspam]

the same fucked code is in: other-licenses/libical/src/libicalvcal/vobject.c

http://lxr.mozilla.org/seamonkey/source/other-licenses/libical/src/libicalvcal/vobject.c#1253

probably it also should be fixed.

Comment 6

[Ere Maijala (slow)]

Attachment: Patch for addrbook/src/nsVCardObj.cpp

This is a patch to use string class instead of char arrays for
addrbook/src/nsVCardObj.cpp.

Comment 7

[Ere Maijala (slow)]

I checked the original libical package libical-0.24.RC4.tar.gz and it has the
same bad code as the one in other-licenses. Libical developers should be
notified about that, right? 

Comment 8

[Ere Maijala (slow)]

Looks to me like other-licenses/libical/src/libicalvcal/vobject.c is not used at
least in SeaMonkey with calendar.

Comment 9

[Ben Bucksch (:BenB)]

Simple HTML not vulnerable.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 157352 [details] [diff] [review]
Patch for addrbook/src/nsVCardObj.cpp

I'm not a mailnews peer, let's get the r= from mscott;I'll sr=

scott: while you're reviewing, please check out nsDirPrefs.cpp
in the same directory which also has a lot of unchecked strcats and strcpys.
Less susceptible to a remote hacker, but is there any way these
prefs could exceed 128 chars? Seems possible since some are hostname
based and I've seen some exceptionally long hostnames (though rare).
The longest ldap pref I currently have is 60 chars so it's unlikely
to overflow by accident, but hackers are not accidental. ("Get your
free LDAP here! e-mail addresses of your favorite celebrities!").

Anyway, back to this...

>+  while ((o=isAPropertyOf(o,VCGroupingProp)) != 0) {
>+    buf.Insert(NS_LITERAL_CSTRING("."), 0);
>+    buf.Insert(STRINGZ_VALUE_OF(o), 0);

What are the guarantees about this data structure? Since Insert
is slow if we're sure o->val.strs is non-null you could
combine these using nsDependentString

      but.Insert(nsDependentString(STRINGZ_VALUE_OF(o) +
		 NS_LITERAL_CSTRING("."), 0);

But what you have works and isn't going to be called much,
so unless we're sure I'm happy with the patch as-is

sr=dveditz

Comment 11

[Daniel Veditz [:dveditz]]

I can at least say it's blocking 1.7x and 1.8a4; I'm going out on a limb and
making this block ff 1.0PR and 1.7.3 as well and let Asa/BenG clear it.

Comment 12

[Scott MacGregor]

Comment on attachment 157352 [details] [diff] [review]
Patch for addrbook/src/nsVCardObj.cpp

I'm not positive that the old vcard string code can promise that these strings
are null terminated, so like Dan I'm afraid to use the nsDependentString
shortcut. 

To answer Dan's other question, those other calls could be problematic too.
Maybe we should get another bug going for someone to step through the debugger
and take a closer look.

Comment 13

[Ere Maijala (slow)]

Patch checked in to SeaMonkey trunk. Could someone drive it in for the others?

Comment 14

[Scott MacGregor]

fixed for aviary

Comment 15

[Daniel Veditz [:dveditz]]

Checked in on 1.7 and 1.7.2 branches

Comment 16

[Daniel Veditz [:dveditz]]

fixed on trunk per comment 13

Comment 17

[Marc Bejarano]

re: comment 5 and comment 7 about fixing libical upstream

it looks like the bad code that georgi uncovered was already found and fixed by
KDE long ago.

unfortunately, it didn't make it to the libical CVS until shortley after their
last release :(

according to
http://cvs.sourceforge.net/viewcvs.py/freeassociation/libical/src/libicalvcal/vobject.c
rev 1.4 of freeassociation/libical/src/libicalvcal/vobject.c was checked in on
12/16/02 with the following comment:
--
Fix possible buffer overflows. This comes from a code review of KDE.

Obtained from: Waldo Bastian <[e-mail]>
--
diff available at
http://cvs.sourceforge.net/viewcvs.py/freeassociation/libical/src/libicalvcal/vobject.c?r1=1.3&r2=1.4

should waldo bastian also be given credit at
http://www.mozilla.org/projects/security/known-vulnerabilities.html ?

btw: i opened Bug 263515 to track updating of other-licenses/libical

Comment 18

[Marc Bejarano]

bug 272691 tracks crediting waldo

Comment 19

[Marcia Knous [:marcia]]

Tested using Mac 1.75 - 20041216 - 09 - looks good to me, no crash when
attaching vcard to a mail message and then previewing it.

Comment 20

[Tracy Walker [:tracy]]

verified with windows 1.7.5 2004-12-16-10-1.7