Last Comment Bug 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
: Wrong condition in else-if statement (mozilla-2.0/intl/uconv/util/uscan.c) le...
Status: RESOLVED FIXED
[sg:low]
:
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All Other
: -- critical (vote)
: mozilla12
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-10 11:52 PDT by Minh Cuong Tran
Modified: 2012-02-01 14:00 PST (History)
5 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.00 KB, patch)
2011-07-13 01:21 PDT, Simon Montagu :smontagu
VYV03354: review+
Details | Diff | Splinter Review
Tests (413.05 KB, patch)
2011-07-13 01:22 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review

Description Minh Cuong Tran 2011-07-10 11:52:42 PDT
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 2011-07-10 11:59:50 PDT
In uCnSAlways8BytesDecomposedHangul
Comment 2 Minh Cuong Tran 2011-07-10 12:03:11 PDT
Same problem uCnSAlways8BytesDecomposedHangul for:
  if((in[3] < 0xa1) && (in[3] > 0xbe)) { /* illegal leading consonant */
Comment 3 Boris Zbarsky [:bz] 2011-07-11 05:22:55 PDT
Ms2ger, want to patch?
Comment 4 Daniel Veditz [:dveditz] 2011-07-11 10:33:47 PDT
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 2011-07-13 01:21:54 PDT
Created attachment 545619 [details] [diff] [review]
Patch
Comment 7 Simon Montagu :smontagu 2011-07-13 01:22:35 PDT
Created attachment 545621 [details] [diff] [review]
Tests
Comment 8 Masatoshi Kimura [:emk] 2012-01-22 00:31:09 PST
Comment on attachment 545619 [details] [diff] [review]
Patch

Sorry for forgetting the review request.
Comment 9 Masatoshi Kimura [:emk] 2012-01-22 00:31:27 PST
Comment on attachment 545621 [details] [diff] [review]
Tests

> + *  * octet6 == 0xa7
0xa4

Otherwise looks fine.

Note You need to log in before you can comment on or make changes to this bug.