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

RESOLVED FIXED in mozilla2.0

Status

()

Core
Internationalization
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: hsivonen, Assigned: smontagu)

Tracking

Trunk
mozilla2.0
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .14+, status1.9.2 .14-fixed, blocking1.9.1 .17+, status1.9.1 .17-fixed)

Details

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

Attachments

(4 attachments)

(Reporter)

Description

7 years ago
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

7 years ago
http://crash-stats.mozilla.com/report/index/ae6368f5-a6f8-4b7c-8f33-ef9102100429
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

7 years ago
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.
(Assignee)

Comment 5

7 years ago
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
Whiteboard: [sg:critical?]

Updated

7 years ago
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]
(Assignee)

Comment 6

7 years ago
Created attachment 443713 [details] [diff] [review]
Patch
Attachment #443713 - Flags: review?
(Assignee)

Comment 7

7 years ago
Created attachment 443714 [details] [diff] [review]
Tests
(Assignee)

Comment 8

7 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

7 years ago
Attachment #443713 - Flags: review? → review?(VYV03354)
Attachment #443713 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 9

7 years ago
http://hg.mozilla.org/mozilla-central/rev/0841ec7eec87
http://hg.mozilla.org/mozilla-central/rev/93874cf6d6ba
(Assignee)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

7 years ago
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:resolved]
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
status1.9.1: --- → wanted
status1.9.2: --- → wanted

Comment 10

6 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

6 years ago
Attachment #443713 - Flags: approval1.9.2.14?
Attachment #443713 - Flags: approval1.9.1.17?

Comment 11

6 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

6 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a5a94cd7b310
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a2ae8b0e65ee
status1.9.2: wanted → .14-fixed
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b576ae79792e
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ee5ea05fcd81
status1.9.1: wanted → .17-fixed
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

6 years ago
The crashtests don't currently crash or assert on branch, so I think it's OK in this case.
(Assignee)

Comment 16

6 years ago
(even without the patch, I mean)
Is there any way to verify the bug fix for branches?
Keywords: verified1.9.1, verified1.9.2
Whiteboard: [sg:critical?][critsmash:resolved] → [sg:critical?][critsmash:resolved] [qa-examined-192] [qa-examined-191] [qa-needs-STR]

Comment 18

6 years ago
Created attachment 507854 [details] [diff] [review]
1.8 version
Group: core-security
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.