Closed Bug 257314 Opened 20 years ago Closed 20 years ago

stack based buffer overflow with vcards when previewing email message

Categories

(Core :: Security, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: guninski, Assigned: dveditz)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.3, Whiteboard: [sg:fix])

Attachments

(2 files)

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
vcard.vcf which causes stack overflow when previewed in mail message.
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
Gerv: are you showing attachments inline?

The default option is to show attachments inline i believe.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
This is a patch to use string class instead of char arrays for
addrbook/src/nsVCardObj.cpp.
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? 
Attachment #157352 - Flags: review?(dveditz)
Looks to me like other-licenses/libical/src/libicalvcal/vobject.c is not used at
least in SeaMonkey with calendar.
Simple HTML not vulnerable.
Flags: blocking1.8a4?
Flags: blocking1.7.x?
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0?
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
Attachment #157352 - Flags: superreview+
Attachment #157352 - Flags: review?(mscott)
Attachment #157352 - Flags: review?(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.
Blocks: sbb?
Flags: blocking1.8a4?
Flags: blocking1.8a4+
Flags: blocking1.7.x?
Flags: blocking1.7.x+
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0PR+
Whiteboard: [sg:fix]
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.
Attachment #157352 - Flags: review?(mscott) → review+
Patch checked in to SeaMonkey trunk. Could someone drive it in for the others?
fixed for aviary
Whiteboard: [sg:fix] → [sg:fix],fixed-aviary1.0
Checked in on 1.7 and 1.7.2 branches
Whiteboard: [sg:fix],fixed-aviary1.0 → [sg:fix] fixed1.7.2+
fixed on trunk per comment 13
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0?
Group: security
Whiteboard: [sg:fix] fixed1.7.2+ → [sg:fix] fixed1.7.3
Blocks: sbb+
No longer blocks: sbb?
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
bug 272691 tracks crediting waldo
Tested using Mac 1.75 - 20041216 - 09 - looks good to me, no crash when
attaching vcard to a mail message and then previewing it.
verified with windows 1.7.5 2004-12-16-10-1.7
Status: RESOLVED → VERIFIED
Keywords: fixed1.7.5fixed1.7.3
Whiteboard: [sg:fix] fixed1.7.3 → [sg:fix]
You need to log in before you can comment on or make changes to this bug.