Possible use-after-CC-Unlink due to incorrect cycle collector WeakMap optimizations

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({regression, sec-high})

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox17 wontfix, firefox18 wontfix, firefox19+ fixed, firefox20+ fixed, firefox-esr10 unaffected, firefox-esr1719+ fixed, b2g1819+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [adv-main19+][adv-esr1703+])

Attachments

(1 attachment, 3 obsolete attachments)

In pseudocode, the GC's WeakMap::markIteratively works like this:
  if (key is marked) {
    mark value
  } else (delegate is marked) {
    mark key
    mark value
  }

This means that even if the value is marked when we start, the key will get marked if the delegate is marked.

On the other hand, the cycle collector will bail out at a number of points (TraceWeakMapping, GCGraphBuilder::NoteWeakMapping, maybe other places) if the value is marked black, so we never hit the equivalent code in ScanWeakMaps, which I think means that the key can end up being marked as dead in the CC when it is alive in the GC, which means we'll Unlink a live object, which is not awesome.

I don't really want to eliminate the optimization entirely, as I imagine the bulk of things in weak maps are black. Maybe changing the bailouts to check if the key AND value are black would work. I don't really want to model full logic from markIteratively in the optimization that is in the plumbing.
This is not dangerous by itself, but use-after-CC-Unlink errors can be combined with errors in Unlink functions to cause badness.
Assignee: nobody → continuation
Posted patch WIP (obsolete) — Splinter Review
This patch does a number of things.

1. At the start of TraceWeakMapping, we only bail out early if both the key and value are things we aren't going to care about in the CC.

2. The child tracer, which handles the weird fanout case where the value isn't representable in the CC so we instead add all of its children (maybe not a problem post E4X removal, because I think the only cases for this are XML and shapes), could encounter only values that we optimize out of the CC, and thus not report any entries to the CC, which would prevent the delegate->key edge from being communicated to the CC. To handle this case, we ensure that at least one entry is reported, with an empty NULL value.

3. Don't bother trying to optimize away weak map entries in GCGraphBuilder::NoteWeakMapping any more: this should be handled in XPC, and I don't want to duplicate the weird logic.

4. The rest of the changes in the CC are accounting for the fact that we now sometimes pass NULL as the value pointer, to handle (2).

The good news that 3 and 4 make the code a little simpler and more uniform...
Attachment #682510 - Flags: review?(continuation)
Attachment #682510 - Flags: review?(continuation) → review?(wmccloskey)
Comment on attachment 682510 [details] [diff] [review]
WIP

This isn't quite right for the !AddToCCKind(vkind) case.
Attachment #682510 - Flags: review?(wmccloskey)
Hmm.  I think this may not actually be a problem after all, because the only keys this can apply to are Proxies, and the only outgoing edge of the proxy is to the wrappee, so if the wrappee is really alive, then tricking the CC into thinking the wrapper isn't alive doesn't help, because the CC already thinks the wrappee is alive.

You can also confuse the CC about whether the entry is alive or not... but the only case where this applies is when the value is either marked black, and thus definitely alive, or something that the CC doesn't care about, so I think that's irrelevant.

Marking moderate for now, but I want to think about this for a bit. And we may want to fix this anyways, because it is unsettling.
Keywords: sec-highsec-moderate
Posted patch don't optimize as much (obsolete) — Splinter Review
Attachment #682510 - Attachment is obsolete: true
Attachment #683652 - Flags: review?(wmccloskey)
Summary: Use-after-CC-Unlink due to incorrect cycle collector WeakMap optimizations → Possible use-after-CC-Unlink due to incorrect cycle collector WeakMap optimizations
Okay, never mind, after talking to billm it sounds like the wrapper could have point to other things besides its wrappee.
Keywords: sec-moderatesec-high
Posted patch optimize less (obsolete) — Splinter Review
jorendorff said watchpoint closures can be null, so I went through this function and added a bunch of null checks for keys and values.  Gross.
Attachment #683652 - Attachment is obsolete: true
Attachment #683652 - Flags: review?(wmccloskey)
Attachment #684224 - Flags: review?(wmccloskey)
Attachment #684224 - Attachment is obsolete: true
Attachment #684224 - Flags: review?(wmccloskey)
Attachment #684231 - Flags: review?(wmccloskey)
Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

Review of attachment 684231 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsCycleCollector.cpp
@@ +2268,5 @@
>                  anyChanged = true;
>              }
>  
> +            if (mColor == black && kColor == black && vColor != black) {
> +                GraphWalker<ScanBlackVisitor>(ScanBlackVisitor(mWhiteNodeCount)).Walk(wm->mVal);

Do you need a null check on wm->mVal here? Or will Walk handle it?
Attachment #684231 - Flags: review?(wmccloskey) → review+
> Do you need a null check on wm->mVal here? Or will Walk handle it?

(vColor != black) implies (v != null), because (v == null) implies (vColor == black).
(from the definition of vColor)
Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

[Security approval request comment]
How easily can the security issue be deduced from the patch? Probably not very easily.

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? 17+

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Shouldn't be any problem, as I think this code hasn't changed much.

How likely is this patch to cause regressions; how much testing does it need? Probably not too bad, but this code is a little bit tricky.
Attachment #684231 - Flags: sec-approval?
Attachment #684231 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/62d807d96900
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Internally discovered, a bit tricky, Let's get this landed on Aurora, and then fixed on the ESR17 in the 19+ timeframe.
Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 798678
User impact if declined: possible crashes, security problems
Testing completed (on m-c, etc.): it has been on m-c for a few days
Risk to taking this patch (and alternatives if risky): shouldn't be too bad. most of the changes in this patch are fairly simple. but of course, all of this code is a little unpleasant.
String or UUID changes made by this patch: none
Attachment #684231 - Flags: approval-mozilla-aurora?
Keywords: regression
Attachment #684231 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm assuming that the 19+ on the ESR-17 thing means this shouldn't land there yet.
Yep - please prepare a patch for both ESR17 and B2G18. Thanks!
Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 19, 20
Risk to taking this patch (and alternatives if risky): should be low
String or UUID changes made by this patch: none

This patch also applies to b2g18 cleanly. I'm assuming I shouldn't flag it for approval yet.
Attachment #684231 - Flags: approval-mozilla-esr17?
Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

Approving this low risk fix for esr17. You can nominate for b2g18 but we won't be approving those uplifts for tracking-b2g18=19+ until after 1/25
Attachment #684231 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Comment on attachment 684231 [details] [diff] [review]
optimize less, null check more

[Approval Request Comment]
Oops, I guess this never got nominated. See comment 2.
Attachment #684231 - Flags: approval-mozilla-b2g18?
Blocks: 690970
Attachment #684231 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Whiteboard: [adv-main19+][adv-esr1703+]
Marking as tef+ - we should either uplift to v1.0.1 or mark that flag as fixed.
blocking-b2g: --- → tef+
Looks fixed to me in the repo.
Group: core-security
You need to log in before you can comment on or make changes to this bug.