The default bug view has changed. See this FAQ.

Double free in nsVCard.cpp

VERIFIED FIXED

Status

MailNews Core
Address Book
--
critical
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: emk, Assigned: Bienvenu)

Tracking

({fixed1.8.1, verified1.8.0.4})

Trunk
x86
Windows XP
fixed1.8.1, verified1.8.0.4
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] need branch landing)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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%).
(Assignee)

Comment 1

11 years ago
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...
Assignee: nobody → bienvenu
Status: NEW → ASSIGNED
Attachment #218720 - Flags: superreview?(mscott)

Updated

11 years ago
Attachment #218720 - Flags: superreview?(mscott) → superreview+
(Assignee)

Updated

11 years ago
Attachment #218720 - Flags: approval1.8.0.3?
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 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?]
(Assignee)

Comment 3

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

Comment 6

11 years ago
Created attachment 219683 [details] [diff] [review]
fix including handling out of memory

just break out of loop when we run out of memory here.
Attachment #219683 - Flags: superreview?

Comment 7

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

Comment 8

11 years ago
heh, yes, thx.
(Assignee)

Comment 9

11 years ago
oom memory fix also checked in.
(Assignee)

Updated

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

Updated

11 years ago
Keywords: fixed1.8.0.4

Comment 11

11 years ago
bienvenu, any idea how to test this?
(Assignee)

Comment 12

11 years ago
Perhaps the reporter has a test case he could provide
(Reporter)

Comment 13

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

Comment 14

11 years ago
(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!
(Reporter)

Comment 15

11 years ago
(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.
Keywords: fixed1.8.0.4 → verified1.8.0.4
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
You need to log in before you can comment on or make changes to this bug.