stack based buffer overflow with vcards when previewing email message

VERIFIED FIXED

Status

()

Core
Security
--
critical
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: dveditz)

Tracking

({fixed-aviary1.0, fixed1.7.3})

Trunk
fixed-aviary1.0, fixed1.7.3
Points:
---
Bug Flags:
blocking1.7.5 +
blocking-aviary1.0PR +
blocking1.8a4 +

Firefox Tracking Flags

(Not tracked)

Details

([sg:fix])

Attachments

(2 attachments)

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
Created attachment 157317 [details]
vcard.vcf which causes stack overflow

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.

Comment 4

13 years ago
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.

Comment 6

13 years ago
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

13 years ago
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? 

Updated

13 years ago
Attachment #157352 - Flags: review?(dveditz)

Comment 8

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

Comment 9

13 years ago
Simple HTML not vulnerable.
Flags: blocking1.8a4?
Flags: blocking1.7.x?
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0?
(Assignee)

Comment 10

13 years ago
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)
(Assignee)

Comment 11

13 years ago
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: 256195
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 12

13 years ago
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+

Comment 13

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

Comment 14

13 years ago
fixed for aviary
Whiteboard: [sg:fix] → [sg:fix],fixed-aviary1.0
(Assignee)

Comment 15

13 years ago
Checked in on 1.7 and 1.7.2 branches
Keywords: fixed-aviary1.0, fixed1.7.x
Whiteboard: [sg:fix],fixed-aviary1.0 → [sg:fix] fixed1.7.2+
(Assignee)

Comment 16

13 years ago
fixed on trunk per comment 13
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0?
(Assignee)

Updated

13 years ago
Group: security
Whiteboard: [sg:fix] fixed1.7.2+ → [sg:fix] fixed1.7.3
(Assignee)

Updated

13 years ago
Blocks: 256197
(Assignee)

Updated

13 years ago
No longer blocks: 256195

Comment 17

13 years ago
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

12 years ago
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
(Assignee)

Updated

12 years ago
Keywords: fixed1.7.5 → fixed1.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.