Bug 645565 (CVE-2011-0070)

Crash due to double-delete in nsDirIndexParser

RESOLVED FIXED in mozilla5

Status

()

Core
Networking
--
critical
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: Ian Beer, Assigned: dveditz)

Tracking

({regression})

unspecified
mozilla5
x86
All
regression
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(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)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
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)
(Reporter)

Comment 1

7 years ago
Created attachment 522256 [details]
PoC - serves on localhost:8080
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

7 years ago
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: --- → ?
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Whiteboard: [sg:critical?]
(Assignee)

Comment 4

7 years ago
Created attachment 522781 [details] [diff] [review]
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?
Attachment #522781 - Flags: review?(cbiesinger)
(Assignee)

Updated

7 years ago
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+
(Assignee)

Updated

7 years ago
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
(Reporter)

Comment 7

7 years ago
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
(Assignee)

Comment 8

7 years ago
gavin: any reason you pulled this out of blocking Macaw?
blocking2.0: --- → ?
(Assignee)

Updated

7 years ago
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?
(Assignee)

Updated

7 years ago
blocking1.9.1: .20+ → .19+
blocking1.9.2: .18+ → .17+
blocking2.0: ? → Macaw+

Updated

7 years ago
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+

Updated

7 years ago
Attachment #522781 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 9

7 years ago
Created attachment 525319 [details] [diff] [review]
trunk patch (minor context diff)
(Assignee)

Updated

7 years ago
Attachment #522781 - Attachment description: patch → branch patch
(Assignee)

Comment 10

7 years ago
http://hg.mozilla.org/releases/mozilla-2.0/rev/fe85a4b7e1ca
http://hg.mozilla.org/mozilla-central/rev/5c1d236f0d5f
Status: NEW → RESOLVED
Last Resolved: 7 years ago
status2.0: wanted → .1-fixed
Resolution: --- → FIXED
(Assignee)

Comment 11

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a661e51cc977
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7238621a4e1f
status1.9.1: wanted → .19-fixed
status1.9.2: wanted → .17-fixed
(In reply to comment #8)
> gavin: any reason you pulled this out of blocking Macaw?

grr. mistake. :(
(Assignee)

Updated

6 years ago
Alias: CVE-2011-0070
(Assignee)

Updated

6 years ago
Group: core-security
(Assignee)

Updated

6 years ago
Blocks: 443299
Keywords: regression
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.