1403646 - Dead object proxies can violate some invariants that JSObject::swap asserts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Not sure whether this is a sec issue, but...

JSObject::swap has this assert:

  MOZ_ASSERT(IsBackgroundFinalized(a->asTenured().getAllocKind()) ==
             IsBackgroundFinalized(b->asTenured().getAllocKind()));

This assert can fail, as in bug 1386490 comment 8, when "a" and "b" are both dead object proxies.

Specifically, if we took an existing non-background-finalized proxy and hueyfixed it to get a dead object proxy, and then we try to pass it to JSCompartment::getNonWrapperObjectForCurrentCompartment, that will land it in WrapperFactory::PrepareForWrapping which will do:

    if (JS_IsDeadWrapper(obj)) {
        retObj.set(JS_NewDeadWrapper(cx, obj));

OK, but JS_NewDeadWrapper creates a background-finalized thing.  And if we're coming in via RemapWrapper, we will now swap() these two objects and violate the assertion.

I'm not sure how to write a test for this yet.  I'll think on it...

Comment 1

[Boris Zbarsky [:bzbarsky]]

One thing that complicates the test situation: I'm not quite sure how to go about ending up with dead object wrappers in RemapWrapper in the first place!

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure dead object proxies have the same background-finalization status as the wrapper they replace

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8912809 [details] [diff] [review]
Make sure dead object proxies have the same background-finalization status as the wrapper they replace

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

Right, so finalizeInBackground() doesn't work for dead proxy objects because the private value is always null.

This is kinda ugly, but it looks like it will fix the problem.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8912809 [details] [diff] [review]
Make sure dead object proxies have the same background-finalization status as the wrapper they replace

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not very easily.  I don't even know how to create this situation without the patch from bug 1355109.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No.  I'm not even sure it _is_ a security problem.

Which older supported branches are affected by this flaw?  Could just be 56, where we're shipping the fix for bug 1355109.  But maybe there's a way to trigger the relevant code before then too...

If not all supported branches, which bug introduced the flaw?  This code has been like this for a while.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I don't have backports for anything before 56 yet, but I expect this would not be too hard to backport.  I expect it to pretty much apply cleanly to whatever older branches we think it might be useful on.

How likely is this patch to cause regressions; how much testing does it need?  The biggest issue is that this patch may be needed to reland bug 1355109, which we want to do ASAP so we get maximal bake time before we uplift it to 57.

Comment 5

[Andrew McCreight [:mccr8]]

I'll conservatively mark this high, as it seems like it could break some GC stuff.

Comment 6

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

sec-approval+ for trunk. This needs a beta nomination for a patch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8912809 [details] [diff] [review]
Make sure dead object proxies have the same background-finalization status as the wrapper they replace

Approval Request Comment
[Feature/Bug causing the regression]: Unknown.  The only known way to trigger
   this so far is with the patch for bug 1355109.
[User impact if declined]: Unclear.  Could lead to weird corruption issues,
   maybe.
[Is this code covered by automated tests?]:  Yes, though not the cases this
   patch fixes.
[Has the fix been verified in Nightly?]: Sort of, yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: I think the risk is medium.
[Why is the change risky/not risky?]:  It's slightly complicated code, but I
   think we got it all right.
[String changes made/needed]: None.

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/296e1b4704deb1c6b3f9a6f5fc56688e89c01117

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/296e1b4704de

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912809 [details] [diff] [review]
Make sure dead object proxies have the same background-finalization status as the wrapper they replace

Fix a sec high, taking it.
Should be in 57b5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f1496c1455f

Comment 12

[Andrei Vaida [:avaida]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7)
> [Is this code covered by automated tests?]:  Yes, though not the cases this
>    patch fixes.
> [Has the fix been verified in Nightly?]: Sort of, yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Boris's assessment on manual testing needs.

Comment 13

[Daniel Veditz [:dveditz]]

Is this patch also needed in a 56 point release if we don't remove bug 1355109 from it?

Comment 14

[Boris Zbarsky [:bzbarsky]]

To fix the crashes, yes.

Past that, I'm not 100% clear on what possible fallout can be.