Jon Coppeard (:jonco) Assignee
	Description • 6 years ago

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

	Jon Coppeard (:jonco) Assignee
	Comment 1 • 6 years ago

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

	Jon Coppeard (:jonco) Assignee
	Comment 2 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 3 • 6 years ago

Any idea what the severity of this issue is, Jon?

	Paul Bone [:pbone]
	Comment 4 • 6 years ago

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?

	Paul Bone [:pbone]
	Comment 5 • 6 years ago

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

	Jon Coppeard (:jonco) Assignee
	Comment 6 • 6 years ago

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

	Jon Coppeard (:jonco) Assignee
	Comment 7 • 6 years ago

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

	Jon Coppeard (:jonco) Assignee
	Comment 8 • 6 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 9 • 6 years ago

Doesn't this test expose the issue? We don't normally check in a new test with a sec-high fix...

	Jon Coppeard (:jonco) Assignee
	Comment 10 • 6 years ago

Good point.  I'll land the test separately.

	Al Billings [:abillings - ex-MoCo]
	Comment 11 • 6 years ago

Sec-approval+ for the patch without a test. :-)

	Al Billings [:abillings - ex-MoCo]
	Comment 12 • 6 years ago

Comment on attachment 9024772 [details] [diff] [review]
bug1506640

We'll want patches nominated for affected branches too.

	Jon Coppeard (:jonco) Assignee
	Comment 13 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e459cfc0d2ade802b05fc2ed591bf3a5cb527f7

	Jon Coppeard (:jonco) Assignee
	Comment 14 • 6 years ago

Patch to add test.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/0e459cfc0d2a

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 6 years ago

Please nominate this for Beta/ESR60 approval when you get a chance.

	Jon Coppeard (:jonco) Assignee
	Comment 17 • 6 years ago

Updated patch without testcase.

	Jon Coppeard (:jonco) Assignee
	Comment 18 • 6 years ago

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

	Jon Coppeard (:jonco) Assignee
	Comment 19 • 6 years ago

[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

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago

Comment on attachment 9025877 [details] [diff] [review]
bug1506640-patch

[Triage Comment]
Approved for 64.0b11 and 60.4.0esr.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/8bbf80948b50

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/d4ce8b3cdee8