Last Comment Bug 257314 - stack based buffer overflow with vcards when previewing email message
: stack based buffer overflow with vcards when previewing email message
Status: VERIFIED FIXED
[sg:fix]
: fixed-aviary1.0, fixed1.7.3
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
Depends on:
Blocks: sbb+
  Show dependency treegraph
 
Reported: 2004-08-29 06:23 PDT by georgi - hopefully not receiving bugspam
Modified: 2006-03-12 17:55 PST (History)
9 users (show)
dveditz: blocking1.7.5+
dveditz: blocking‑aviary1.0PR+
dveditz: blocking1.8a4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
vcard.vcf which causes stack overflow (1.03 KB, text/x-vcard)
2004-08-29 06:26 PDT, georgi - hopefully not receiving bugspam
no flags Details
Patch for addrbook/src/nsVCardObj.cpp (955 bytes, patch)
2004-08-29 13:08 PDT, Ere Maijala (slow)
mscott: review+
dveditz: superreview+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2004-08-29 06:23:48 PDT
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 2004-08-29 06:26:26 PDT
Created attachment 157317 [details]
vcard.vcf which causes stack overflow

vcard.vcf which causes stack overflow when previewed in mail message.
Comment 2 Gervase Markham [:gerv] 2004-08-29 07:39:18 PDT
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 2004-08-29 08:13:43 PDT
Gerv: are you showing attachments inline?

The default option is to show attachments inline i believe.
Comment 4 Asa Dotzler [:asa] 2004-08-29 09:18:31 PDT
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 2004-08-29 11:05:58 PDT
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) 2004-08-29 13:08:23 PDT
Created attachment 157352 [details] [diff] [review]
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) 2004-08-29 13:18:28 PDT
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) 2004-08-29 13:54:49 PDT
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) 2004-08-29 16:34:37 PDT
Simple HTML not vulnerable.
Comment 10 Daniel Veditz [:dveditz] 2004-08-30 09:55:51 PDT
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] 2004-08-30 09:59:12 PDT
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 2004-08-30 12:35:51 PDT
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) 2004-08-30 12:42:17 PDT
Patch checked in to SeaMonkey trunk. Could someone drive it in for the others?
Comment 14 Scott MacGregor 2004-08-30 12:46:15 PDT
fixed for aviary
Comment 15 Daniel Veditz [:dveditz] 2004-08-31 00:46:28 PDT
Checked in on 1.7 and 1.7.2 branches
Comment 16 Daniel Veditz [:dveditz] 2004-08-31 15:21:17 PDT
fixed on trunk per comment 13
Comment 17 Marc Bejarano 2004-10-08 11:33:02 PDT
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 <bastian@kde.org>
--
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 2004-12-01 17:52:56 PST
bug 272691 tracks crediting waldo
Comment 19 Marcia Knous [:marcia - use ni] 2004-12-16 14:47:35 PST
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] 2004-12-16 15:38:47 PST
verified with windows 1.7.5 2004-12-16-10-1.7

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