Closed
Bug 334384
Opened 19 years ago
Closed 19 years ago
Double free in nsVCard.cpp
Categories
(MailNews Core :: Address Book, defect)
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)
988 bytes,
patch
|
mscott
:
superreview+
dveditz
:
approval1.8.0.4-
|
Details | Diff | Splinter Review |
1.54 KB,
patch
|
mscott
:
superreview+
dveditz
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
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...
Updated•19 years ago
|
Attachment #218720 -
Flags: superreview?(mscott) → superreview+
Assignee | ||
Updated•19 years ago
|
Attachment #218720 -
Flags: approval1.8.0.3?
Assignee | ||
Updated•19 years ago
|
Comment 2•19 years ago
|
||
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.
Updated•19 years ago
|
Flags: blocking1.8.0.3?
Whiteboard: [sg:critical?]
Assignee | ||
Comment 3•19 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.
Comment 4•19 years ago
|
||
(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 5•19 years ago
|
||
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•19 years ago
|
||
just break out of loop when we run out of memory here.
Attachment #219683 -
Flags: superreview?
Comment 7•19 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•19 years ago
|
||
heh, yes, thx.
Assignee | ||
Comment 9•19 years ago
|
||
oom memory fix also checked in.
Assignee | ||
Updated•19 years ago
|
Attachment #219683 -
Flags: approval1.8.0.4?
Comment 10•19 years ago
|
||
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+
Updated•19 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?] need branch landing
Assignee | ||
Updated•19 years ago
|
Keywords: fixed1.8.0.4
Comment 11•19 years ago
|
||
bienvenu, any idea how to test this?
Assignee | ||
Comment 12•19 years ago
|
||
Perhaps the reporter has a test case he could provide
Reporter | ||
Comment 13•19 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•19 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•19 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
Updated•19 years ago
|
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Updated•19 years ago
|
Group: security
Comment 16•18 years ago
|
||
If anyone has ideas on how to verify this on Thunderbird 2 (rc1), please help out.
Updated•16 years ago
|
Product: Core → MailNews Core
Updated•5 years ago
|
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.
Description
•