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

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: 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.)

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1241034-utf8-ucs2-len-hg1.diff

Slight adjustment: do the new tests after the other ones, for more appropriate failure messages.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/5fde729fdbff

Comment 6

[Daniel Veditz [:dveditz]]

Tracking upstream NSS 3.21.3 security updates for the ESR-45 "50+" release.

Comment 7

[Kai Engert (:KaiE:)]

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Fixed in bug 1310009