sec_port_ucs2_utf8_conversion_function detects UTF-16 surrogates incorrectly

RESOLVED FIXED in Firefox 47

Status

NSS
Libraries
RESOLVED FIXED
2 years ago
6 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, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

a year ago
Created attachment 8714547 [details] [diff] [review]
bug1241037-utf8-surrogate-hg0.diff
(Assignee)

Comment 2

a year ago
Created attachment 8715093 [details] [diff] [review]
bug1241037-utf8-surrogate-hg1.diff

Updated patch.  Depends on the patch for bug 1206283.  Adds some test cases.
Attachment #8714547 - Attachment is obsolete: true
Attachment #8715093 - Flags: review?(ttaubert)
(Assignee)

Updated

a year ago
See Also: → bug 1206283
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?
Attachment #8715093 - Flags: review?(ttaubert) → review+
(Assignee)

Updated

a year ago
See Also: → bug 1246852
(Assignee)

Comment 4

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

Updated

a year ago
Keywords: checkin-needed
Jed, your patch doesn't apply anymore. Can you please updated it so I can push it?
Status: NEW → ASSIGNED
Flags: needinfo?(jld)
Keywords: checkin-needed
(Assignee)

Comment 6

a year ago
Created attachment 8717959 [details] [diff] [review]
bug1241037-utf8-surrogate-hg2.diff

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+
https://hg.mozilla.org/projects/nss/rev/329932eb1700
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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
status-firefox46: affected → wontfix
status-firefox47: --- → fixed
status-firefox-esr38: --- → wontfix
status-firefox-esr45: --- → affected
Whiteboard: [adv-main47+]

Updated

10 months ago
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 11

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

Updated

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