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: