Assertion failure: found() running jit-test basic/bug908915.js with GC zeal

RESOLVED FIXED in Firefox -esr60

Status

()

defect
P1
normal
RESOLVED FIXED
9 months ago
12 days ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

({assertion, csectype-uaf, sec-high})

61 Branch
mozilla65
Points:
---
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr6064+ fixed, firefox63 wontfix, firefox64+ fixed, firefox65+ fixed)

Details

(Whiteboard: [adv-main64+][adv-esr60.4+])

Attachments

(3 attachments, 1 obsolete attachment)

The basic/bug908915.js test fails when I run it locally with zeal mode 10 and the JITs disabled.

$ JS_GC_ZEAL=10 ./jit-test/jit_test.py optdebug-build/shell --args='--no-baseline --no-ion' bug908915

Assertion failure: found(), at /home/jon/clone/inbound/js/src/optdebug-build/dist/include/mozilla/HashTable.h:1273
Exit code: -11
FAIL - basic/bug908915.js
This happens in js::RemapWrapper() and the code that's causing this is:

    // The old value should still be in the cross-compartment wrapper map, and
    // the lookup should return wobj.
    WrapperMap::Ptr p = wcompartment->lookupWrapper(origv);
    MOZ_ASSERT(&p->value().unsafeGet()->toObject() == wobj);

Stack trace:

0x0000561877efb3fa in mozilla::detail::HashTable<mozilla::HashMapEntry<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value> >, mozilla::HashMap<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value>, js::CrossCompartmentKey::Hasher, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Ptr::operator-> (this=0x7ffe82c0d080) at /home/jon/clone/inbound/js/src/default-build/dist/include/mozilla/HashTable.h:1273
1273	      MOZ_ASSERT(found());
(rr) bt 10
#0  0x0000561877efb3fa in mozilla::detail::HashTable<mozilla::HashMapEntry<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value> >, mozilla::HashMap<js::CrossCompartmentKey, js::detail::UnsafeBareReadBarriered<JS::Value>, js::CrossCompartmentKey::Hasher, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::Ptr::operator-> (this=0x7ffe82c0d080) at /home/jon/clone/inbound/js/src/default-build/dist/include/mozilla/HashTable.h:1273
#1  0x0000561878740c11 in js::RemapWrapper (cx=0x7f3150319000, wobjArg=0x7f314f14ee00, newTargetArg=0x7f314f161d60)
    at /home/jon/clone/inbound/js/src/proxy/CrossCompartmentWrapper.cpp:634
#2  0x00005618787416a6 in js::RecomputeWrappers (cx=0x7f3150319000, sourceFilter=warning: RTTI symbol not found for class 'RecomputeWrappers(JSContext*, unsigned int, JS::Value*)::SingleOrAllCompartments'
..., targetFilter=warning: RTTI symbol not found for class 'RecomputeWrappers(JSContext*, unsigned int, JS::Value*)::SingleOrAllCompartments'
...)
    at /home/jon/clone/inbound/js/src/proxy/CrossCompartmentWrapper.cpp:745
#3  0x0000561877ec294a in RecomputeWrappers (cx=0x7f3150319000, argc=0, vp=0x7ffe82c0d750) at /home/jon/clone/inbound/js/src/shell/js.cpp:6432
#4  0x0000561877fe9ed3 in CallJSNative (cx=0x7f3150319000, native=0x561877ec27c0 <RecomputeWrappers(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/jon/clone/inbound/js/src/vm/Interpreter.cpp:468
Assignee: nobody → jcoppeard
Group: javascript-core-security
Posted patch bug1506640 (obsolete) — Splinter Review
The problem is that we need to trace all wrapper rooters while we are marking (explanation below) and we currently only do this when we enter GCRuntime::incrementalSlice() in the Mark state.  But we are still marking when we enter in the Sweep state, for those zones that we have not yet started sweeping.  So we need to do this there too.

The reason for this wrapper rooting is as follows.  Normally we would have a read barrier when accessing cross compartment wrappers, but if we did that then remapping all wrappers would mark every wrapper and any dead wrappers would be kept alive until the next collection.  In this case we know that the wrappers do not escape from the remapping code so we put them in an AutoWrapperVector and only mark then if we happen to do any marking while we're touching them.  This is like a delayed read barrier - and if we don't happen to do a slice while the vector is live then we can safely skip the barrier.

