1241037 - sec_port_ucs2_utf8_conversion_function detects UTF-16 surrogates incorrectly

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

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

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.

Comment 1

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

Attachment: bug1241037-utf8-surrogate-hg0.diff

Comment 2

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

Attachment: bug1241037-utf8-surrogate-hg1.diff

Updated patch.  Depends on the patch for bug 1206283.  Adds some test cases.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8715093 [details] [diff] [review]
bug1241037-utf8-surrogate-hg1.diff

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?

Comment 4

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

(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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Jed, your patch doesn't apply anymore. Can you please updated it so I can push it?

Comment 6

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

Attachment: bug1241037-utf8-surrogate-hg2.diff

Updated; sorry about that.  Looks like it (trivially) conflicted with the post-review changes in bug 1206283.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/329932eb1700

Comment 8

[Daniel Veditz [:dveditz]]

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.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

This will be in 3.23 if I'm not mistaken.

Comment 10

[Daniel Veditz [:dveditz]]

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

Comment 11

[Kai Engert (:KaiE:)]

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Fixed in bug 1310009