Closed Bug 563618 Opened 10 years ago Closed 10 years ago

Crash due to writing past the end of buffer [@ nsEUCJPToUnicodeV2::Convert]

Categories

(Core :: Internationalization, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking1.9.2 --- .14+
status1.9.2 --- .14-fixed
blocking1.9.1 --- .17+
status1.9.1 --- .17-fixed

People

(Reporter: hsivonen, Assigned: smontagu)

References

()

Details

(Whiteboard: [sg:critical?][critsmash:resolved] [qa-examined-192] [qa-examined-191] [qa-needs-STR])

Attachments

(4 files)

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.
This was a one-time only crash for me, I couldn't reproduce it.
Iirc, I changed the charset a lot with the testcase.
So the input when it crashed was probably not actual EUC-JP?
Probably not, no. This was a crash occuring during fuzzing, which I couldn't reproduce.
Attached file 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
Whiteboard: [sg:critical?]
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]
Attached patch PatchSplinter Review
Attachment #443713 - Flags: review?
Attached patch TestsSplinter Review
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.
Attachment #443713 - Flags: review? → review?(VYV03354)
Attachment #443713 - Flags: review?(VYV03354) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:resolved]
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
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+
Attachment #443713 - Flags: approval1.9.2.14?
Attachment #443713 - Flags: approval1.9.1.17?
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+
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.
The crashtests don't currently crash or assert on branch, so I think it's OK in this case.
(even without the patch, I mean)
Is there any way to verify the bug fix for branches?
Whiteboard: [sg:critical?][critsmash:resolved] → [sg:critical?][critsmash:resolved] [qa-examined-192] [qa-examined-191] [qa-needs-STR]
Group: core-security
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.