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.
Possibly exploitable if previous actions leave interesting user data as garbage on the stack.
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 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
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 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.
Please land on trunk and 1.8 branches first
Comment on attachment 223964 [details] [diff] [review] possible fix approved for 1.8.0 branch, a=dveditz for drivers
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!
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.