Last Comment Bug 334384 - Double free in nsVCard.cpp
: Double free in nsVCard.cpp
Status: VERIFIED FIXED
[sg:critical?] need branch landing
: fixed1.8.1, verified1.8.0.4
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: David :Bienvenu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-17 13:26 PDT by Masatoshi Kimura [:emk]
Modified: 2008-07-31 04:30 PDT (History)
8 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (988 bytes, patch)
2006-04-17 13:48 PDT, David :Bienvenu
mscott: superreview+
dveditz: approval1.8.0.4-
Details | Diff | Review
fix including handling out of memory (1.54 KB, patch)
2006-04-24 16:45 PDT, David :Bienvenu
mscott: superreview+
dveditz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review

Description Masatoshi Kimura [:emk] 2006-04-17 13:26:44 PDT
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsVCard.cpp&rev=1.6#976
|oldBytes| would become invalid when PR_Realloc succeeded.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsVCard.cpp&rev=1.6#942
Therefore |oldBytes| should be freed only if |bytes| is null.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsVCard.cpp&rev=1.6#995
|oldBytes| handled correctly here.

Malicious vCard attachment may compromise the system. Although I have no exploit, I confirmed that invalid vCard caused a hang up (CPU 100%).
Comment 1 David :Bienvenu 2006-04-17 13:48:46 PDT
Created attachment 218720 [details] [diff] [review]
proposed fix

yes, you're right - so we should check if bytes is null before freeing oldBytes. And we don't need to null the resulting pointer since we're going to return soon anyway...
Comment 2 Daniel Veditz [:dveditz] 2006-04-17 19:28:21 PDT
What about when realloc fails? does mime_error() break the while loop somehow, like set a flag I'm not seeing? Seems like next time around it'll see that bytes==null and start over allocating after setting bytesMax = 1024, which is guaranteed to be too small if we've already tried to realloc.
Comment 3 David :Bienvenu 2006-04-20 20:16:34 PDT
yes, you're right, it doesn't handle realloc failing - that should be quite a bit less exploitable, however, and is orthogonal to this fix.
Comment 4 Daniel Veditz [:dveditz] 2006-04-21 11:55:36 PDT
(In reply to comment #3)
> yes, you're right, it doesn't handle realloc failing - that should be quite a
> bit less exploitable, however, and is orthogonal to this fix.

When the patch lands people will look at this area of code; we need to fix both problems. Do we really need another bug to fix the other part?
Comment 5 Daniel Veditz [:dveditz] 2006-04-21 11:56:25 PDT
Comment on attachment 218720 [details] [diff] [review]
proposed fix

minus the patch, need to fix the realloc bit too.
Comment 6 David :Bienvenu 2006-04-24 16:45:19 PDT
Created attachment 219683 [details] [diff] [review]
fix including handling out of memory

just break out of loop when we run out of memory here.
Comment 7 Scott MacGregor 2006-05-01 15:14:25 PDT
Comment on attachment 219683 [details] [diff] [review]
fix including handling out of memory

I assume you wanted me to review this? :)
Comment 8 David :Bienvenu 2006-05-01 15:15:54 PDT
heh, yes, thx.
Comment 9 David :Bienvenu 2006-05-01 16:25:37 PDT
oom memory fix also checked in.
Comment 10 Daniel Veditz [:dveditz] 2006-05-01 18:48:10 PDT
Comment on attachment 219683 [details] [diff] [review]
fix including handling out of memory

a=dveditz for 1.8.0 and 1.8 branches
Comment 11 Bob Clary [:bc:] 2006-05-22 10:40:18 PDT
bienvenu, any idea how to test this?
Comment 12 David :Bienvenu 2006-05-22 11:00:48 PDT
Perhaps the reporter has a test case he could provide
Comment 13 Masatoshi Kimura [:emk] 2006-05-22 13:46:40 PDT
Sorry, I can't provide the testcase because it contains private info.
I verified that the double free problem was fixed, but unfortunatly, this didn't fix the freeze problem. It was another problem (bug 334947).
Comment 14 Bob Clary [:bc:] 2006-05-22 14:03:12 PDT
(In reply to comment #13)

 Masatoshi, could you check with a Firefox 1.5.0.4 candidate from <http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/1.5.0.4-candidates/rc3/> ? Thanks!
Comment 15 Masatoshi Kimura [:emk] 2006-05-23 07:09:11 PDT
(In reply to comment #14)
>  Masatoshi, could you check with a Firefox 1.5.0.4 candidate from
<http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/1.5.0.4-candidates/rc3/>
> ? Thanks!
I had to use debugger to confirm because freeze still occurs until bug 334947 is fixed. But I've confirmed anyway.
Comment 16 juan becerra [:juanb] 2007-03-30 16:30:03 PDT
If anyone has ideas on how to verify this on Thunderbird 2 (rc1), please help out.

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