It was a little fiddly coming up with a test that reproduced this.  The basic idea is to create a bunch of wrappers (the values of the other global), remove references to them and then call recomputeWrappers() while we are sweeping in an incremental GC.
Attachment #9024772 - Flags: review?(pbone)
Any idea what the severity of this issue is, Jon?
Flags: needinfo?(jcoppeard)
Flags: in-testsuite?
Keywords: assertion
Comment on attachment 9024772 [details] [diff] [review]
bug1506640

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

This makes sense.  If you can answer the questions r+

Do we need to wait before landing this due to security?

::: js/src/gc/GC.cpp
@@ +7586,5 @@
>  
>        case State::Sweep:
>          MOZ_ASSERT(nursery().isEmpty());
> +
> +        AutoGCRooter::traceAllWrappers(rt->mainContextFromOwnThread(), &marker);

I had a question about whether it is best to always do this or check if some zones are still marking.  But if there are no wrappers in the stack then this should be cheap.  Is that right?

Would it be inefficient/bad to do this tracing when no zones are marking?
Attachment #9024772 - Flags: review?(pbone) → review+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> Any idea what the severity of this issue is, Jon?

I don't think it could be used directly or easilly.  It could be used to currupt memory, if you could then use that to attack something else then it could be dangerous.  I'll let Jon confirm.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
It's a UAF but not easily exploitable.  The attacker doesn't have control over the object accessed after it's freed.  I'm going with sec-high.
Flags: needinfo?(jcoppeard)
(In reply to Paul Bone [:pbone] from comment #4)
> Do we need to wait before landing this due to security?

Yes, this needs security approval to land (see https://wiki.mozilla.org/Security/Bug_Approval_Process for details).

> I had a question about whether it is best to always do this or check if some
> zones are still marking.  But if there are no wrappers in the stack then
> this should be cheap.  Is that right?
> 
> Would it be inefficient/bad to do this tracing when no zones are marking?

You are correct that this is unnecessary if we're sweeping the last sweep group as there will be no zones that we are still marking.  But this should be cheap.  And eventually the idea is to move away from using AutoRooters at allexcept for this one case for wrappers.
Comment on attachment 9024772 [details] [diff] [review]
bug1506640

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I'd say very difficult.

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?: All supported branches.

If not all supported branches, which bug introduced the flaw?: None

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: This patch should apply.

How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions.
Attachment #9024772 - Flags: sec-approval?
Doesn't this test expose the issue? We don't normally check in a new test with a sec-high fix...
Flags: needinfo?(jcoppeard)
Good point.  I'll land the test separately.
Flags: needinfo?(jcoppeard)
Sec-approval+ for the patch without a test. :-)
Comment on attachment 9024772 [details] [diff] [review]
bug1506640

We'll want patches nominated for affected branches too.
Attachment #9024772 - Flags: sec-approval? → sec-approval+
Patch to add test.
Attachment #9025322 - Flags: review+
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/0e459cfc0d2a
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please nominate this for Beta/ESR60 approval when you get a chance.
Flags: needinfo?(jcoppeard)
Updated patch without testcase.
Attachment #9024772 - Attachment is obsolete: true
Flags: needinfo?(jcoppeard)
Attachment #9025877 - Flags: review+
Comment on attachment 9025877 [details] [diff] [review]
bug1506640-patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Incremental GC

User impact if declined: Possible crash / security vulnerability.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a very simple change to mark wrappers at the start of sweep slices.

String changes made/needed: None
Attachment #9025877 - Flags: approval-mozilla-beta?
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug.

User impact if declined: Possible crash / security vulnvrability.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a very simple change to mark wrappers at the start of sweep slices.

String or UUID changes made by this patch: None
Attachment #9025882 - Flags: review+
Attachment #9025882 - Flags: approval-mozilla-esr60?
Comment on attachment 9025877 [details] [diff] [review]
bug1506640-patch

[Triage Comment]
Approved for 64.0b11 and 60.4.0esr.
Attachment #9025877 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9025882 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main64+][adv-esr60.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.