Closed
Bug 733282
Opened 13 years ago
Closed 13 years ago
Crash in nsHtml5TreeBuilder | ASSERTION: The Unicode decoder wrote too much data.: 'end <= NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE' | ASSERTION: The decoder signaled an error other than NS_ERROR_ILLEGAL_INPUT.: 'convResult == NS_ERROR_ILLEGAL_INPUT'
Categories
(Core Graveyard :: History: Global, defect)
Tracking
(firefox11 wontfix, firefox12+ verified, firefox13+ verified, firefox14+ verified, firefox-esr1012+ verified)
People
(Reporter: bc, Unassigned)
References
()
Details
(4 keywords, Whiteboard: [sg:critical][qa+:ashughes])
Crash Data
Attachments
(3 files)
|
19.63 KB,
application/x-bzip2
|
Details | |
|
1.75 KB,
patch
|
smontagu
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
|
10.52 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
1. http://www.kogakkan-u.ac.jp/users/sirayama/index.html
perhaps reload
2. Crash Nightly/13. I can't reproduce with Aurora/12 or Beta/11.
###!!! ASSERTION: The Unicode decoder wrote too much data.: 'end <= NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE', file /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5StreamParser.cpp, line 829
###!!! ASSERTION: The decoder signaled an error other than NS_ERROR_ILLEGAL_INPUT.: 'convResult == NS_ERROR_ILLEGAL_INPUT', file /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5StreamParser.cpp, line 836
Mac OS X 10.5
bp-d4c07014-dc00-4446-9f2d-f27532120306
Firefox 13.0a1 Crash Report [@ small_free_list_remove_ptr | small_malloc_from_free_list | szone_malloc ]
bp-1cdb6ee0-3e92-4686-8a82-bb2d72120306
Firefox 13.0a1 Crash Report [@ small_free_list_remove_ptr | szone_free | free | nsHtml5DataAvailable::~nsHtml5DataAvailable ]
Windows XP
bp-fafa95bc-780e-4162-a0e0-7d8cd2120306
Firefox 13.0a1 Crash Report [@ nsHtml5TreeBuilder::flushCharacters() ]
Windows 7
bp-f10abc9a-e10d-4d73-aaa5-99d232120306
Firefox 13.0a1 Crash Report [@ RtlEnterCriticalSection ]
On Windows 7 with a debug build I saw
Assertion failure: local < script->nfixed, at c:/work/mozilla/builds/nightly/moz
illa/js/src/jsanalyze.cpp:186
And in Crash automation on Mac OS X 10.6 I saw
Assertion failure: (ptrBits & 0x7) == 0
Operating system: Mac OS X
10.6.8 10K549
CPU: amd64
family 6 model 23 stepping 10
2 CPUs
Crash reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0
Thread 0 (crashed)
0 libmozglue.dylib!MOZ_Crash [Assertions.cpp : 76 + 0x5]
rbx = 0x00000001002078d0 r12 = 0x0000000103082502
r13 = 0x0000000100107bb0 r14 = 0x0000000000000010
r15 = 0x0000000100107bb0 rip = 0x0000000100010e17
rsp = 0x00007fff5fbf8f80 rbp = 0x00007fff5fbf8f80
Found by: given as instruction pointer in context
1 libmozglue.dylib!MOZ_Assert [Assertions.cpp : 88 + 0x4]
rbx = 0x00000001002078d0 r12 = 0x0000000103082502
r13 = 0x0000000100107bb0 r14 = 0x0000000000000010
r15 = 0x0000000100107bb0 rip = 0x0000000100010e79
rsp = 0x00007fff5fbf8f90 rbp = 0x00007fff5fbf8fb0
Found by: call frame info
2 XUL!JSVAL_TO_OBJECT_IMPL [jsval.h : 766 + 0x23]
rbx = 0x00000001002078d0 r12 = 0x0000000103082502
r13 = 0x0000000100107bb0 r14 = 0x0000000000000010
r15 = 0x0000000100107bb0 rip = 0x0000000103654032
rsp = 0x00007fff5fbf8fc0 rbp = 0x00007fff5fbf8fe0
Found by: call frame info
3 XUL!JS::Value::toObject [jsapi.h : 544 + 0xb]
rbx = 0x00000001002078d0 r12 = 0x0000000103082502
r13 = 0x0000000100107bb0 r14 = 0x0000000000000010
r15 = 0x0000000100107bb0 rip = 0x0000000103678308
rsp = 0x00007fff5fbf8ff0 rbp = 0x00007fff5fbf9000
Found by: call frame info
4 XUL!js::GCMarker::processMarkStackTop [jsgcmark.cpp : 1090 + 0x8]
rbx = 0x00000001002078d0 r12 = 0x0000000103082502
r13 = 0x0000000100107bb0 r14 = 0x0000000000000010
r15 = 0x0000000100107bb0 rip = 0x0000000103701752
rsp = 0x00007fff5fbf9010 rbp = 0x00007fff5fbf90b0
Found by: call frame info
5 XUL!js::GCMarker::drainMarkStack [jsgcmark.cpp : 1172 + 0xc]
rbx = 0x00000001002078d0 r12 = 0x0000000103082502
r13 = 0x0000000100107bb0 r14 = 0x0000000000000010
r15 = 0x0000000100107bb0 rip = 0x00000001036fe751
rsp = 0x00007fff5fbf90c0 rbp = 0x00007fff5fbf9100
Found by: call frame info
6 XUL!MarkAndSweep [jsgc.cpp : 3293 + 0x13]
rbx = 0x00000001002078d0 r12 = 0x0000000103082502
r13 = 0x0000000100107bb0 r14 = 0x0000000000000010
r15 = 0x0000000100107bb0 rip = 0x00000001036e2efb
rsp = 0x00007fff5fbf9110 rbp = 0x00007fff5fbf9190
Found by: call frame info
7 XUL!GCCycle [jsgc.cpp : 3644 + 0xb]
rbx = 0x00000001002078d0 r12 = 0x0000000103082502
r13 = 0x0000000100107bb0 r14 = 0x0000000000000010
r15 = 0x0000000100107bb0 rip = 0x00000001036e30e6
rsp = 0x00007fff5fbf91a0 rbp = 0x00007fff5fbf9210
Found by: call frame info
s-s due to all of the various crashes related to memory and gc.
| Reporter | ||
Comment 1•13 years ago
|
||
1. install https://www.squarefree.com/extensions/quitter.xpi
2. tar jxvf sirayama.tar.bz2
3. firefox ./sirayama/index.html
Another nice assertion:
###!!! ASSERTION: Pointer overrun even before check: 'aDest <= aDestEnd', file /work/mozilla/builds/nightly/mozilla/intl/uconv/ucvja/../util/nsUCSupport.h, line 51
Comment 2•13 years ago
|
||
Henri, can you look into this? I'm guessing sg:critical here as this is a buffer overrun, which could lead to controllable crashes. If you disagree with that rating please let me know.
Assignee: nobody → hsivonen
Whiteboard: [sg:critical]
Updated•13 years ago
|
status-firefox-esr10:
--- → affected
status-firefox10:
--- → wontfix
status-firefox11:
--- → wontfix
status-firefox12:
--- → affected
status-firefox13:
--- → affected
tracking-firefox12:
--- → +
tracking-firefox13:
--- → +
Comment 3•13 years ago
|
||
(In reply to Bob Clary [:bc:] from comment #0)
> 1. http://www.kogakkan-u.ac.jp/users/sirayama/index.html
> perhaps reload
So this is Shift_JIS content mislabeled as iso-2022-jp. There's a bug in the iso-2022-jp decoder that's tickled by Shift_JIS input.
> ###!!! ASSERTION: The Unicode decoder wrote too much data.: 'end <=
> NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE', file
> /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5StreamParser.cpp,
> line 829
This is a bad bug in nsISO2022JPToUnicodeV2::Convert. Investigating.
> ###!!! ASSERTION: The decoder signaled an error other than
> NS_ERROR_ILLEGAL_INPUT.: 'convResult == NS_ERROR_ILLEGAL_INPUT', file
> /work/mozilla/builds/nightly/mozilla/parser/html/nsHtml5StreamParser.cpp,
> line 836
At least this one is trivial: nsISO2022JPToUnicodeV2::Convert can return NS_ERROR_UNEXPECTED even though it shouldn't do that.
Status: NEW → ASSIGNED
Comment 4•13 years ago
|
||
I changed from == to >= to mitigate the damage in case a buffer overrun happens in the future by other means. When the operation is correct, == can be true but > should never.
Attachment #604370 -
Flags: review?
Updated•13 years ago
|
Attachment #604370 -
Flags: review? → review?(smontagu)
Comment 5•13 years ago
|
||
Attachment #604371 -
Flags: review?(smontagu)
Comment 6•13 years ago
|
||
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #2)
> I'm guessing sg:critical here as this is a
> buffer overrun, which could lead to controllable crashes. If you disagree
> with that rating please let me know.
This bug indeed would allow data of the attackers choice to be written past the end of the buffer with the limitations that such a run of data would be prefixed with the REPLACEMENT CHARACTER and the data would need to be representable as iso-2022-jp.
(In reply to Henri Sivonen (:hsivonen) from comment #3)
> There's a bug in the iso-2022-jp decoder
Well, hunting that "bug" was the wrong approach. The problem was that the HTML5 parser assumed that the Unicode decoders work in a certain way (signal that the output is full when it is full), but they aren't guaranteed to work that way. Instead, a decoder can check for illegal input before checking for the space in the output buffer and can signal illegal input when the output buffer is full also.
So this is yet another case of the decoder API contract being insufficiently documented. :-(
I split the fix into two patches: The first one can be landed as a security fix and the second one as a follow-up after the first one has been delivered to users so that it's OK to reveal more details.
| Reporter | ||
Comment 7•13 years ago
|
||
Henri, could this be a root cause for bug 608994 ?
Comment 8•13 years ago
|
||
(In reply to Bob Clary [:bc:] from comment #7)
> Henri, could this be a root cause for bug 608994 ?
I'm not authorized to see that bug.
Comment 9•13 years ago
|
||
(In reply to Bob Clary [:bc:] from comment #7)
> Henri, could this be a root cause for bug 608994 ?
Looks unrelated. Different converter back end. Different direction of conversion. Different caller.
Comment 10•13 years ago
|
||
Comment on attachment 604370 [details] [diff] [review]
Fix for the bug
Review of attachment 604370 [details] [diff] [review]:
-----------------------------------------------------------------
::: parser/html/nsHtml5StreamParser.cpp
@@ +855,5 @@
> + nsHtml5OwningUTF16Buffer::FalliblyCreate(
> + NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE);
> + if (!newBuf) {
> + return NS_ERROR_OUT_OF_MEMORY;
> + }
Can you not share this code instead of adding another copy of it?
Comment 11•13 years ago
|
||
Comment on attachment 604371 [details] [diff] [review]
Documentation, test and converter result code correctness
Review of attachment 604371 [details] [diff] [review]:
-----------------------------------------------------------------
::: parser/htmlparser/tests/crashtests/733282-1.html
@@ +1,1 @@
> +<meta charset=iso-2022-jp>
The raw bytes make this rather obfuscated. Perhaps you can make it clearer by using <iframe src="data:text/html;charset=iso-2022-jp,%80%80%80%80.... or something similar?
Attachment #604371 -
Flags: review?(smontagu) → review+
Comment 12•13 years ago
|
||
(In reply to Simon Montagu from comment #10)
> Comment on attachment 604370 [details] [diff] [review]
> Fix for the bug
>
> Review of attachment 604370 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: parser/html/nsHtml5StreamParser.cpp
> @@ +855,5 @@
> > + nsHtml5OwningUTF16Buffer::FalliblyCreate(
> > + NS_HTML5_STREAM_PARSER_READ_BUFFER_SIZE);
> > + if (!newBuf) {
> > + return NS_ERROR_OUT_OF_MEMORY;
> > + }
>
> Can you not share this code instead of adding another copy of it?
The repeated bit needs to be able to return early and needs to set a local variable in one flavor of the repeated bit. Having a function whose return value needs to be checked in order to return early didn't seem much of a win.
Should I share the code and have a return value check and early return outside the shared bit?
Comment 13•13 years ago
|
||
Comment on attachment 604370 [details] [diff] [review]
Fix for the bug
Review of attachment 604370 [details] [diff] [review]:
-----------------------------------------------------------------
> The repeated bit needs to be able to return early and needs to set a local
> variable in one flavor of the repeated bit. Having a function whose return
> value needs to be checked in order to return early didn't seem much of a win.
A matter of taste, I suppose.
Attachment #604370 -
Flags: review?(smontagu) → review+
Comment 14•13 years ago
|
||
Thanks. I landed the fix without landing the test & documentation patch yet:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3760562b6c
status-firefox-esr10:
affected → ---
status-firefox10:
wontfix → ---
status-firefox11:
wontfix → ---
status-firefox12:
affected → ---
status-firefox13:
affected → ---
tracking-firefox12:
+ → ---
tracking-firefox13:
+ → ---
Target Milestone: --- → mozilla13
Version: Trunk → Other Branch
Comment 15•13 years ago
|
||
(Restoring flags reset by the last comment; also trunk is now mozilla14)
status-firefox-esr10:
--- → affected
status-firefox11:
--- → wontfix
status-firefox12:
--- → affected
status-firefox13:
--- → affected
tracking-firefox-esr10:
--- → ?
tracking-firefox12:
--- → ?
tracking-firefox13:
--- → ?
Target Milestone: mozilla13 → mozilla14
Version: Other Branch → Trunk
Comment 16•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
status-firefox14:
--- → fixed
tracking-firefox14:
--- → +
| Reporter | ||
Comment 17•13 years ago
|
||
Another example: http://www.tbs.co.jp/anime/kmb/radio/radio.html
Comment 18•13 years ago
|
||
Comment on attachment 604370 [details] [diff] [review]
Fix for the bug
[Approval Request Comment]
Regression caused by (bug #): Not really a regression unless you count the HTML5 landing as the cause.
User impact if declined: The user's system might get attacked by an attacker writing data past the end of a buffer. Non-malicious mislabeled pages can crash the browser.
Testing completed (on m-c, etc.): Has baked on m-c for a few days.
Risk to taking this patch (and alternatives if risky): Very low risk. Adds a buffer allocation in one place.
String changes made by this patch: None.
Attachment #604370 -
Flags: approval-mozilla-esr10?
Attachment #604370 -
Flags: approval-mozilla-beta?
Attachment #604370 -
Flags: approval-mozilla-aurora?
Comment 19•13 years ago
|
||
Comment on attachment 604370 [details] [diff] [review]
Fix for the bug
[Triage Comment]
Approving to land given low risk and being early in the beta cycle. For ESR see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Attachment #604370 -
Flags: approval-mozilla-esr10?
Attachment #604370 -
Flags: approval-mozilla-esr10+
Attachment #604370 -
Flags: approval-mozilla-beta?
Attachment #604370 -
Flags: approval-mozilla-beta+
Attachment #604370 -
Flags: approval-mozilla-aurora?
Attachment #604370 -
Flags: approval-mozilla-aurora+
Comment 20•13 years ago
|
||
| Reporter | ||
Comment 21•13 years ago
|
||
tested using Nightly/14, Aurora/13, Beta/12 on Linux, OS X 10.6, Windows XP/7
http://www.tbs.co.jp/anime/kmb/radio/radio.html
http://www.kogakkan-u.ac.jp/users/sirayama/index.html
-> No Crashes
Comment 22•13 years ago
|
||
(In reply to Bob Clary [:bc:] from comment #21)
> tested using Nightly/14, Aurora/13, Beta/12 on Linux, OS X 10.6, Windows XP/7
> http://www.tbs.co.jp/anime/kmb/radio/radio.html
> http://www.kogakkan-u.ac.jp/users/sirayama/index.html
> -> No Crashes
So we can call this verified fixed on all three.
Assignee: hsivonen → nobody
Status: RESOLVED → VERIFIED
Component: HTML: Parser → History: Global
QA Contact: parser → history.global
Updated•13 years ago
|
Group: core-security
Comment 23•13 years ago
|
||
Verified fixed in Firefox 10.0.4esr and 10.0.5esrpre.
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+:ashughes]
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•