Closed Bug 806433 Opened 8 years ago Closed 8 years ago

Use-after-free of XPCIncrementalReleaseRunnable


(Core :: XPConnect, defect)

Not set



Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 + fixed
firefox19 --- fixed
firefox-esr10 --- unaffected


(Reporter: mccr8, Assigned: mccr8)



(Keywords: csectype-uaf, regression, sec-critical, Whiteboard: [adv-track-main17-])


(1 file, 1 obsolete file)

I'm pretty sure the assertion failing in bug 802471 indicates a use-after-free bug: we're calling the destructor on an object that the XPCJSRuntime still has a pointer to.

At a first glance, it seems sketchy that mReleaseRunnable is a raw pointer instead of a refcounted pointer.  Is there any reason why it can't be a refcounted pointer?  I'm looking into precisely how this is happening to try to figure out the lifetime, in case this assertion indicates something else is actually going wrong.

I'm not sure why this assertion suddenly started showing up.  Maybe bug 791798 jumbled up the behavior enough to cause some new ordering, though the timing isn't quite right.

I'm speculatively marking this as affecting 17, 18, 19, and as a sec-critical.
Try run for a patch that replaces the raw pointer with a nsRefPtr:
Attached patch make mReleaseRunnable a ref ptr (obsolete) — Splinter Review
Seems okay on try so far...
Attachment #676381 - Flags: review?(bugs)
Attachment #676381 - Flags: feedback?(wmccloskey)
Comment on attachment 676381 [details] [diff] [review]
make mReleaseRunnable a ref ptr

Remove mReleaseRunnable(nullptr) from the ctor.
Attachment #676381 - Flags: review?(bugs) → review+
Carrying forward Olli's r+.

[Security approval request comment]
How easily can the security issue be deduced from the patch? I changed a weak pointer to a strong pointer, which generally indicates some kind of UAF. But if you know what to look for, the indication that there's a UAF here is already present in the public bug 802471.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? This was initially inspired by a public random orange, bug 802471, which has only shown up on 19, but I'll want to land this on 17 and 18 to be safe.

If not all supported branches, which bug introduced the flaw? bug 743112

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Same patch should work, low risk.

How likely is this patch to cause regressions; how much testing does it need?  The only possible problem would be leaks, but that seems unlikely given the nature of the object we're creating a strong ref to.
Attachment #676381 - Attachment is obsolete: true
Attachment #676381 - Flags: feedback?(wmccloskey)
Attachment #676765 - Flags: sec-approval?
Attachment #676765 - Flags: review+
Blocks: 743112
I'm still not exactly sure why this is happening on 19, but not earlier. Peter touched this code around the start of October, but it was 2 weeks before it started showing up on Tinderbox.
Furthermore, the particular sequence of events that would lead to this is unclear.  Somehow we're calling XPCIncrementalReleaseRunnable::Run() but we end up returning while (a) there are still items to process and (b) we haven't AddRefed |this|, which it seems like NS_DispatchToMainThread should do on success.
Comment on attachment 676765 [details] [diff] [review]
make mReleaseRunnable a ref ptr

Please nominate for affected branches as well.
Attachment #676765 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
I'll land it myself once the tree has settled down.
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 676765 [details] [diff] [review]
make mReleaseRunnable a ref ptr

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 743112
User impact if declined: possible security problems, crashes
Testing completed (on m-c, etc.): it has been on m-c for a day or two, with no signs of regressions
Risk to taking this patch (and alternatives if risky): low, only thing I can think of is that it maybe could cause leaks
String or UUID changes made by this patch: none
Attachment #676765 - Flags: approval-mozilla-beta?
Attachment #676765 - Flags: approval-mozilla-aurora?
Attachment #676765 - Flags: approval-mozilla-beta?
Attachment #676765 - Flags: approval-mozilla-beta+
Attachment #676765 - Flags: approval-mozilla-aurora?
Attachment #676765 - Flags: approval-mozilla-aurora+

Aurora is closed right now, so I'll land it there later.
Keywords: checkin-needed
Whiteboard: [adv-track-main17-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.