339740 - extraneous '=' in vcard base64 leads to heap overwrite (crash)

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Daniel Veditz [:dveditz]]

In lexGetDataFromBase64 (nsVCard.cpp) base64 padding characters ('=') are accepted anywhere in the string and the "pad" count is never reset. Pad characters anywhere but the last one or two characters are illegal. Treating them as 'A' would be relatively benign if it weren't for the pad count. This ultimately can make numOut negative which is passed to memcpy() as a very large unsigned size_t. If this doesn't crash you bytesLen is then decremented before you loop back around and do the same thing over.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsVCard.cpp&rev=1.7&mark=939,970,986-987#937

A '=' when quadIx is 0 or 1 is always invalid base64, and at the very least you should reset pad to 0 when quadIx is reset. Or at that point if pad != 0 assume you're at the end of the string and quit, or look ahead and return an error if you're not at the end of the string.

Comment 1

[Daniel Veditz [:dveditz]]

Possibly exploitable if previous actions leave interesting user data as garbage on the stack.

Comment 2

[David :Bienvenu]

Attachment: possible fix

this is untested - do you have a test case to try, Dan? This should treat mal-formed base64 as an error and fall into the normal error handling.

Comment 3

[(not reading, please use [e-mail] instead)]

Comment on attachment 223964 [details] [diff] [review]
possible fix

r=sspitzer, acting reviewer for mailnews while mscott is away.

I forgot the vcard code had it's own base64 decoder (and it's own quoted printable decoder, see lexGetQuotedPrintable()

two questions:  does the nspr version (mozilla/nsprpub/lib/libc/src/base64.c) have this heap overwrite issue?

2) should we log a spin off bug about switching to the existing implementations of base64 (and quoted printable) decoding in the tree?

as for base64, see also toolkit/components/url-classifier/content/moz/base64.js

Comment 4

[David :Bienvenu]

I don't think it would be easy to switch to an other parser since we don't know how big the base64 data is - we have to parse the data one character at a time to find that out, and nspr wants the whole base64 data buffer to start with.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 223964 [details] [diff] [review]
possible fix

This isn't the only spot with its own base64 code :-(

sr=dveditz for fixing the potential buffer problem. This will now accept not-quite-valid base64 but in a benign way.

Comment 6

[Daniel Veditz [:dveditz]]

Please land on trunk and 1.8 branches first

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 223964 [details] [diff] [review]
possible fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 8

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with code inspection.  If anyone has a way to test this, please add a testcase to the bug.  Thanks!

Comment 9

[Wayne Mery (:wsmwk)]

This FIXED bug is flagged with in‑testsuite?   It would be great if assignee or someone else can clear the flag if a test is not appropriate.  And if appropriate, create a test and plus the flag to finish off the bug.