Found by visual inspection of the code in bug 1206283 comment #3: > if( (inBuf[i+H_0] == 0x00) && ((inBuf[i+H_0] & 0x80) == 0x00) ) len += 1; The second H_0 should be H_1; this gives a UTF-8 length of 1 instead of 2 for U+0080 through U+00FF. The code does two passes over the input UTF-16 string, first to compute the UTF-8 length and then (after comparing with the caller-supplied buffer size) to do the actual conversion without further bounds checks. This bug is in the first pass and not the second, so it undercomputes the length and potentially overruns the buffer. (The accompanying tests missed this because they don't cover the case where the buffer is too small, either to verify that conversion fails as expected or that the predicted length returned in that case is correct.) Depending on how this function is used, it could write past the end of a buffer with arbitrary UTF-8. Fortunately, uses inside NSS and Gecko (in particular the one that seems to expose this to certificate processing) seem to use worst-case-sized output buffers and wouldn't be affected, but I haven't audited them thoroughly, and this is also exported from libnssutil, so anything linked against it could be affected.
calling it sec-moderate because Jed says we only call this function with worst-case large enough buffers. if that's not actually true this would become a sec-critical bug.
I traced through the various uses/callers within NSS in cscope; notes: * PORT_UCS2_UTF8Conversion * p12u_ucs2_ascii_conversion_function (This subtree applies only in "pk12util" cmd.) (I don't know why pk12util installs a UTF-8 conversion as the ASCII conversion.) * PORT_UCS2_ASCIIConversion (ASCIIness not enforced?) * P12U_UnicodeConversion WARNING: would be broken if toUnicode could be PR_FALSE. (But this is inside pk12util, so all callers are known.) * p12U_ReadPKCS12File (only caller of P12U_UnicodeConversion) Safe: toUnicode is always PR_TRUE. * sec_pkcs12_create_virtual_password Safe: Always UTF-8 to UTF-16 * sec_pkcs12_convert_item_to_unicode Safe; see below * sec_pkcs12_convert_item_to_unicode Safe: Output buf size is 6 bytes per UTF-16 code unit * pkix_UTF16_to_UTF8 Safe: Output buf size is 4 bytes per UTF-16 code unit * pkix_UTF8_to_UTF16 Safe: Always UTF-8 to UTF-16 More about P12U_UnicodeConversion: it uses 2 bytes per UTF-16 code unit for the output buffer, so an input containing a 3-byte char and an affected 2-byte char would be computed as 4 bytes instead of 5 and overflow the 4-byte buffer it would get (and repeat this for as many bytes of overrun as needed). I haven't tried to actually verify this, because it's dead code. PORT_UCS2_UTF8Conversion is exported, so other projects could be using it. In particular: Gecko/Firefox is: * AppendBMPtoUTF16 in security/manager/ssl/nsNSSCertHelper.cpp Safe: Output buf size is 6 bytes per UTF-16 code unit Also, AppendBMPtoUTF16 is doing a pointless UTF-16 -> UTF-8 -> UTF-16 round-trip for the sole purpose of handling byte order, and this called out in a fixme comment.
Created attachment 8713907 [details] [diff] [review] bug1241034-utf8-ucs2-len-hg0.diff Here's a patch. I've tried to avoid saying "buffer overrun" in as many words, but it's pretty obvious. Even just the one-character fix itself (without the test changes) might obvious — it's a subtle change to a length calculation in string processing — and especially with the bug number as a clue it's sec-related. But see above about exploitability. (I'll figure out the r? later.)
Created attachment 8713909 [details] [diff] [review] bug1241034-utf8-ucs2-len-hg1.diff Slight adjustment: do the new tests after the other ones, for more appropriate failure messages.
Attachment #8713907 - Attachment is obsolete: true
Attachment #8713909 - Flags: review?(ttaubert)
Attachment #8713909 - Flags: review?(ttaubert) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
status-firefox46: affected → wontfix
status-firefox47: --- → fixed
status-firefox-esr38: --- → wontfix
status-firefox-esr45: --- → affected
Tracking upstream NSS 3.21.3 security updates for the ESR-45 "50+" release.
status-firefox-esr45: wontfix → affected
tracking-firefox-esr45: --- → 50+
Fixed in bug 1310009
status-firefox-esr45: affected → fixed
You need to log in before you can comment on or make changes to this bug.