Last Comment Bug 563618 - Crash due to writing past the end of buffer [@ nsEUCJPToUnicodeV2::Convert]
: Crash due to writing past the end of buffer [@ nsEUCJPToUnicodeV2::Convert]
Status: RESOLVED FIXED
[sg:critical?][critsmash:resolved] [q...
:
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla2.0
Assigned To: Simon Montagu :smontagu
:
Mentors:
http://crash-stats.mozilla.com/report...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-04 03:42 PDT by Henri Sivonen (:hsivonen)
Modified: 2011-05-18 18:19 PDT (History)
10 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.14+
.14-fixed
.17+
.17-fixed


Attachments
Patch (1.86 KB, patch)
2010-05-05 13:39 PDT, Simon Montagu :smontagu
VYV03354: review+
christian: approval1.9.2.14+
christian: approval1.9.1.17+
Details | Diff | Review
Tests (5.57 KB, patch)
2010-05-05 13:39 PDT, Simon Montagu :smontagu
no flags Details | Diff | Review
1.8 version (1.31 KB, patch)
2011-01-28 07:41 PST, Martin Stránský
no flags Details | Diff | Review

Description Henri Sivonen (:hsivonen) 2010-05-04 03:42:59 PDT
No steps to reproduce; filed based on a crash stack.

nsJapaneseToUnicode.cpp has these lines:
367               *dest++ = 0xFFFD;             
368               // if 0x8e is not followed by a valid JIS X 0201 byte
369               // but by a valid US-ASCII, save it instead of eating it up.
370               if ( (PRUint8)*src < (PRUint8)0x7f )
371                  *dest++ = (PRUnichar) *src;

The destination pointer is incremented on line 367 and then written to again on line 371 without first checking if the increment reached the end of the buffer. When the increment on line 367 reaches the end of the buffer, line 371 crashes.

Even though this was already discussed on #developers, I'm marking this is as security sensitive in case the write past the buffer could be used to deliberately write something to the heap.
Comment 2 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2010-05-04 03:53:58 PDT
This was a one-time only crash for me, I couldn't reproduce it.
Iirc, I changed the charset a lot with the testcase.
Comment 3 Simon Montagu :smontagu 2010-05-04 04:42:29 PDT
So the input when it crashed was probably not actual EUC-JP?
Comment 4 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2010-05-04 04:47:07 PDT
Probably not, no. This was a crash occuring during fuzzing, which I couldn't reproduce.
Comment 5 Simon Montagu :smontagu 2010-05-04 10:24:03 PDT
Created attachment 443380 [details]
testcase

This test case doesn't reliably crash for me, but it does assert:

###!!! ASSERTION: The Unicode decoder wrote too much data.: 'mLastBuffer->getEnd() <= NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE', file /Users/simon/mozwork/hgtree/mozilla/parser/html/nsHtml5StreamParser.cpp, line 493
Comment 6 Simon Montagu :smontagu 2010-05-05 13:39:15 PDT
Created attachment 443713 [details] [diff] [review]
Patch
Comment 7 Simon Montagu :smontagu 2010-05-05 13:39:43 PDT
Created attachment 443714 [details] [diff] [review]
Tests
Comment 8 Simon Montagu :smontagu 2010-05-05 13:42:54 PDT
Comment on attachment 443713 [details] [diff] [review]
Patch

The change

>-               if ( ! (*src & 0xc0)  )
>+              if ( (PRUint8)*src < (PRUint8)0x7f )

isn't related to the crash, but I think the original is wrong, maybe caused by copy-pasting code in the patch to bug 25037.
Comment 10 christian 2010-12-20 10:22:27 PST
We'd like to get this fix in for 1.9.2.14 and 1.9.1.17
Comment 11 christian 2010-12-20 10:42:13 PST
Comment on attachment 443713 [details] [diff] [review]
Patch

a=LegNeato for 1.9.2.14 and 1.9.1.17
Comment 14 Al Billings [:abillings] 2011-01-03 17:23:11 PST
The crashtest passes for 1.9.1 and 1.9.2 on all platforms.

Is there a reason that crashtests for a security bug were checked in prior to release, though? We normally hold those until after the release goes out.
Comment 15 Simon Montagu :smontagu 2011-01-04 01:14:08 PST
The crashtests don't currently crash or assert on branch, so I think it's OK in this case.
Comment 16 Simon Montagu :smontagu 2011-01-04 01:14:25 PST
(even without the patch, I mean)
Comment 17 Al Billings [:abillings] 2011-01-04 09:13:17 PST
Is there any way to verify the bug fix for branches?
Comment 18 Martin Stránský 2011-01-28 07:41:34 PST
Created attachment 507854 [details] [diff] [review]
1.8 version

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