733282 - 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'

Status: VERIFIED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Bob Clary [:bc] (inactive)]

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.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: sirayama.tar.bz2

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

[Johnny Stenback (:jst)]

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.

Comment 3

[Henri Sivonen (:hsivonen)]

(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.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Fix for the bug

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.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Documentation, test and converter result code correctness

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Johnny Stenback (:jst, [e-mail]) 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.

Comment 7

[Bob Clary [:bc] (inactive)]

Henri, could this be a root cause for bug 608994 ?

Comment 8

[Henri Sivonen (:hsivonen)]

(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

[Henri Sivonen (:hsivonen)]

(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

[Simon Montagu :smontagu]

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

[Simon Montagu :smontagu]

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?

Comment 12

[Henri Sivonen (:hsivonen)]

(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

[Simon Montagu :smontagu]

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.

Comment 14

[Henri Sivonen (:hsivonen)]

Thanks. I landed the fix without landing the test & documentation patch yet:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3760562b6c

Comment 15

[Ed Morley [:emorley]]

(Restoring flags reset by the last comment; also trunk is now mozilla14)

Comment 16

[Justin Wood (:Callek)]

https://hg.mozilla.org/mozilla-central/rev/2e3760562b6c

Comment 17

[Bob Clary [:bc] (inactive)]

Another example: http://www.tbs.co.jp/anime/kmb/radio/radio.html

Comment 18

[Henri Sivonen (:hsivonen)]

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.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 20

[Henri Sivonen (:hsivonen)]

Thanks for the approvals.
http://hg.mozilla.org/releases/mozilla-aurora/rev/772477595ea4
http://hg.mozilla.org/releases/mozilla-beta/rev/35f34594bd5e
http://hg.mozilla.org/releases/mozilla-esr10/rev/0325ae38d384

Comment 21

[Bob Clary [:bc] (inactive)]

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

[Al Billings [:abillings - ex-MoCo]]

(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.

Comment 23

[u279076]

Verified fixed in Firefox 10.0.4esr and 10.0.5esrpre.