Out-of-bounds write (buffer overflow) in sec_port_ucs2_utf8_conversion_function

RESOLVED FIXED in Firefox 47

Status

NSS
Libraries
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: jld, Assigned: jld)

Tracking

({sec-moderate})

trunk
3.23
sec-moderate

Firefox Tracking Flags

(firefox46 wontfix, firefox47 fixed, firefox-esr38 wontfix, firefox-esr4550+ fixed)

Details

(Whiteboard: [adv-main47+])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
Keywords: sec-moderate
(Assignee)

Comment 2

a year ago
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.
(Assignee)

Comment 3

a year ago
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.)
(Assignee)

Comment 4

a year ago
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
(Assignee)

Updated

a year ago
Attachment #8713909 - Flags: review?(ttaubert)
Attachment #8713909 - Flags: review?(ttaubert) → review+
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/nss/rev/5fde729fdbff
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Group: crypto-core-security → core-security-release
Target Milestone: --- → 3.23
status-firefox46: affected → wontfix
status-firefox47: --- → fixed
status-firefox-esr38: --- → wontfix
status-firefox-esr45: --- → affected
Whiteboard: [adv-main47+]
status-firefox-esr45: affected → wontfix
Tracking upstream NSS 3.21.3 security updates for the ESR-45 "50+" release.
status-firefox-esr45: wontfix → affected
tracking-firefox-esr45: --- → 50+

Comment 7

7 months ago
NSS_3_21_BRANCH
https://hg.mozilla.org/projects/nss/rev/e51a1972bd8c

Updated

7 months ago
Blocks: 1310009
Fixed in bug 1310009
status-firefox-esr45: affected → fixed
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.