Closed Bug 645565 (CVE-2011-0070) Opened 13 years ago Closed 13 years ago

Crash due to double-delete in nsDirIndexParser


(Core :: Networking, defect)

Not set



Tracking Status
blocking2.0 --- Macaw+
status2.0 --- .1-fixed
blocking1.9.2 --- .17+
status1.9.2 --- .17-fixed
blocking1.9.1 --- .19+
status1.9.1 --- .19-fixed


(Reporter: ianpbeer, Assigned: dveditz)



(Keywords: regression, Whiteboard: [sg:critical?])


(3 files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv: Gecko/20110323 Ubuntu/10.10 (maverick) Firefox/3.6.16
Build Identifier: 3.6.16

By passing a malformed "application/http-index-format" response it is possible to cause a double delete in nsDirIndexParser leading to application crash and potential arbitrary code execution.


(All code below is from netwerk/streamconv/converters/nsDirIndexParser.cpp)

The nsDirIndexParser destructor deletes the member mFormat:

    101 nsDirIndexParser::~nsDirIndexParser() {
    102   delete[] mFormat;

This is usually fine as mFormat is either null (as it is initalised in the Init method) or a value which has been return by new in the method ParseFormat (line 206):

    178 nsDirIndexParser::ParseFormat(const char* aFormatStr) {
    179   // Parse a "200" format line, and remember the fields and their
    180   // ordering in mFormat. Multiple 200 lines stomp on each other.
    182   delete[] mFormat;
    184   // Lets find out how many elements we have.
    185   // easier to do this then realloc
    186   const char* pos = aFormatStr;
    187   unsigned int num = 0;
    188   do {
    189     while (*pos && nsCRT::IsAsciiSpace(PRUnichar(*pos)))
    190       ++pos;
    192     ++num;
    193     // There are a maximum of six allowed header fields (doubled plus
    194     // terminator, just in case) -- Bug 443299
    195     if (num > (2 * NS_ARRAY_LENGTH(gFieldTable)))
    196       return NS_ERROR_UNEXPECTED;
    198     if (! *pos)
    199       break;
    201     while (*pos && !nsCRT::IsAsciiSpace(PRUnichar(*pos)))
    202       ++pos;
    204   } while (*pos);
    206   mFormat = new int[num+1];

mFormat is deleted on entry to this function on line 182.

By passing a correct 200: line (with num < 14), mFormat will be assigned the value returned by new on line 206. If another 200: line is encountered, mFormat will be deleted on 182 but not set to null. If this second 200: line has an incorrect number of header fields it will fail the check on line 195 and an error value will be return, with mFormat retaining the value which was just deleted.

When this instance of nsDirIndexParser is destroyed the destructor will be called, deleting the same mFormat which has already been deleted, leading potentially to a crash or arbitrary code execution.


A simple patch would be to set mFormat to null after it is deleted on line 182.

This issue appears to have been introduced by the fix for Bug 443299 which limited the number of header fields.


I'll attach a simple python script which will generate the required "application/http-index-format" response (based on the PoC for Bug 443299)

Due to the nature of double delete bugs it may not crash every time and may crash with many different stack traces. I am able to reliably crash firefox here (sometimes having to mash f5 a bit, sometimes not.)

I've only tested on FF 3.6.16 32 bit Ubuntu 10.10.

Reproducible: Always

Steps to Reproduce:
Run attached python script

navigate to localhost:8080

crash. (try with different tabs open etc to get different traces)
I got this while trying the test case with 4.0 final.
Ever confirmed: true
I don't know why you'd crash over in nsHtml5HtmlAttributes::~nsHtml5HtmlAttributes but I guess general memory corruption.

On 3.6.17pre Mac I didn't crash after a bunch of reloads, but the test page doesn't do a lot and probably didn't trigger reallocating that memory (or if it freed something used elsewhere I might not crash until some later time). I crashed immediately (fatal abort) when running with MallocScribble turned on. QA should verify this fix using MallocScribble on Mac and gflags on Windows: don't rely on multiple reloads triggering a crash by chance.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Whiteboard: [sg:critical?]
Attached patch branch patchSplinter Review
Rather than null out mFormat right after the delete, what if we delay the delete until we've decided that the new format is good?
Attachment #522781 - Flags: review?(cbiesinger)
Assignee: nobody → dveditz
Attachment #522781 - Attachment is patch: true
Attachment #522781 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 522781 [details] [diff] [review]
branch patch

looks good, but have you considered making making mFormat an nsAutoArrayPtr?
Attachment #522781 - Flags: review?(cbiesinger) → review+
blocking1.9.1: ? → .20+
blocking1.9.2: ? → .18+
blocking2.0: --- → Macaw
status2.0: --- → wanted
Target Milestone: --- → mozilla2.2
blocking2.0: Macaw → ---
OS: Linux → Windows CE
Any particular reason the affected OS for this bug was changed to Windows CE?

I've changed it to 'All' now.
OS: Windows CE → All
gavin: any reason you pulled this out of blocking Macaw?
blocking2.0: --- → ?
Attachment #522781 - Flags: approval2.0?
Attachment #522781 - Flags: approval1.9.2.18?
Attachment #522781 - Flags: approval1.9.2.17?
Attachment #522781 - Flags: approval1.9.1.20?
Attachment #522781 - Flags: approval1.9.1.19?
blocking1.9.1: .20+ → .19+
blocking1.9.2: .18+ → .17+
blocking2.0: ? → Macaw+
Attachment #522781 - Flags: approval1.9.2.18?
Attachment #522781 - Flags: approval1.9.2.17?
Attachment #522781 - Flags: approval1.9.2.17+
Attachment #522781 - Flags: approval1.9.1.20?
Attachment #522781 - Flags: approval1.9.1.19?
Attachment #522781 - Flags: approval1.9.1.19+
Attachment #522781 - Flags: approval2.0? → approval2.0+
Attachment #522781 - Attachment description: patch → branch patch
(In reply to comment #8)
> gavin: any reason you pulled this out of blocking Macaw?

grr. mistake. :(
Alias: CVE-2011-0070
Group: core-security
Keywords: regression
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.