Possible crash if object containing GCPtr destroyed after failed initialisation

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({sec-moderate})

unspecified
mozilla50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox48 wontfix, firefox49+ fixed, firefox-esr4549+ fixed, firefox50+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main49+][adv-esr45.4+])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
The GCPtr destructor contains this assertion as to when it's safe to destroy one of these things:

        // No prebarrier necessary as this only happens when we are sweeping or
        // before the containing object becomes part of the GC graph.
        MOZ_ASSERT(CurrentThreadIsGCSweeping() || CurrentThreadIsHandlingInitFailure());

This doesn't take account of the fact that a post-barrier may have put a pointer to this GCPtr into the storebuffer.  If we do a minor GC in this state we'll attempt to mark whatever garbage ends up at this location.

This only happens when an object containing a GCPtr is destroyed due to initialization failure (otherwise they are destroyed during sweeping which is fine).  

We already have AutoInitGCManagedObject that is supposed to take care of issues like this for us.  A possible fix is to make that put the object on a queue whose contents are destroyed after the next minor GC.  Also, we need jsapi-tests for this.
(Assignee)

Comment 1

3 years ago
Posted patch bug1288780-gc-ptr (obsolete) — Splinter Review
Here's a failing test case.
(Assignee)

Updated

3 years ago
Assignee: nobody → jcoppeard
(Assignee)

Comment 2

3 years ago
Here's a patch to implement the proposed fix.

It removes AutoInitGCManagedObject and replaces it with GCManagedDeletePolicy.  The latter can be used as the delete policy for a UniquePtr, or used to implement the default delete policy for the relevant classes (I've done the latter here).

The delete policy queues a thunk to delete the object after the next minor GC, by which time the store buffer will be empty.  It uses a nursery allocation for the linked list element.  If the nursery is disabled or we are not on the main thread then the problem doesn't occur anyway and we can delete the object immediately.  Similarly, we evict the nursery instead if we can't allocate (and this path is exercised by our OOM testing).

UnboxedLayout caused me a lot of problems with the assertions in the GCPtr destructor because it can theoretically be constructed and hence destroyed off main thread (I never observed this though).  This is safe as stated previously.  I relaxed the assertion to allow destruction off main thread where the wrapper doesn't contain a nursery pointer.  I feel like that's a bit of a cop-out but couldn't see a better way.

Also, UnboxedLayout is constructed when we are inside an AutoSuppressGC region.  I had to add a UniquePtr in AutoEnterAnalysis for it to use so that it was cleaned up after then end of the GC suppression.
Attachment #8773860 - Attachment is obsolete: true
Attachment #8775250 - Flags: review?(terrence)
Comment on attachment 8775250 [details] [diff] [review]
bug1288780-gc-ptr v1

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

::: js/src/gc/Barrier.h
@@ +454,5 @@
> +        // after we have just collected the nursery.
> +        bool inNursery = InternalBarrierMethods<T>::isInsideNursery(this->value);
> +        MOZ_ASSERT(CurrentThreadIsGCSweeping() ||
> +                   CurrentThreadCanSkipPostBarrier(inNursery));
> +        memset(this, JS_FREED_HEAP_PTR_PATTERN, sizeof(*this));

Nice touch! Please use the Poison function so that we have global control over when this is enabled.

::: js/src/gc/Nursery.cpp
@@ +73,5 @@
> +    uint32_t padding;
> +#endif
> +};
> +
> +

Extra whitespace.

@@ +829,5 @@
> +        runtime()->gc.evictNursery();
> +        AutoSetThreadIsSweeping threadIsSweeping;
> +        thunk(data);
> +        return;
> +    }

Neat! Is the intention that we can implement our other ad-hoc sweeping activity with this mechanism?
Attachment #8775250 - Flags: review?(terrence) → review+
(Assignee)

Comment 5

3 years ago
Comment on attachment 8775250 [details] [diff] [review]
bug1288780-gc-ptr v1

Ah, I forgot to get approval before landing, sorry about that.

[Security approval request comment]

How easily could an exploit be constructed based on the patch? Difficult, would require an attacker to understand the changes and work out where this problem currently manifests itself.  I'm not sure it's even an issue in the current codebase just that it has the possibility to become one.

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? Pretty much everything.

If not all supported branches, which bug introduced the flaw? Bug 1165966 was the previous attempt to address this issue but the underlying problem may be have there since bug 619558.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Shouldn't be too hard to create patches along the same lines.

How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8775250 - Flags: sec-approval?
backed for causing hazard failures like https://treeherder.mozilla.org/logviewer.html#?job_id=32784246&repo=mozilla-inbound
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 7

3 years ago
Steve, I want to do something like the following in a jsapi test:

    auto heapPtr = cx->make_unique<GCPtr<JSObject*>>();
    // ...
    cx->minorGC(JS::gcreason::API);

Is there any way I can do this without the hazard analysis complaining that heapPtr is unrooted?
Flags: needinfo?(jcoppeard) → needinfo?(sphink)
Let's try to get this into aurora (49) now and then work on ESR for next cycle (e.g. the one shipping with 49).
I guess the problem is that you're basically putting a heap pointer on the stack, or rather in UniquePtr memory, which the analysis treats as unsafe as the stack.

Which is ok here because... oh, because it's a jsapi-test. So to confirm: you want a way to avoid the analysis for a test only, right? As in, it would still be correct for the analysis to complain for the above code normally?

Assuming so, I think this seems like a totally appropriate use of AutoSuppressGCAnalysis. You're sort of intentionally setting up the situation that the analysis protects against: you're putting a GC pointer into memory that doesn't get traced, and then you GC. If your test did another GC after the minorGC, then this would be a UAF, right?

I'm working on something that would allow annotating specific variables as holding only non-GC pointers even though their type says they can have GC pointers. It's intended for use in cases where you know you only have a numeric Value or whatever, but it would be handy here. I wouldn't wait for it, though; I'm not sure how feasible it is yet.
Flags: needinfo?(sphink)
Attachment #8775250 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/1506fafba57d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 14

3 years ago
Approval Request Comment
[Feature/regressing bug #]: Bug 619558
[User impact if declined]: Possible (though unlikely) crash / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On central since 30th July
[Risks and why]: Low
[String/UUID change made/needed]:
Attachment #8777254 - Flags: review+
Attachment #8777254 - Flags: approval-mozilla-beta?
(Assignee)

Comment 15

3 years ago
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible (though unlikely) crash / security vulnerability.
Fix Landed on Version: 50
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8777255 - Flags: review+
Attachment #8777255 - Flags: approval-mozilla-esr45?
Comment on attachment 8777254 [details] [diff] [review]
bug1288780-beta

Sec-mod, still early in the beta cycle, Beta49+
Attachment #8777254 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8777255 [details] [diff] [review]
bug1288780-esr45

Sec-mod, ESR45+
Attachment #8777255 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Group: javascript-core-security → core-security-release
(Assignee)

Updated

3 years ago
Depends on: 1292564
(Assignee)

Updated

3 years ago
Depends on: 1292529
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main49+][adv-esr45.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.