1206283 - ASan: heap-buffer-overflow in sec_port_ucs2_utf8_conversion_function()

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: call_stack.txt

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

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.bin

Comment 2

[Daniel Veditz [:dveditz]]

Does this still repro with the patches for the other 3 bugs?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 4

[Tyson Smith [:tsmith]]

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

Comment 5

[Sylvestre Ledru [:Sylvestre]]

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

Comment 6

[Ryan Sleevi]

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.

Comment 7

[Ryan Sleevi]

s/3.21/3.20 || 3.19.4/

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Thanks Ryan, updating the flag accordingly.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Assuming this is also a wontfix for 43. Is there any plan for the next nss update?

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like there isn't. Wontfix for 43.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 12

[Ryan Sleevi]

No patch has been committed to NSS to resolve this.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

ff44 is fixed based on comment 12.

Comment 14

[Ryan Sleevi]

Ritu: Please read my reply. Comment #13 is the literal exact opposite of what comment #12 says :)

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 16

[Ryan Sleevi]

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.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late to take a new NSS rev up for Fx44, this is now a wontfix for that version.

Comment 18

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

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

Comment 19

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

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

Comment 20

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

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

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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?

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

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"

Comment 23

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

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

Comment 24

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

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

Comment 25

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

Attachment: bug1206283-utf16-read-hg1.diff

Updating; carrying over r+.

Comment 26

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

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.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/1ba7cd83c672

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

[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?

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 31

[Al Billings [:abillings - ex-MoCo]]

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

Comment 32

[Daniel Veditz [:dveditz]]

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

Comment 33

[Kai Engert [:KaiE:]]

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

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Fixed in bug 1310009