645565 - (CVE-2011-0070) Crash due to double-delete in nsDirIndexParser

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Ian Beer]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.2.16) 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.

Details:

(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.
    181 
    182   delete[] mFormat;
    183 
    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;
    191     
    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;
    197 
    198     if (! *pos)
    199       break;
    200 
    201     while (*pos && !nsCRT::IsAsciiSpace(PRUnichar(*pos)))
    202       ++pos;
    203 
    204   } while (*pos);
    205 
    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)

Comment 1

[Ian Beer]

Attachment: PoC - serves on localhost:8080

Comment 2

[Ludovic Hirlimann [:Usul]]

I got this https://crash-stats.mozilla.com/report/index/bp-027acac0-cf23-4875-a0ca-abcbc2110328 while trying the test case with 4.0 final.

Comment 3

[Daniel Veditz [:dveditz]]

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.

Comment 4

[Daniel Veditz [:dveditz]]

Attachment: branch patch

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?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 522781 [details] [diff] [review]
branch patch

looks good, but have you considered making making mFormat an nsAutoArrayPtr?

Comment 7

[Ian Beer]

Any particular reason the affected OS for this bug was changed to Windows CE?

I've changed it to 'All' now.

Comment 8

[Daniel Veditz [:dveditz]]

gavin: any reason you pulled this out of blocking Macaw?

Comment 9

[Daniel Veditz [:dveditz]]

Attachment: trunk patch (minor context diff)

Comment 10

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-2.0/rev/fe85a4b7e1ca
http://hg.mozilla.org/mozilla-central/rev/5c1d236f0d5f

Comment 11

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a661e51cc977
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7238621a4e1f

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> gavin: any reason you pulled this out of blocking Macaw?

grr. mistake. :(