Bug 926361 (CVE-2013-5618)

ASAN use-after-free in nsNodeUtils::LastRelease on anonymous node from ShowInlineTableEditingUI

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: nils, Assigned: smaug)

Tracking

({csectype-uaf, sec-critical})

Trunk
mozilla27
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox24 wontfix, firefox25 wontfix, firefox26+ fixed, firefox27+ fixed, firefox-esr17 wontfix, firefox-esr2426+ fixed, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [asan][adv-main26+][adv-esr24.2+])

Attachments

(3 attachments)

Reporter

Description

6 years ago
Posted file crash.html
The attached testcase crashes the latest nightly ASAN build. It requires domFuzzLite https://www.squarefree.com/extensions/domFuzzLite3.xpi
Reporter

Comment 1

6 years ago
attaching ASAN output
Assignee

Updated

6 years ago
Assignee: nobody → bugs
Whiteboard: [asan]
Assignee

Comment 2

6 years ago
Posted patch patchSplinter Review
This is lovely. I'd say a bug in NS_RELEASE, but one could say a feature.
So we end up last-releasing <body>, but while doing the last release, 
an <a> has still mParent pointing to <body> (because releasing that mParent caused the initial last-release) and we end up trying to re-unbind.
Attachment #816762 - Flags: review?(jst)
Attachment #816762 - Flags: review?(jst) → review+
Assignee

Comment 3

6 years ago
Comment on attachment 816762 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily. I haven't see this kind of crash ever before, so one needs to figure out a way
to use editor in a particular way and run gc/cc just at the right moment.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Well, the patch is rather obvious, but it is hard to combine that to the functionality of editor/ code

Which older supported branches are affected by this flaw?
all

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
there was a file rename nsGenericElement.cpp -> Element.cpp, but otherwise the code looks the same 

How likely is this patch to cause regressions; how much testing does it need?
Hmm, not totally risk-free. Some code might rely on the mParent to point to somewhere while doing the release...
Attachment #816762 - Flags: sec-approval?
Attachment #816762 - Flags: approval-mozilla-esr24?
Attachment #816762 - Flags: approval-mozilla-esr17?
Attachment #816762 - Flags: approval-mozilla-beta?
Attachment #816762 - Flags: approval-mozilla-b2g18?
Attachment #816762 - Flags: approval-mozilla-aurora?
Release Management, how do you feel about taking this at this point in the cycle? I assume it is probably ok but I wanted to check.
Flags: needinfo?(release-mgmt)
(In reply to Olli Pettay [:smaug] from comment #3)
> How likely is this patch to cause regressions; how much testing does it need?
> Hmm, not totally risk-free. Some code might rely on the mParent to point to
> somewhere while doing the release...

This makes me nervous. I'd need to know more about worst case scenarios. If we do decide to take this, it'll need to land for Thursday's Beta.
Assignee

Comment 6

6 years ago
Well, some code might work differently if element doesn't have parentNode anyway.
Not likely, but in theory possible.
And I'm talking about Gecko internal code.
Crashes or anything like that should be very unlikely.
I think the possibility of a web regressions (if I'm reading your comment correctly) should preclude this from landing in our final beta.
Comment on attachment 816762 [details] [diff] [review]
patch

sec-approval+ for trunk.
Aurora approval. Let's get it in.

I want this in ESR24 but I expect we'll need to land it for the 26+ tracking release, not the current 25+ tracking. So I'm not sure how you want to track it to land it on ESR24 after we branch in two weeks.

I'm going to won't fix this for ESR17 since it is done after the next release.
Attachment #816762 - Flags: sec-approval?
Attachment #816762 - Flags: sec-approval+
Attachment #816762 - Flags: approval-mozilla-esr24?
Attachment #816762 - Flags: approval-mozilla-esr24+
Attachment #816762 - Flags: approval-mozilla-esr17?
Attachment #816762 - Flags: approval-mozilla-beta?
Attachment #816762 - Flags: approval-mozilla-beta-
Attachment #816762 - Flags: approval-mozilla-aurora?
Attachment #816762 - Flags: approval-mozilla-aurora+
Tracking with the ESR24 tracking flag. Duh.
Flags: needinfo?(release-mgmt)
fixed in mozilla-central https://hg.mozilla.org/mozilla-central/rev/77a86dcd14d8
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Whiteboard: [asan] → [asan][land on esr24 and b2g18 when gecko26 is on beta]
Comment on attachment 816762 [details] [diff] [review]
patch

Just a+ on the b2g18 approval for sanity. Although this is already landed
Attachment #816762 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
I'm not able to reproduce the crash on a pre-patch ASan build. Also, I see the same debug output both before and after the patch.

Example output:
WARNING: Someone passed native anonymous content directly into frame construction.  Stop doing that!: file /builds/slave/try-osx64-d-000000000000000000/build/layout/base/nsCSSFrameConstructor.cpp, line 6047

Nils, can you verify on at least one affected branch that the issue no longer reproduces for you? Thanks.
Whiteboard: [asan] → [asan][adv-main26+][adv-esr24.2+]
Alias: CVE-2013-5618
Group: core-security
You need to log in before you can comment on or make changes to this bug.