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...

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 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 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.

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

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

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 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

(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.

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

To fix the crashes, yes. Past that, I'm not 100% clear on what possible fallout can be.