Last Comment Bug 339740 - extraneous '=' in vcard base64 leads to heap overwrite (crash)
: extraneous '=' in vcard base64 leads to heap overwrite (crash)
Status: RESOLVED FIXED
[sg:critical?][need testcase]
: crash, fixed1.8.1, verified1.8.0.5
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: David :Bienvenu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-30 15:23 PDT by Daniel Veditz [:dveditz]
Modified: 2009-01-12 06:14 PST (History)
8 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.9a1+
dveditz: blocking‑thunderbird2+
dveditz: blocking1.8.0.5+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
possible fix (1.30 KB, patch)
2006-05-31 13:08 PDT, David :Bienvenu
moco: review+
dveditz: superreview+
dveditz: approval1.8.0.5+
Details | Diff | Review

Description Daniel Veditz [:dveditz] 2006-05-30 15:23:38 PDT
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] 2006-05-30 15:29:22 PDT
Possibly exploitable if previous actions leave interesting user data as garbage on the stack.
Comment 2 David :Bienvenu 2006-05-31 13:08:53 PDT
Created attachment 223964 [details] [diff] [review]
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 seth@sspitzer.org instead) 2006-06-13 10:57:26 PDT
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 2006-06-13 11:08:43 PDT
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] 2006-06-14 06:25:34 PDT
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] 2006-06-15 14:27:32 PDT
Please land on trunk and 1.8 branches first
Comment 7 Daniel Veditz [:dveditz] 2006-06-15 14:33:07 PDT
Comment on attachment 223964 [details] [diff] [review]
possible fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 8 Jay Patel [:jay] 2006-06-26 16:07:34 PDT
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, NI for questions) 2009-01-12 06:14:44 PST
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.

Note You need to log in before you can comment on or make changes to this bug.