Closed Bug 1241037 Opened 4 years ago Closed 4 years ago

sec_port_ucs2_utf8_conversion_function detects UTF-16 surrogates incorrectly


(NSS :: Libraries, defect)

Not set


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

Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 50+ fixed


(Reporter: jld, Assigned: jld)



(Keywords: sec-moderate, Whiteboard: [adv-main47+])


(1 file, 2 obsolete files)

Found by visual inspection (ellipsis for clarity):

    324       else if( ((inBuf[i+0+H_0] & 0xDC) == 0xD8) ) {
    325         if( ((inBuf[i+2+H_0] & 0xDC) == 0xDC) …

These conditions are incorrect, as are similar ones later in the function.  The mask should be 0xFC; as written, this will misidentify anything between U+F800 and U+FBFF as a high surrogate and anything between U+FC00 and U+FFFF as a low surrogate.  This means that characters in the first range are usually considered invalid encodings, or if they occur before a character in the second range will be silently mistranslated.  (Arabic Presentation Forms occur in both of those ranges, incidentally.)

For some reason, unpaired low surrogates aren't rejected like unpaired high surrogates; they're just transcribed as-is into (equally invalid) UTF-8, so this doesn't affect un-"paired" instances of characters in the U+FC00 to U+FFFF range.

Not sure if this is really security-sensitive, but it looks like it's used by certificate processing, so I don't know if there's some way that a crafted certificate could be misinterpreted to assert something it shouldn't be.
Updated patch.  Depends on the patch for bug 1206283.  Adds some test cases.
Attachment #8714547 - Attachment is obsolete: true
Attachment #8715093 - Flags: review?(ttaubert)
See Also: → 1206283
Comment on attachment 8715093 [details] [diff] [review]

Review of attachment 8715093 [details] [diff] [review]:

Nice spot, this function is a gift that keeps on giving. It's equally great that its name includes UCS-2 but it does accept UTF-16 surrogate pairs, so actually UTF-16.

::: lib/util/utf8.c
@@ +1161,5 @@
>    { 0xD800, 0xfe, 0 },
>    { 0xD800, 0x3bb, 0 },
>    { 0xD800, 0xD800, 0 },
> +  { 0xD800, 0xFEFF, 0 },
> +  { 0xD800, 0xFFFD, 0 },

Shouldn't we discard low surrogates not preceded by a high surrogate too? How high's the chance we'd break anything with that change?
Attachment #8715093 - Flags: review?(ttaubert) → review+
See Also: → 1246852
(In reply to Tim Taubert [:ttaubert] from comment #3)
> Shouldn't we discard low surrogates not preceded by a high surrogate too?

We should, and I noticed this (see comment #0), but I didn't file a bug for it; I now have.  Thanks for reminding me about that.
Jed, your patch doesn't apply anymore. Can you please updated it so I can push it?
Flags: needinfo?(jld)
Keywords: checkin-needed
Updated; sorry about that.  Looks like it (trivially) conflicted with the post-review changes in bug 1206283.
Attachment #8715093 - Attachment is obsolete: true
Flags: needinfo?(jld)
Attachment #8717959 - Flags: review+
Closed: 4 years ago
Resolution: --- → FIXED
Group: crypto-core-security → core-security-release
Which NSS version did this land in? We need to link this to an NSS version so we can figure out which Firefox release gets the fix.
Flags: needinfo?(ttaubert)
This will be in 3.23 if I'm not mistaken.
Flags: needinfo?(ttaubert)
Target Milestone: --- → 3.23
Whiteboard: [adv-main47+]
Tracking upstream NSS 3.21.3 security updates for the ESR-45 "50+" release.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.