Closed Bug 334384 Opened 15 years ago Closed 15 years ago

Double free in nsVCard.cpp

Categories

(MailNews Core :: Address Book, defect)

x86
Windows XP
defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: emk, Assigned: Bienvenu)

Details

(Keywords: fixed1.8.1, verified1.8.0.4, Whiteboard: [sg:critical?] need branch landing)

Attachments

(2 files)

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%).
Attached patch proposed fixSplinter Review
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...
Assignee: nobody → bienvenu
Status: NEW → ASSIGNED
Attachment #218720 - Flags: superreview?(mscott)
Attachment #218720 - Flags: superreview?(mscott) → superreview+
Attachment #218720 - Flags: approval1.8.0.3?
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
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.
Flags: blocking1.8.0.3?
Whiteboard: [sg:critical?]
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.
(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?
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 218720 [details] [diff] [review]
proposed fix

minus the patch, need to fix the realloc bit too.
Attachment #218720 - Flags: approval1.8.0.3? → approval1.8.0.3-
just break out of loop when we run out of memory here.
Attachment #219683 - Flags: superreview?
Comment on attachment 219683 [details] [diff] [review]
fix including handling out of memory

I assume you wanted me to review this? :)
Attachment #219683 - Flags: superreview? → superreview+
heh, yes, thx.
oom memory fix also checked in.
Attachment #219683 - Flags: approval1.8.0.4?
Comment on attachment 219683 [details] [diff] [review]
fix including handling out of memory

a=dveditz for 1.8.0 and 1.8 branches
Attachment #219683 - Flags: approval1.8.0.4?
Attachment #219683 - Flags: approval1.8.0.4+
Attachment #219683 - Flags: approval-branch-1.8.1+
Whiteboard: [sg:critical?] → [sg:critical?] need branch landing
Keywords: fixed1.8.0.4
bienvenu, any idea how to test this?
Perhaps the reporter has a test case he could provide
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).
Status: RESOLVED → VERIFIED
(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!
(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.
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Group: security
If anyone has ideas on how to verify this on Thunderbird 2 (rc1), please help out.
Product: Core → MailNews Core
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
You need to log in before you can comment on or make changes to this bug.