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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({csectype-uaf, sec-high})

53 Branch
mozilla58
csectype-uaf, sec-high
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 wontfix, firefox57+ fixed, firefox58+ fixed)

Details

(Whiteboard: [adv-main57+])

Attachments

(1 attachment)

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!
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8912809 - Flags: review?(jcoppeard) → review+
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.
Attachment #8912809 - Flags: sec-approval?
Blocks: 1355109
I'll conservatively mark this high, as it seems like it could break some GC stuff.
Keywords: csectype-uaf, sec-high
sec-approval+ for trunk. This needs a beta nomination for a patch.
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox58: --- → affected
tracking-firefox57: --- → +
tracking-firefox58: --- → +
Attachment #8912809 - Flags: sec-approval? → sec-approval+
status-firefox55: --- → ?
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.
Attachment #8912809 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/296e1b4704de
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: ? → unaffected
status-firefox58: affected → fixed
status-firefox-esr52: --- → unaffected
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Attachment #8912809 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: javascript-core-security → core-security-release
(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.
Flags: qe-verify-
Is this patch also needed in a 56 point release if we don't remove bug 1355109 from it?
Flags: needinfo?(bzbarsky)
To fix the crashes, yes.

Past that, I'm not 100% clear on what possible fallout can be.
Flags: needinfo?(bzbarsky)
Whiteboard: [adv-main57+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.