Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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



MailNews Core
Address Book
11 years ago
9 years ago


(Reporter: dveditz, Assigned: Bienvenu)


({crash, fixed1.8.1, verified1.8.0.5})

crash, fixed1.8.1, verified1.8.0.5
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.9a1 +
blocking-thunderbird2 +
blocking1.8.0.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [sg:critical?][need testcase])


(1 attachment)

1.30 KB, patch
(not reading, please use instead)
: review+
: superreview+
Details | Diff | Splinter Review
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.,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.
Flags: blocking1.9a1+
Flags: blocking1.8.0.5+
Flags: blocking1.7.14?
Flags: blocking-thunderbird2+
Flags: blocking-aviary1.0.9?
Whiteboard: [sg:critical?]


11 years ago

Comment 2

11 years ago
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.
Attachment #223964 - Flags: review?


11 years ago
Attachment #223964 - Flags: superreview?(dveditz)
Attachment #223964 - Flags: review?(sspitzer)
Attachment #223964 - Flags: review?
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
Attachment #223964 - Flags: review?(sspitzer) → review+

Comment 4

11 years ago
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.
Attachment #223964 - Flags: superreview?(dveditz) → superreview+


11 years ago
Attachment #223964 - Flags: approval1.8.0.5?
Please land on trunk and 1.8 branches first


11 years ago
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Comment on attachment 223964 [details] [diff] [review]
possible fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #223964 - Flags: approval1.8.0.5? → approval1.8.0.5+


11 years ago
Keywords: fixed1.8.0.5

Comment 8

11 years ago
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!
Keywords: fixed1.8.0.5 → verified1.8.0.5
Whiteboard: [sg:critical?] → [sg:critical?][need testcase]
Group: security
Flags: in-testsuite?
Product: Core → MailNews Core
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.
You need to log in before you can comment on or make changes to this bug.