670556 - Wrong condition in else-if statement (mozilla-2.0/intl/uconv/util/uscan.c) let you access outside the bounds [0,29] of array tMap

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Minh Cuong Tran]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232052

Steps to reproduce:

Reading code.


Actual results:

I found this piece of of code at intl/uconv/util/uscan.c:624

=========
  /* Compute TIndex  */
  if(0xd4 == in[7])  
  {
    TIndex = 0;
  } 
  else if((in[7] < 0xa1) && (in[7] > 0xbe)) {/* illegal trailling consonant */
    return PR_FALSE;
  } 
  else {
    static const PRUint8 tMap[] = {
      /*        A1   A2   A3   A4   A5   A6   A7  */
      1,   2,   3,   4,   5,   6,   7,
        /*   A8   A9   AA   AB   AC   AD   AE   AF  */
        0xff,   8,   9,  10,  11,  12,  13,  14,
        /*   B0   B1   B2   B3   B4   B5   B6   B7  */
        15,  16,  17,0xff,  18,  19,  20,  21,
        /*   B8   B9   BA   BB   BC   BD   BE       */
        22,0xff,  23,  24,  25,  26,  27     
    };
    TIndex = tMap[in[7] - 0xa1];
    if(0xff == (0xff & TIndex))
      return PR_FALSE;
  }
=========

tMap is an array with size [0,29]. The else-if statement is
(in[7] < 0xa1) && (in[7] > 0xbe)
but it should be
(in[7] < 0xa1) || (in[7] > 0xbe)

Otherwise you will always jump in else branch and can access outside of [0,29] of array tMap with the statement:
    TIndex = tMap[in[7] - 0xa1];
since else-if will be always false.


Expected results:

Change code into
(in[7] < 0xa1) || (in[7] > 0xbe)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In uCnSAlways8BytesDecomposedHangul

Comment 2

[Minh Cuong Tran]

Same problem uCnSAlways8BytesDecomposedHangul for:
  if((in[3] < 0xa1) && (in[3] > 0xbe)) { /* illegal leading consonant */

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ms2ger, want to patch?

Comment 4

[Daniel Veditz [:dveditz]]

I assume these characters end up in the string where they could be inspected? Reading ~250 bytes of stack might get an attacker useful information, like a return address that could be used to defeat ASLR defenses. Probably not any sensitive memory from other pages since that would most likely be in heap. Guessing sg:low as a stepping-stone issue.

Comment 6

[Simon Montagu :smontagu]

Attachment: Patch

Comment 7

[Simon Montagu :smontagu]

Attachment: Tests

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 545619 [details] [diff] [review]
Patch

Sorry for forgetting the review request.

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 545621 [details] [diff] [review]
Tests

> + *  * octet6 == 0xa7
0xa4

Otherwise looks fine.

Comment 10

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9596d9d2d2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/71370660e59d

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f9596d9d2d2b
https://hg.mozilla.org/mozilla-central/rev/71370660e59d