Closed Bug 526500 (CVE-2009-1571) Opened 15 years ago Closed 15 years ago

Use-after-free error in HTML parser when handling out-of-memory conditions (SA37242)

Categories

(Core :: XPCOM, defect, P2)

Product:
Core
Component:
XPCOM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2
Tracking Flags:
Tracking Status
status1.9.2 --- beta4-fixed
blocking1.9.1 --- .8+
status1.9.1 --- .8-fixed

People

(Reporter: reed, Assigned: sicking)

Details

(4 keywords, Whiteboard: [sg:critical])

Attachments

(4 files, 1 obsolete file)

PoC
15 years ago
4.81 KB, application/zip
Details
Patch to fix
4.32 KB, patch
benjamin
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
1.9.1/1.9.0 branch patch
1.77 KB, patch
benjamin
: review+
dveditz
: approval1.9.1.8+
dveditz
: approval1.9.0.18+
Details | Diff | Splinter Review
1.8.0 version
1.19 KB, patch
Details | Diff | Splinter Review
From Secunia to security@: --------------------------- Secunia Research has discovered a vulnerability in Mozilla Firefox, which can be exploited by malicious people to compromise a user's system. The vulnerability is caused due to a use-after-free error when handling out-of-memory conditions. This can be exploited to trigger a memory corruption via a specially crafted web page. Successful exploitation may allow execution of arbitrary code. The vulnerability is confirmed in version 3.5.4. Other versions may also be affected. Vulnerability Details: ---------------------- Used memory is incorrectly freed when insufficient space is available to process HTML input. Memory occupied by in-use objects is freed and filled with text placed in the HTML body after all available virtual address space is exhausted. While crashes occur at random locations, most of the observed cases would result in the execution of arbitrary code when calling methods from freed objects. Closing comments: ----------------- We have assigned this vulnerability Secunia advisory SA37242 and CVE identifier CVE-2009-1571. Credits should go to: Alin Rad Pop, Secunia Research. ------------------------
PoC has been requested from Secunia. The vulnerability can also be reproduced by creating an HTML file containing JavaScript code which attempts to allocate ~2900 MBytes in the <head>, followed a 7 Mbyte string in the <body> section, between <script></script> tags.
blocking1.9.1: --- → ?
status1.9.1: --- → ?
Flags: blocking1.9.2?
blocking1.9.1: ? → needed
status1.9.1: ? → wanted
Whiteboard: [sg:critical]
I tried running the PoC from a local file on OSX (a debug build with several patches applied, none of which should affect this bug). I didn't crash at all, not even when I set MallocScribble=1 in my environment to ensure that any calls through deleted pointers would crash. My next step would be to try valgrind, though I wonder if anybody has a stack for the crashes that they saw...
Blocking 1.9.2 on at least figuring out exactly what's causing this and the severity of it.
Assignee: nobody → mrbkap
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
By the way, I think the reason I can't reproduce the crash is that I run out of memory at a different point than the reporter and that specific part of the code deals with OOM correctly.
(In reply to comment #2) > Created an attachment (id=410455) [details] > PoC I forgot to mention that Secunia said that the PoC was tested to crash 3.5.4 on Windows XP SP3.
will test this today !
I crash 3.5.5 on WinXP. The stack is in NSS code, which from the testcase makes no sense (so I'll try it again and see what I get) 0:000> !exploitable -v HostMachine\HostUser Executing Processor Architecture is x86 Debuggee is in User Mode Debuggee is a live user mode debugging session on the local machine Event Type: Exception Exception Faulting Address: 0x15ff0881 First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005) Exception Sub-Type: Read Access Violation Faulting Instruction:005f10b7 mov edx,dword ptr [ecx+0ch] Basic Block: 005f10b7 mov edx,dword ptr [ecx+0ch] Tainted Input Operands: ecx 005f10ba add edx,ebx Tainted Input Operands: edx 005f10bc cmp edx,dword ptr [ecx+8] Tainted Input Operands: ecx, edx 005f10bf jbe plds4!pl_arenaallocate+0xe1 (005f1181) Tainted Input Operands: ZeroFlag, CarryFlag Exception Hash (Major/Minor): 0xc45282b.0x2e6e6b60 Stack Trace: plds4!PL_ArenaAllocate+0x17 nss3!PORT_GetError+0x1945 nss3!PK11_GetSlotID+0x1e266 nss3!PK11_GetSlotID+0x1cd3c nss3!PK11_GetSlotID+0x21532 nss3!PK11_GetSlotID+0x21634 nss3!PK11_GetSlotID+0x1f799 nss3!PK11_GetSlotID+0x21489 nss3!PK11_GetSlotID+0x1b0db js3250!JS_DefineFunctions+0x1079 js3250!js_Invoke+0x38ae js3250!JS_DHashTableOperate+0x7a Instruction Address: 0x5f10b7 Description: Data from Faulting Address controls Branch Selection Short Description: TaintedDataControlsBranchSelection Exploitability Classification: UNKNOWN Recommended Bug Title: Data from Faulting Address controls Branch Selection starting at plds4!PL_ArenaAllocate+0x17 (Hash=0xc45282b.0x2e6e6b60) The data from the faulting address is later used to determine whether or not a branch is taken.
From the disassembly I think this is crashing here http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/lib/ds/plarena.c&rev=3.16L&mark=181#168 pool->current ("a") is pointing off into inaccessible memory. Forgot to paste the registers eax=ff9aff93 ebx=00000080 ecx=15ff0875 edx=75ff53f4 esi=00909de0 edi=75ff53f4 eip=005f10b7 esp=0012f00c ebp=0012f070 iopl=0 nv up ei pl nz na po nc cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00010202
In a 3.0.x debug build (that's what was handy) I get the following pair of assertions repeated thousands of times: ###!!! ASSERTION: You can't |write| into an |nsWritingIterator| with no space!: 'size_forward() > 0', file c:\dev\fftrunk\mozilla\obj-dbg\dist\include\string\nsStringIterator.h, line 332 ###!!! ASSERTION: can't advance a writing iterator beyond the end of a string: 'step>0', file c:\dev\fftrunk\mozilla\obj-dbg\dist\include\string\nsStringIterator.h, line 314 The advance fails (we increment the zero step after the second assertion) but it does look like we happily write past the end of the string after the first assertion: "nsCharTraits<value_type>::move(mPosition, s, n)" is just a memmove(). Seems like the assertion in write is wrong. It's checking "size_forward() > 0" but shouldn't it really be "size_forward() > n" ? In any case, we need something stronger than mere assertions.
before the crash i get the following assertions in a 1.9.1 Debug Build before the crash: ###!!! ASSERTION: You can't |write| into an |nsWritingIterator| with no space!: 'size_forward() > 0', file c:\work\mozilla\builds\1.9.1\mozilla\firefox-debug\di st\include\string\nsStringIterator.h, line 324 ### ERROR: WalkStack64: A dynamic link library (DLL) initialization routine fail ed. ###!!! ASSERTION: can't advance a writing iterator beyond the end of a string: ' step>0', file c:\work\mozilla\builds\1.9.1\mozilla\firefox-debug\dist\include\st ring\nsStringIterator.h, line 306 ### ERROR: WalkStack64: A dynamic link library (DLL) initialization routine fail ed. (390.808): Access violation - code c0000005 (!!! second chance !!!) eax=00580058 ebx=7ffdb000 ecx=3e777416 edx=00000000 esi=067a3000 edi=067a2578 eip=1023ca6a esp=0012f484 ebp=0012f48c iopl=0 nv up ei pl nz na po nc cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00000202 MSVCR80D!memmove+0x5a: 1023ca6a f3a5 rep movs dword ptr es:[edi],dword ptr [esi] 0:000> cdb: Reading initial command '!load winext\msec.dll;.logappend;!exploitab le;k;q' Exploitability Classification: PROBABLY_EXPLOITABLE Recommended Bug Title: Probably Exploitable - Read Access Violation on Block Dat a Move starting at MSVCR80D!memmove+0x000000000000005a (Hash=0x69531066.0x075667 18) This is a read access violation in a block data move, and is therefore classifie d as probably exploitable. ChildEBP RetAddr WARNING: Stack unwind information not available. Following frames may be wrong. 0012f48c 02787727 MSVCR80D!memmove+0x5a 0012f4a0 027876da gkparser!nsCharTraits<unsigned short>::move+0x17 0012f4b8 02787643 gkparser!nsWritingIterator<unsigned short>::write+0x4a 0012f4c8 02786b95 gkparser!nsCharSinkTraits<nsWritingIterator<unsigned short> >: :write+0x13 0012f4e0 02786a48 gkparser!copy_multifragment_string+0x45 0012f514 027861f7 gkparser!CopyUnicodeTo+0x78 0012f558 027a41b2 gkparser!nsScannerSubstring::AsString+0x77 0012f564 027b9063 gkparser!CTextToken::GetStringValue+0x12 0012f570 01d4ae10 gkparser!nsCParserNode::GetText+0x23 0012f64c 01d4df37 gklayout!SinkContext::AddLeaf+0x340 0012f664 0279f51d gklayout!HTMLContentSink::AddLeaf+0x57 0012f684 0279a5a5 gkparser!CNavDTD::AddLeaf+0x5d 0012f6d4 0279b8f2 gkparser!CNavDTD::HandleDefaultStartToken+0x3f5 0012f728 027997ad gkparser!CNavDTD::HandleStartToken+0x382 0012f778 02798c98 gkparser!CNavDTD::HandleToken+0x49d 0012f7d4 027afab2 gkparser!CNavDTD::BuildModel+0x298 0012f80c 027af5cc gkparser!nsParser::BuildModel+0xe2 0012f858 027b07eb gkparser!nsParser::ResumeParse+0x1bc 0012f8a8 0186adfc gkparser!nsParser::OnDataAvailable+0x22b 0012f8d0 02b9a79d docshell!nsDocumentOpenInfo::OnDataAvailable+0x4c quit:
Here is the stack for the assertion in comment 10 and 11. CopyUnicodeTo probably fails to deal with OOM somewhere. Investigating. nsWritingIterator<unsigned short>::write(const wchar_t * s=0x0792396c, unsigned int n=3782) Line 324 nsCharSinkTraits<nsWritingIterator<unsigned short> >::write(nsWritingIterator<unsigned short> & iter={...}, const wchar_t * s=0x0792396c, unsigned int n=3782) Line 813 copy_multifragment_string(nsScannerIterator & first={...}, const nsScannerIterator & last={...}, nsWritingIterator<unsigned short> & result={...}) Line 497 CopyUnicodeTo(const nsScannerIterator & aSrcStart={...}, const nsScannerIterator & aSrcEnd={...}, nsAString_internal & aDest={...}) Line 518 nsScannerSubstring::AsString() Line 252 CTextToken::GetStringValue() Line 937 nsCParserNode::GetText() Line 164 SinkContext::AddLeaf(const nsIParserNode & aNode={...}) Line 1121 HTMLContentSink::AddLeaf(const nsIParserNode & aNode={...}) Line 2408 CNavDTD::AddLeaf(const nsIParserNode * aNode=0x06a877c8) Line 3008 CNavDTD::HandleDefaultStartToken(CToken * aToken=0x0477ada0, nsHTMLTag aChildTag=eHTMLTag_text, nsCParserNode * aNode=0x06a877c8) Line 1041 CNavDTD::HandleStartToken(CToken * aToken=0x0477ada0) Line 1390 CNavDTD::HandleToken(CToken * aToken=0x0477ada0) Line 717 CNavDTD::BuildModel(nsITokenizer * aTokenizer=0x04701818, int aCanInterrupt=1, int aCountLines=1, const nsCString * __formal=0x072b617c) Line 304 nsParser::BuildModel() Line 2452
Wow, this is really crappy. The bug is in our core string classes, not related to the parser at all. The problem is essentially in nsTSubstring_CharT::SetLength. The code does: SetCapacity(length); if (Capacity() >= length) mLength = length; The call to SetCapacity fails to allocate enough memory. The next line tries to cover this case. However, Capacity() has this odd quirk that it returns PRUint32(-1) in case the string is immutable, which is the case here since the string is fully uninitialized. It seems to me that the main problem here is the unexpected behavior of Capacity(). I'd imagine there are more functions than SetLength that gets it wrong. The best way I can think of for fixing this would be to make Capacity() return either a signed PRInt32(-1), or simply return 0. Chalk this up as another reason for infallible malloc :(
Assignee: mrbkap → jonas
Component: HTML: Parser → String
QA Contact: parser → string
Attached patch Patch to fix — — Splinter Review
Comment 13 explains what's going wrong. There are currently are 3 callers of Capacity(), two of them didn't handle a returned PRUint32(-1) to indicate immutable-buffer correctly. This patch make Capacity() return 0 if the current buffer is immutable. It also changes SetCapacity to return a PRBool in indicate success/failure so that users don't have to call Capacity afterward. The patch also fixes another problem I found in SetLength. Currently the SetLength bails early if the string already has the exact right length. However this doesn't guarentee that the string is mutable. So the following code (from CopyUnicodeTo) was not OOM safe: if (!EnsureStringLength(aDest, Distance(aSrcStart, aSrcEnd))) { aDest.Truncate(); return; // out of memory } aDest.BeginWriting(writer); copy_multifragment_string(aSrcStart, aSrcEnd, writer); (note that EnsureStringLength uses SetLength to set the length). The patch fixes that by removing the early-return in SetLength. This only results in a few more branches when the existing length is the desired length. And it saves one branch in the common case when SetLength actually modifies the length.
Attachment #412148 - Flags: superreview?(dbaron)
Attachment #412148 - Flags: review?(benjamin)
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.17?
Comment on attachment 412148 [details] [diff] [review] Patch to fix >diff --git a/xpcom/string/public/nsTSubstring.h b/xpcom/string/public/nsTSubstring.h >- NS_COM void NS_FASTCALL SetCapacity( size_type newCapacity ); >+ NS_COM PRBool NS_FASTCALL SetCapacity( size_type newCapacity ); More context would have been nice. A better doccomment explaining what the return value means is essential!
Attachment #412148 - Flags: review?(benjamin) → review+
blocking1.9.1: needed → .7+
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.17?
Flags: blocking1.9.0.17+
Whiteboard: [sg:critical] → [sg:critical] embargo until the Firefox 3.0/3.5 releases (Jan/Feb?)
Comment on attachment 412148 [details] [diff] [review] Patch to fix at the end of SetCapacity; otherwise you're falling off the end of a void function. (Given that, you might want to drop the other return PR_TRUE as well.) I think you should also add assertions to all Capacity implementations that Capacity() doesn't return 0 in any of the "mutable" cases; otherwise you've lost information in this change. (If those assertions fire, then I'd be inclined to skip the change from -1 to 0; it's not critical here.) sr=dbaron with that
Attachment #412148 - Flags: superreview?(dbaron) → superreview+
Lost a line there; that should have started with: You need a |return PR_TRUE| at the end of SetCapacity
(In reply to comment #16) > I think you should also add assertions to all Capacity implementations that > Capacity() doesn't return 0 in any of the "mutable" cases; otherwise you've > lost information in this change. (If those assertions fire, then I'd be > inclined to skip the change from -1 to 0; it's not critical here.) Is there really a difference between a 0-sized buffer an an immutable one? How could you mutate a 0-sized buffer?
Alternatively, let 0 indicate "0-sized or immutable buffer", I can't think of a reason why a caller would care about the distinction (the single remaining one certainly doesn't)
(In reply to comment #19) > Alternatively, let 0 indicate "0-sized or immutable buffer", I can't think of a > reason why a caller would care about the distinction (the single remaining one > certainly doesn't) ok
Fixed on trunk http://hg.mozilla.org/mozilla-central/rev/a80fb8f5552f
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
For what it's worth, I'm not sure that this is [sg:critical] rather than simply [sg:dos]. I didn't look closely, but I *think* we'll null-pointer-deref here. However I'm not sure, so leaving the status as it is for now.
Attached patch 1.9.1 branch patch (obsolete) — — Splinter Review
Attachment #413284 - Flags: approval1.9.1.7?
Comment on attachment 413284 [details] [diff] [review] 1.9.1 branch patch Actually, I don't think we can change the signature for SetCapacity on a stable branch. Smaller patch coming up
Attachment #413284 - Attachment is obsolete: true
Attachment #413284 - Flags: approval1.9.1.7?
This is a smaller patch which should *just* fix the bugs, without the compat-breaking API improvements.
Attachment #413286 - Flags: review?(benjamin)
Comment on attachment 413286 [details] [diff] [review] 1.9.1/1.9.0 branch patch This patch also applies to the 1.9.0 branch. Will ask for approvals once I have review.
Attachment #413286 - Attachment description: 1.9.1 branch patch → 1.9.1/1.9.0 branch patch
Attachment #413286 - Flags: review?(benjamin) → review+
Attachment #413286 - Flags: approval1.9.1.7?
Attachment #413286 - Flags: approval1.9.0.17?
- if (mLength == length) { - mFlags &= ~F_VOIDED; // mutation clears voided flag - return; - } why did you make that change?
See comment 14, starting at "The patch also fixes another problem ..."
Hm OK. But shouldn't you still clear the F_VOIDED flag?
SetCapacity will do that.
Sorry. I misread the implementation of SetDataFlags.
Comment on attachment 413286 [details] [diff] [review] 1.9.1/1.9.0 branch patch Approved for 1.9.1.7 and 1.9.0.17, a=dveditz for release-drivers
Attachment #413286 - Flags: approval1.9.1.7?
Attachment #413286 - Flags: approval1.9.1.7+
Attachment #413286 - Flags: approval1.9.0.17?
Attachment #413286 - Flags: approval1.9.0.17+
Fixed on 1.9.0 and 1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5e3b35c7f7fb Checking in xpcom/string/src/nsPrintfCString.cpp; /cvsroot/mozilla/xpcom/string/src/nsPrintfCString.cpp,v <-- nsPrintfCString.cpp new revision: 1.7; previous revision: 1.6 done Checking in xpcom/string/src/nsTSubstring.cpp; /cvsroot/mozilla/xpcom/string/src/nsTSubstring.cpp,v <-- nsTSubstring.cpp new revision: 1.26; previous revision: 1.25 done
status1.9.1: wanted → .7-fixed
Keywords: fixed1.9.0.17
Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.8pre) Gecko/20100115 Shiretoko/3.5.8pre (.NET CLR 3.5.30729). The PoC no longer crashes Firefox as it does with 1.9.1.7 on the same machine (though it does use up all of my system resources over time).
Keywords: verified1.9.1
Verified for 1.9.0.18 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2010020215 GranParadiso/3.0.18pre (.NET CLR 3.5.30729). The POC no longer crashes as it does when I use 3.0.16 with it. It is unresponsive for a while but eventually you can get back in.
Keywords: fixed1.9.0.18 → verified1.9.0.18
Attachment #410455 - Attachment is private: true
Group: core-security
Can the poc attachment be unhidden?
Whiteboard: [sg:critical] embargo until the Firefox 3.0/3.5 releases (Jan/Feb?) → [sg:critical]
dveditz, why is the attachment hidden? It contains "heap fill" code, but just to cause OOM, no nopslide/shellcode.
yes! release the poc!
Attachment #410455 - Attachment is private: false
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: