Closed
Bug 733282
Opened 12 years ago
Closed 12 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•12 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•12 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•12 years ago
|
status-firefox-esr10:
--- → affected
status-firefox10:
--- → wontfix
status-firefox11:
--- → wontfix
status-firefox12:
--- → affected
status-firefox13:
--- → affected
tracking-firefox12:
--- → +
tracking-firefox13:
--- → +
Comment 3•12 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•12 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•12 years ago
|
Attachment #604370 -
Flags: review? → review?(smontagu)
Comment 5•12 years ago
|
||
Attachment #604371 -
Flags: review?(smontagu)
Comment 6•12 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•12 years ago
|
||
Henri, could this be a root cause for bug 608994 ?
Comment 8•12 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•12 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•12 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•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2e3760562b6c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox14:
--- → fixed
tracking-firefox14:
--- → +
Reporter | ||
Comment 17•12 years ago
|
||
Another example: http://www.tbs.co.jp/anime/kmb/radio/radio.html
Comment 18•12 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•12 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•12 years ago
|
||
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
Reporter | ||
Comment 21•12 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•12 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•12 years ago
|
Group: core-security
Comment 23•12 years ago
|
||
Verified fixed in Firefox 10.0.4esr and 10.0.5esrpre.
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+:ashughes]
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•