806433 - Use-after-free of XPCIncrementalReleaseRunnable

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Try run for a patch that replaces the raw pointer with a nsRefPtr: https://tbpl.mozilla.org/?tree=Try&rev=0bf2a42144d4

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: make mReleaseRunnable a ref ptr

Seems okay on try so far...

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 676381 [details] [diff] [review]
make mReleaseRunnable a ref ptr

Remove mReleaseRunnable(nullptr) from the ctor.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: make mReleaseRunnable a ref ptr

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.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

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 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 676765 [details] [diff] [review]
make mReleaseRunnable a ref ptr

Please nominate for affected branches as well.

Comment 8

[Andrew McCreight [:mccr8]]

I'll land it myself once the tree has settled down.

Comment 9

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fdc6565bae9

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6fdc6565bae9

Comment 11

[Andrew McCreight [:mccr8]]

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

Comment 12

[Andrew McCreight [:mccr8]]

Thanks.

https://hg.mozilla.org/releases/mozilla-beta/rev/3baf42fac879

Aurora is closed right now, so I'll land it there later.

Comment 13

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4ebbccbcdae