Closed
Bug 563618
Opened 15 years ago
Closed 15 years ago
Crash due to writing past the end of buffer [@ nsEUCJPToUnicodeV2::Convert]
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla2.0
People
(Reporter: hsivonen, Assigned: smontagu)
References
()
Details
(Whiteboard: [sg:critical?][critsmash:resolved] [qa-examined-192] [qa-examined-191] [qa-needs-STR])
Attachments
(4 files)
(deleted),
text/html; charset=euc-jp
|
Details | |
(deleted),
patch
|
emk
:
review+
christian
:
approval1.9.2.14+
christian
:
approval1.9.1.17+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•15 years ago
|
||
Comment 2•15 years ago
|
||
This was a one-time only crash for me, I couldn't reproduce it.
Iirc, I changed the charset a lot with the testcase.
Assignee | ||
Comment 3•15 years ago
|
||
So the input when it crashed was probably not actual EUC-JP?
Comment 4•15 years ago
|
||
Probably not, no. This was a crash occuring during fuzzing, which I couldn't reproduce.
Assignee | ||
Comment 5•15 years ago
|
||
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
Updated•15 years ago
|
Whiteboard: [sg:critical?]
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #443713 -
Flags: review?
Assignee | ||
Comment 7•15 years ago
|
||
Assignee | ||
Comment 8•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #443713 -
Flags: review? → review?(VYV03354)
Updated•15 years ago
|
Attachment #443713 -
Flags: review?(VYV03354) → review+
Assignee | ||
Comment 9•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•14 years ago
|
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:resolved]
Updated•14 years ago
|
Comment 10•14 years ago
|
||
We'd like to get this fix in for 1.9.2.14 and 1.9.1.17
blocking1.9.1: ? → .17+
blocking1.9.2: ? → .14+
Assignee | ||
Updated•14 years ago
|
Attachment #443713 -
Flags: approval1.9.2.14?
Attachment #443713 -
Flags: approval1.9.1.17?
Comment 11•14 years ago
|
||
Comment on attachment 443713 [details] [diff] [review]
Patch
a=LegNeato for 1.9.2.14 and 1.9.1.17
Attachment #443713 -
Flags: approval1.9.2.14?
Attachment #443713 -
Flags: approval1.9.2.14+
Attachment #443713 -
Flags: approval1.9.1.17?
Attachment #443713 -
Flags: approval1.9.1.17+
Assignee | ||
Comment 12•14 years ago
|
||
Assignee | ||
Comment 13•14 years ago
|
||
Comment 14•14 years ago
|
||
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.
Keywords: verified1.9.1,
verified1.9.2
Assignee | ||
Comment 15•14 years ago
|
||
The crashtests don't currently crash or assert on branch, so I think it's OK in this case.
Assignee | ||
Comment 16•14 years ago
|
||
(even without the patch, I mean)
Comment 17•14 years ago
|
||
Is there any way to verify the bug fix for branches?
Keywords: verified1.9.1,
verified1.9.2
Updated•14 years ago
|
Whiteboard: [sg:critical?][critsmash:resolved] → [sg:critical?][critsmash:resolved] [qa-examined-192] [qa-examined-191] [qa-needs-STR]
Comment 18•14 years ago
|
||
Updated•14 years ago
|
Group: core-security
Updated•14 years ago
|
Target Milestone: --- → mozilla2.0
You need to log in
before you can comment on or make changes to this bug.
Description
•