ASan: heap-buffer-overflow in sec_port_ucs2_utf8_conversion_function()

RESOLVED FIXED in Firefox 47

Status

NSS
Libraries
--
critical
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: tsmith, Assigned: jld)

Tracking

(Blocks: 1 bug, 4 keywords)

trunk
3.23
x86_64
Unspecified
crash, csectype-bounds, sec-moderate, testcase
Dependency tree / graph

Firefox Tracking Flags

(firefox41 wontfix, firefox42+ wontfix, firefox43+ wontfix, firefox44+ wontfix, firefox45 wontfix, firefox46 wontfix, firefox47 fixed, firefox-esr38- wontfix, firefox-esr4550+ fixed)

Details

(Whiteboard: [adv-main47+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8663170 [details]
call_stack.txt

I found this with a new fuzzing harness I am working on. I will share it when it is complete.
(Reporter)

Comment 1

2 years ago
Created attachment 8663171 [details]
test_case.bin
(Reporter)

Updated

2 years ago
Keywords: sec-high
Does this still repro with the patches for the other 3 bugs?
Flags: needinfo?(twsmith)
I think this bug is unrelated to the others, but more benign (probably only out-of-bounds read).

Here's the code in question:

    for( i = 0; i < inBufLen; i += 2 ) {
      if( (inBuf[i+H_0] == 0x00) && ((inBuf[i+H_0] & 0x80) == 0x00) ) len += 1;
      else if( inBuf[i+H_0] < 0x08 ) len += 2;
      else if( ((inBuf[i+0+H_0] & 0xDC) == 0xD8) ) {
325:    if( ((inBuf[i+2+H_0] & 0xDC) == 0xDC) && ((inBufLen - i) > 2) ) {
          i += 2;
          len += 4;
        } else {
          return PR_FALSE;
        }
      }
      else len += 3;
    }

I've noted line 325, where the out-of-bounds read occurs. If inBufLen is 2, inBuf[i+2+H_0] reads one byte off the end of the array (H_0 is 0, I think).

Now that I look at this again, the fix seems obvious - the ((inBufLen - i) > 2) check should occur before the array access. There may be something more going on here, though.
(Reporter)

Comment 4

2 years ago
(In reply to Daniel Veditz [:dveditz] from comment #2)
> Does this still repro with the patches for the other 3 bugs?

Yes this is still reproducible with the latest patches applied.
Flags: needinfo?(twsmith)
status-firefox41: --- → wontfix
status-firefox42: --- → affected
status-firefox44: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox42: --- → +
tracking-firefox43: --- → +
tracking-firefox44: --- → +
tracking-firefox-esr38: --- → ?
David, we are going to start the build of 42 beta 8 today (understand: this is the end of the cycle).
Are you planning to fix this issue for 42? Thanks
Flags: needinfo?(dkeeler)

Comment 6

2 years ago
This wasn't part of the NSS 3.21 set of fixes, as it's much lower severity. So don't expect this to be uplifted in time for FF 42.
Flags: needinfo?(dkeeler)

Comment 7

2 years ago
s/3.21/3.20 || 3.19.4/
Thanks Ryan, updating the flag accordingly.
status-firefox42: affected → wontfix
Assuming this is also a wontfix for 43. Is there any plan for the next nss update?
Sounds like there isn't. Wontfix for 43.
status-firefox43: affected → wontfix
Ryan, FF44 was upgraded to NSS 3.21 in bug 1211568. Do you believe this is fixed in that version of NSS? If so, could you please update status-firefox44 to "fixed"?

If not, do we have a patch ready for uplift to Beta44 and other affected branches? Thanks!
Flags: needinfo?(ryan.sleevi)

Comment 12

a year ago
No patch has been committed to NSS to resolve this.
Flags: needinfo?(ryan.sleevi)
ff44 is fixed based on comment 12.
status-firefox44: affected → fixed

Comment 14

a year ago
Ritu: Please read my reply. Comment #13 is the literal exact opposite of what comment #12 says :)
Flags: needinfo?(rkothari)

Updated

a year ago
status-firefox44: fixed → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
(In reply to Ryan Sleevi from comment #14)
> Ritu: Please read my reply. Comment #13 is the literal exact opposite of
> what comment #12 says :)

My bad, I assumed that this patch was included in NSS 3.21 and in bug 1211568 we updated FF44 to use NSS 3.21 which is why I marked it as fixed. 

Do you believe this will get fixed in 44? If so, we will need to update the NSS version again.
Flags: needinfo?(rkothari) → needinfo?(ryan.sleevi)

Comment 16

a year ago
I think it's unlikely to get fixed in NSS short of 3.22, and I suspect that it's unlikely that NSS 3.22 would get uplifted into FF44. You'd need to talk to whoever handles the NSS<->FF merges (kaie or dkeeler, I think?) to figure out schedule.
Flags: needinfo?(ryan.sleevi)
Too late to take a new NSS rev up for Fx44, this is now a wontfix for that version.
status-firefox44: affected → wontfix
(Assignee)

Updated

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

Comment 18

a year ago
And see also bug 1241037 (which might not be security-sensitive, so I wasn't sure if it ought to link to this bug).
(Assignee)

Updated

a year ago
Assignee: nobody → jld
(Assignee)

Comment 19

a year ago
Incidentally, the test case from this bug (attachment 8663171 [details]) also demonstrates bug 1241037: U+F930 is not a surrogate.
(Assignee)

Comment 20

a year ago
Created attachment 8715089 [details] [diff] [review]
bug1206283-utf16-read-hg0.diff

I added some test cases which, combined with this bug, set off valgrind by reading past the end of a heap buffer.  I tried to match the style of the surrounding code for those (but this will probably all be redone by clang-format anyway).

The patch makes it pretty obvious what's going on here, even without the tests; there's not much to be done about that.

I'm not convinced that this should be sec-high; it's an out-of-bounds read, but the bits from out of bounds shouldn't affect control flow or otherwise be disclosed to anything, so it wouldn't be an info leak.  (Technically the compiler could do anything, but it would have to realize this is a bug first, and it probably won't.)  I think the worst that can happen is a crash, if an attacker can trigger this across a page boundary.
Attachment #8715089 - Flags: review?(ttaubert)
Attachment #8715089 - Flags: feedback?(dkeeler)
(Assignee)

Updated

a year ago
See Also: → bug 1241037
Comment on attachment 8715089 [details] [diff] [review]
bug1206283-utf16-read-hg0.diff

Review of attachment 8715089 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: lib/util/utf8.c
@@ +321,5 @@
>      for( i = 0; i < inBufLen; i += 2 ) {
>        if( (inBuf[i+H_0] == 0x00) && ((inBuf[i+H_1] & 0x80) == 0x00) ) len += 1;
>        else if( inBuf[i+H_0] < 0x08 ) len += 2;
>        else if( ((inBuf[i+0+H_0] & 0xDC) == 0xD8) ) {
> +        if( ((inBufLen - i) > 2) && ((inBuf[i+2+H_0] & 0xDC) == 0xDC) ) {

I wonder if this should be 'inBufLen - i - H_0' for completeness?

@@ +358,5 @@
>          len += 2;
>        } else if( (inBuf[i+H_0] & 0xDC) == 0xD8 ) {
>          int abcde, BCDE;
>  
> +        PORT_Assert(((inBufLen - i) > 2) && ((inBuf[i+2+H_0] & 0xDC) == 0xDC) );

Same idea.

@@ +1152,5 @@
>    "\xED\xBF\xBF",
>    "\xED\xA0\x80\xE0\xBF\xBF",
>  };
>  
> +/* illegal UTF-16 sequences */

Maybe explain why these are bad?
Attachment #8715089 - Flags: feedback?(dkeeler) → feedback+
Comment on attachment 8715089 [details] [diff] [review]
bug1206283-utf16-read-hg0.diff

Review of attachment 8715089 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with keeler's comments addressed.

::: lib/util/utf8.c
@@ +1487,5 @@
> +          destlen);
> +      rv = PR_FALSE;
> +    }
> +    free(buf);
> +    continue;

Nit: remove "continue"
Attachment #8715089 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 23

a year ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #21) 
> ::: lib/util/utf8.c
> @@ +321,5 @@
> >      for( i = 0; i < inBufLen; i += 2 ) {
> >        if( (inBuf[i+H_0] == 0x00) && ((inBuf[i+H_1] & 0x80) == 0x00) ) len += 1;
> >        else if( inBuf[i+H_0] < 0x08 ) len += 2;
> >        else if( ((inBuf[i+0+H_0] & 0xDC) == 0xD8) ) {
> > +        if( ((inBufLen - i) > 2) && ((inBuf[i+2+H_0] & 0xDC) == 0xDC) ) {
> 
> I wonder if this should be 'inBufLen - i - H_0' for completeness?

We're already doing inBuf[i+H_0] and inBuf[i+H_1] on the strength of (i < inBufLen), and this isn't really different, I don't think?  And that's sound because we've already ensured that ((inBufLen % 2) == 0) and returned PR_FALSE if not.  I could go change all the conditions to be <= but I don't know how useful that would be.

> @@ +1152,5 @@
> >    "\xED\xBF\xBF",
> >    "\xED\xA0\x80\xE0\xBF\xBF",
> >  };
> >  
> > +/* illegal UTF-16 sequences */
> 
> Maybe explain why these are bad?

Will do.
(Assignee)

Comment 24

a year ago
(In reply to Tim Taubert [:ttaubert] from comment #22)
> ::: lib/util/utf8.c
> @@ +1487,5 @@
> > +          destlen);
> > +      rv = PR_FALSE;
> > +    }
> > +    free(buf);
> > +    continue;
> 
> Nit: remove "continue"

I was following the example of the existing code, but I'm just as happy not to.  Removed.
(Assignee)

Comment 25

a year ago
Created attachment 8716133 [details] [diff] [review]
bug1206283-utf16-read-hg1.diff

Updating; carrying over r+.
Attachment #8715089 - Attachment is obsolete: true
Attachment #8716133 - Flags: review+
(Assignee)

Comment 26

a year ago
See comment #20; the bits read from out of bounds aren't disclosed as far as I can tell, which would make this not sec-high.
Keywords: sec-high → sec-moderate
(Assignee)

Updated

a year ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/nss/rev/1ba7cd83c672
Status: NEW → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Group: crypto-core-security → core-security-release
Target Milestone: --- → 3.23
[Tracking Requested - why for this release]:

This isn't relevant for esr38 any more as we are about to ship esr 45.2 and stop supporting 38. Do we need to track this bug for esr45?
status-firefox-esr38: affected → wontfix
tracking-firefox-esr38: ? → -
tracking-firefox-esr45: --- → ?
Flags: needinfo?(ttaubert)
45.1 still has NSS 3.21, and this fix landed with 3.23. We could uplift this if we think it's worth it. Given that it's sec-moderate it could ride along with other sec-high fixes in the future?
Flags: needinfo?(ttaubert)
47.0 will have 3.23. It seems late to try for 45.2.0esr since we're getting close to the release date.  

We could plan to update esr to 3.23 in the next esr after this one, so, 45.3.0esr if you think that is a good idea.
tracking-firefox-esr45: ? → 48+
This should have been marked as fixed in 47 then. It wasn't on any list so wasn't going to get an advisory. I use the tags to know what to write advisories on...
status-firefox47: --- → fixed
Whiteboard: [adv-main47+]

Updated

11 months ago
status-firefox45: affected → wontfix
status-firefox46: affected → wontfix
status-firefox-esr45: --- → affected
status-firefox-esr45: affected → wontfix
tracking-firefox-esr45: 48+ → ---
Tracking upstream NSS 3.21.3 security updates for the ESR-45 "50+" release.
status-firefox-esr45: wontfix → affected
tracking-firefox-esr45: --- → 50+

Comment 33

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

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.