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

RESOLVED FIXED in Firefox 19

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({regression, sec-high})

Trunk
mozilla20
regression, sec-high
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)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
This is not dangerous by itself, but use-after-CC-Unlink errors can be combined with errors in Unlink functions to cause badness.
status-firefox-esr10: --- → unaffected
status-firefox17: --- → wontfix
status-firefox18: --- → affected
status-firefox19: --- → affected
Keywords: sec-high
(Assignee)

Updated

6 years ago
Assignee: nobody → continuation
(Assignee)

Comment 4

6 years ago
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...
(Assignee)

Updated

6 years ago
Attachment #682510 - Flags: review?(continuation)
(Assignee)

Updated

6 years ago
Attachment #682510 - Flags: review?(continuation) → review?(wmccloskey)
(Assignee)

Comment 5

6 years ago
Comment on attachment 682510 [details] [diff] [review]
WIP

This isn't quite right for the !AddToCCKind(vkind) case.
Attachment #682510 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

6 years ago
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-high → sec-moderate
(Assignee)

Comment 7

6 years ago
Created attachment 683652 [details] [diff] [review]
don't optimize as much
Attachment #682510 - Attachment is obsolete: true
Attachment #683652 - Flags: review?(wmccloskey)
(Assignee)

Updated

6 years ago
Summary: Use-after-CC-Unlink due to incorrect cycle collector WeakMap optimizations → Possible use-after-CC-Unlink due to incorrect cycle collector WeakMap optimizations
(Assignee)

Comment 8

6 years ago
Okay, never mind, after talking to billm it sounds like the wrapper could have point to other things besides its wrappee.
Keywords: sec-moderate → sec-high
(Assignee)

Comment 9

6 years ago
Created attachment 684224 [details] [diff] [review]
optimize less

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)
(Assignee)

Comment 10

6 years ago
Created attachment 684231 [details] [diff] [review]
optimize less, null check more
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+
(Assignee)

Comment 12

6 years ago
> 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).
(Assignee)

Comment 13

6 years ago
(from the definition of vColor)
(Assignee)

Comment 14

6 years ago
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?

Updated

6 years ago
Attachment #684231 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/62d807d96900
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox20: affected → fixed
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.
status-firefox18: affected → wontfix
status-firefox-esr17: --- → affected
tracking-firefox19: --- → +
tracking-firefox20: --- → +
tracking-firefox-esr17: --- → 19+
(Assignee)

Comment 18

6 years ago
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?
(Assignee)

Updated

6 years ago
Keywords: regression
Attachment #684231 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 19

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3e93b76c6bc
status-firefox19: affected → fixed
(Assignee)

Comment 20

6 years ago
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!
status-b2g18: --- → affected
tracking-b2g18: --- → 19+
(Assignee)

Comment 22

6 years ago
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+
(Assignee)

Comment 24

6 years ago
https://hg.mozilla.org/releases/mozilla-esr17/rev/e9cbc3f091c6
status-firefox-esr17: affected → fixed
(Assignee)

Comment 25

6 years ago
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?
(Assignee)

Updated

6 years ago
Blocks: 690970

Updated

6 years ago
Attachment #684231 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
(Assignee)

Comment 26

6 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/9d285ca1bf1a
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → affected

Updated

6 years ago
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+
status-b2g18-v1.0.1: --- → affected
(Assignee)

Comment 28

5 years ago
Looks fixed to me in the repo.
status-b2g18-v1.0.0: affected → wontfix
status-b2g18-v1.0.1: affected → fixed
Group: core-security
You need to log in before you can comment on or make changes to this bug.