334384 - Double free in nsVCard.cpp

Status: VERIFIED FIXED

(MailNews Core :: Address Book, defect)

Description

[Masatoshi Kimura [:emk]]

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]

Attachment: 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]]

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]

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]]

(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]]

Comment on attachment 218720 [details] [diff] [review]
proposed fix

minus the patch, need to fix the realloc bit too.

Comment 6

[David :Bienvenu]

Attachment: fix including handling out of memory

just break out of loop when we run out of memory here.

Comment 7

[Scott MacGregor]

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]

heh, yes, thx.

Comment 9

[David :Bienvenu]

oom memory fix also checked in.

Comment 10

[Daniel Veditz [:dveditz]]

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] (inactive)]

bienvenu, any idea how to test this?

Comment 12

[David :Bienvenu]

Perhaps the reporter has a test case he could provide

Comment 13

[Masatoshi Kimura [:emk]]

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] (inactive)]

(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]]

(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]]

If anyone has ideas on how to verify this on Thunderbird 2 (rc1), please help out.