Last Comment Bug 526500 - (CVE-2009-1571) Use-after-free error in HTML parser when handling out-of-memory conditions (SA37242)
(CVE-2009-1571)
: Use-after-free error in HTML parser when handling out-of-memory conditions (S...
Status: RESOLVED FIXED
[sg:critical]
: crash, verified1.9.0.18, verified1.9.1
Product: Core
Classification: Components
Component: String (show other bugs)
: unspecified
: All All
: P2 critical (vote)
: mozilla1.9.2
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-04 08:23 PST by Reed Loden [:reed] (use needinfo?)
Modified: 2010-08-11 12:43 PDT (History)
19 users (show)
jst: blocking1.9.2+
dveditz: blocking1.9.0.18+
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta4-fixed
.8+
.8-fixed


Attachments
PoC (4.81 KB, application/zip)
2009-11-05 00:00 PST, Reed Loden [:reed] (use needinfo?)
no flags Details
Patch to fix (4.32 KB, patch)
2009-11-12 23:19 PST, Jonas Sicking (:sicking) PTO Until July 5th
benjamin: review+
dbaron: superreview+
Details | Diff | Splinter Review
1.9.1 branch patch (7.65 KB, patch)
2009-11-19 00:12 PST, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Splinter Review
1.9.1/1.9.0 branch patch (1.77 KB, patch)
2009-11-19 00:29 PST, Jonas Sicking (:sicking) PTO Until July 5th
benjamin: review+
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Splinter Review
1.8.0 version (1.19 KB, patch)
2010-01-27 05:17 PST, Martin Stránský
no flags Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2009-11-04 08:23:15 PST
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.
------------------------
Comment 1 Reed Loden [:reed] (use needinfo?) 2009-11-04 08:24:42 PST
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.
Comment 3 Blake Kaplan (:mrbkap) 2009-11-05 05:50:53 PST
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...
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-05 08:42:03 PST
Blocking 1.9.2 on at least figuring out exactly what's causing this and the severity of it.
Comment 5 Blake Kaplan (:mrbkap) 2009-11-05 09:40:53 PST
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.
Comment 6 Reed Loden [:reed] (use needinfo?) 2009-11-05 16:45:41 PST
(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.
Comment 7 Carsten Book [:Tomcat] 2009-11-06 07:43:38 PST
will test this today !
Comment 8 Daniel Veditz [:dveditz] 2009-11-06 12:47:33 PST
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.
Comment 9 Daniel Veditz [:dveditz] 2009-11-06 12:51:42 PST
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
Comment 10 Daniel Veditz [:dveditz] 2009-11-06 13:43:09 PST
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.
Comment 11 Carsten Book [:Tomcat] 2009-11-06 13:48:05 PST
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:
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-12 16:19:19 PST
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
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-12 18:32:36 PST
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 :(
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-12 23:19:49 PST
Created attachment 412148 [details] [diff] [review]
Patch to fix

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.
Comment 15 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-11-16 13:11:24 PST
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!
Comment 16 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2009-11-18 13:18:54 PST
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
Comment 17 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2009-11-18 13:19:53 PST
Lost a line there; that should have started with:

You need a |return PR_TRUE| at the end of SetCapacity
Comment 18 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-18 15:41:30 PST
(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?
Comment 19 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-18 15:47:30 PST
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)
Comment 20 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2009-11-18 15:49:39 PST
(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
Comment 21 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-18 17:15:45 PST
Fixed on trunk

http://hg.mozilla.org/mozilla-central/rev/a80fb8f5552f
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-18 17:17:36 PST
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.
Comment 23 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-18 23:57:12 PST
Fixed in 1.9.2

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0825dfb5f3c9
Comment 24 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-19 00:12:36 PST
Created attachment 413284 [details] [diff] [review]
1.9.1 branch patch
Comment 25 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-19 00:23:17 PST
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
Comment 26 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-19 00:29:56 PST
Created attachment 413286 [details] [diff] [review]
1.9.1/1.9.0 branch patch

This is a smaller patch which should *just* fix the bugs, without the compat-breaking API improvements.
Comment 27 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-19 00:32:01 PST
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.
Comment 28 Christian :Biesinger (don't email me, ping me on IRC) 2009-11-19 14:17:21 PST
-    if (mLength == length) {
-      mFlags &= ~F_VOIDED;  // mutation clears voided flag
-      return;
-    }

why did you make that change?
Comment 29 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-19 14:41:29 PST
See comment 14, starting at "The patch also fixes another problem ..."
Comment 30 Christian :Biesinger (don't email me, ping me on IRC) 2009-11-20 04:50:33 PST
Hm OK. But shouldn't you still clear the F_VOIDED flag?
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-20 12:00:43 PST
SetCapacity will do that.
Comment 32 Christian :Biesinger (don't email me, ping me on IRC) 2009-11-20 12:15:55 PST
Sorry. I misread the implementation of SetDataFlags.
Comment 33 Daniel Veditz [:dveditz] 2009-11-30 14:25:49 PST
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
Comment 34 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-30 18:02:52 PST
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
Comment 35 Al Billings [:abillings] 2010-01-15 16:55:35 PST
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).
Comment 36 Martin Stránský 2010-01-27 05:17:33 PST
Created attachment 423781 [details] [diff] [review]
1.8.0 version
Comment 37 Al Billings [:abillings] 2010-02-05 15:25:36 PST
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.
Comment 38 Martijn Wargers [:mwargers] (not working for Mozilla) 2010-08-02 05:23:00 PDT
Can the poc attachment be unhidden?
Comment 39 Jesse Ruderman 2010-08-02 13:24:13 PDT
dveditz, why is the attachment hidden? It contains "heap fill" code, but just to cause OOM, no nopslide/shellcode.
Comment 40 Juan Pablo Lopez Yacubian 2010-08-11 11:27:29 PDT
yes! release the poc!

Note You need to log in before you can comment on or make changes to this bug.