624187 - (CVE-2011-0072) Use after free after appending a frame/iframe element to a DOM tree with the NoScript add-on enabled.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Martin Barbella]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101206 Ubuntu/10.04 (lucid) Firefox/3.6.13
Build Identifier: Mozilla Firefox 3.6.13

After appending a frame with a src of "#" to a title element with the NoScript add-on enabled, Firefox will usually crash. This is caused by a read of freed memory. The crash is usually occurring in nsDocShellEnumerator::GetNext(nsISupports**).

I have tested this in Firefox 3.6.13 with NoScript 2.0.9.3 on Ubuntu 10.04 (64 bit) and Windows Vista (32 bit).

Reproducible: Sometimes

Steps to Reproduce:
1. Open the attached testcase, 1.html, in Firefox with the NoScript add-on enabled.
2. If a crash does not occur immediately, either refresh the page a few times, or close the browser. The crash after closing is extremely consistent, but refreshing the page a few times seems to do the trick.
Actual Results:  
The browser will either crash after loading the page, or at exit.

Expected Results:  
The particular script should be executed as expected, and the browser should not crash.

The html file that produces the crash, a stack trace using gdb, and output from valgrind will be attached after this bug is created. If I discover any other useful information about this bug, I will include it.

Comment 1

[Martin Barbella]

Attachment: Testcase which triggers the crash (referred to in the initial report as 1.html).

Comment 2

[Martin Barbella]

Attachment: Stack trace and other information from gdb.

Comment 3

[Martin Barbella]

Attachment: Output from valgrind (with flags --malloc-fill=aa --free-fill=bb --leak-check=no).

Comment 4

[Andreas Gal :gal]

b? so the drivers see this, we still need confirmation whats affected and whats not

Comment 5

[Johnny Stenback (:jst)]

Ben, can you have a look at this one?

Comment 6

[Johnny Stenback (:jst)]

Actually, maybe Mounir could dig in here?

Comment 7

[Mounir Lamouri (:mounir)]

Is there a reason why nsDocShellEnumerator keeps weak reference to nsIDocShellTreeItem?

Comment 8

[Johnny Stenback (:jst)]

Cc:ing more people who might be able to answer Mounir's question above regarding nsDocShellEnumerator...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Uh.... no.  No reason.  And if fact, holding raw weak refs there is clearly unsafe.  The safe fix here (in terms of not leaking, etc) is probably to use an array of nsWeakPtrs.  The next-safer thing is to just make these strong refs and hope.  Note that docshell is not CCed.

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This is fixing the crash locally.

Comment 11

[Daniel Veditz [:dveditz]]

Is this really sg:critical? is there any way to cause this crash in a default configuration?

Comment 12

[Mounir Lamouri (:mounir)]

This patch seems to turn some tests orange on the try server.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

This should pass the try server.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 15

[Daniel Veditz [:dveditz]]

(In reply to comment #11)
> Is this really sg:critical? is there any way to cause this crash in a default
> configuration?

Possibly -- we use nsDocShellEnumerators in our own code (as do several add-ons in addition to NoScript) and could maybe get into this state. Assuming the worst back to [sg:critical?]

Comment 16

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/e23b4fe4e09c

Comment 17

[Mounir Lamouri (:mounir)]

For 1.9.2, the same patch will be usable.
For 1.9.1, it will require more work.

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Patch for 1.9.1

I had to apply a part of https://hg.mozilla.org/mozilla-central/rev/f2f8545e079b
Re-asking a review to double-check.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 503508 [details] [diff] [review]
Patch v1.1

approved for 1.9.2.15, a=dveditz for release-drivers

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 508166 [details] [diff] [review]
Patch for 1.9.1

approved for 1.9.1.18, a=dveditz for release-drivers

Comment 22

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a47c1882ad1d
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/04c4b77a3b8f

Comment 23

[Daniel Veditz [:dveditz]]

The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.