1386490 - Crash in js::WrapperMap::lookup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-082cfa92-4e83-4814-ba76-397460170801.
=============================================================
Top # of Nightly 20170730100307 on Windows, 9 reports from 3 installations.

Comment 1

[Ting-Yu Chou [:ting] (away)]

(In reply to Ting-Yu Chou [:ting] from comment #0)
> report bp-082cfa92-4e83-4814-ba76-397460170801.

In the dump, the first 8 bytes of Variant's rawData are 0 and it has tag 0x0, also the Value origv in js::RemapWrapper() [1] has data.asBits 0xfffe000000000000.

The first observation of this signature was Nightly 56.0a1 20170621102301.

[1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/js/src/proxy/CrossCompartmentWrapper.cpp#627

Comment 2

[Andrew McCreight [:mccr8]]

These crashes are mostly RemapWrapper getting called from ContentPrincipal::SetDomain().

I think this is the regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7a6baa6cca3292e8099e652b64d27e74df560874&tochange=e916ab827babb677ce5ab2cac0390c1401eaca0e

Unfortunately, I can't see anything obviously related to wrappers or principals in that regression range.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> data.asBits 0xfffe000000000000

That .... looks like an ObjectValue with a nullptr JSObject*, if I did my bit-twiddling right:

JSVAL_TYPE_OBJECT is 0xc
JSVAL_TAG_MAX_DOUBLE is 0x1FFF0

so JSVAL_TAG_OBJECT is 0x1FFFc and shifting by JSVAL_TAG_SHIFT (47) gives 0xfffe000000000000.  So we've got JSVAL_TAG_OBJECT and null.

OK, where does origv come from?  From here:

    JSObject* origTarget = Wrapper::wrappedObject(wobj);
    MOZ_ASSERT(origTarget);
    Value origv = ObjectValue(*origTarget);

so presumably Wrapper::wrappedObject(wobj) returned null... but why?

Comment 4

[YF (Yang)]

STR:
1. Install Greasemonkey and Tampermonkey.
2. Install https://greasyfork.org/scripts/13463-%E7%BD%91%E7%9B%98%E8%87%AA%E5%8A%A8%E5%A1%AB%E5%86%99%E5%AF%86%E7%A0%81-%E5%A2%9E%E5%BC%BA%E7%89%88 with Greasemonkey.
3. Open the http://blog.sina.com.cn/s/blog_c3383d160102wcq0.html.


Regression range (surmise):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=17920e3ba616e99089ec76e7c35c3dd31650caf3&tochange=072f8d4a9964129a06d774a5698f7f9f8128c66c

Bug 1355109 - Add IC for property reads on xrays, r=jandem,bz.

Comment 5

[YF (Yang)]

(In reply to YF (Yang) from comment #4)
> STR:
> 1. Install Greasemonkey and Tampermonkey.
> 2. Install
> https://greasyfork.org/scripts/13463-
> %E7%BD%91%E7%9B%98%E8%87%AA%E5%8A%A8%E5%A1%AB%E5%86%99%E5%AF%86%E7%A0%81-
> %E5%A2%9E%E5%BC%BA%E7%89%88 with Greasemonkey.
> 3. Open the http://blog.sina.com.cn/s/blog_c3383d160102wcq0.html.

STR 2:
1. Enable Tampermonkey and LastPass.
2. Install https://greasyfork.org/scripts/13463-%E7%BD%91%E7%9B%98%E8%87%AA%E5%8A%A8%E5%A1%AB%E5%86%99%E5%AF%86%E7%A0%81-%E5%A2%9E%E5%BC%BA%E7%89%88 with Tampermonkey.
3. Open the http://blog.sina.com.cn/s/blog_c3383d160102wcq0.html.

Comment 6

[Julien Cristau [:jcristau]]

tracking as new regression in 56.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Regression range (surmise):

What does "surmise" mean in this case?

We could consider turning off the ICs from bug 1355109 for 56 while we sort this out, if the regression range is reliable.

Comment 8

[Brian Hackett [Laid off!]]

I can reproduce this but don't know what's going on.  The problem doesn't seem to be with the ICs (disabling them doesn't fix anything) but with the new expando/wrapper handling code added by bug 1355109 to allow the ICs to work.

In a debug build this busts on this assert in JSObject::swap(), which I'm supposing leads to the later crash in opt builds:

// Ensure swap doesn't cause a finalizer to not be run.
MOZ_ASSERT(IsBackgroundFinalized(a->asTenured().getAllocKind()) ==
           IsBackgroundFinalized(b->asTenured().getAllocKind()));

|a| was created by XrayTraits::cloneExpandoChain(), and is a wrapper for the |dst| object there which an expando chain is being created for (this is code added by bug 1355109).  |b| originates from the rewrap() call in RemapWrapper().

Does anyone know what assurance we have that the assert above will actually hold?  It seems like JSCompartment::wrap() creates wrappers of all sorts of different allocation kinds (background vs. foreground finalized, some are in the nursery as well), and I don't see any checks around RemapWrapper related to this property.

Comment 9

[YF (Yang)]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7)
> > Regression range (surmise):
> 
> What does "surmise" mean in this case?
> 
> We could consider turning off the ICs from bug 1355109 for 56 while we sort
> this out, if the regression range is reliable.

I got a different regression range in the second investigate, the first range I think is unlikely. I have not found the exact evidence for the second range is reliable.

Comment 10

[Bobby Holley (:bholley)]

I don't have any answers to comment 8 offhand.

Comment 11

[Bobby Holley (:bholley)]

NI mccr8 in case he has cycles to dig for an answer.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7)
> We could consider turning off the ICs from bug 1355109 for 56 while we sort
> this out, if the regression range is reliable.

Boris, do you think this is the path we should take for 56?

Comment 13

[Liz Henry (:lizzard)]

This may be a sec issue from looking at some of the crash reports.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Boris, do you think this is the path we should take for 56?

Well, per comment 8 that doesn't help, so no.

We could consider backing out the XrayWrapper.cpp changes and disabling whatever JS bits rely on those changes, though, in parallel with working on fixing this the right way.  How feasible is it to get code changes like that landed before the 56 release at this point?

And yes, I would say this is a security issue...

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #14)
> We could consider backing out the XrayWrapper.cpp changes and disabling
> whatever JS bits rely on those changes, though, in parallel with working on
> fixing this the right way.  How feasible is it to get code changes like that
> landed before the 56 release at this point?

This is more of a question for Liz.

Comment 16

[Liz Henry (:lizzard)]

We can still take patches for 56 RC2, which i am hoping (but not certain) to be ready for going to build tomorrow. There are several unresolved release blocking issues and the release itself should be next Thursday, Sept. 28th.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

It is a sec issue - I had noticed a whole bunch of wildptr (perhaps UAF, but not poisoned) reads/writes, especially when looking at months of crashes.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Brian, are you able to put together a "turn it off" patch for 56?

I'm unfortunately not available tomorrow (Thursday, Sept 21), but will be back on Friday...

Comment 19

[Brian Hackett [Laid off!]]

I think the best thing to do is back out 072f8d4a9964 entirely.  I'll try to get a patch for that today but anyone with a copy of the tree could do so as well.

Comment 20

[Brian Hackett [Laid off!]]

Attachment: backout

072f8d4a9964 backs out cleanly on the FIREFOX_56b13_RELBRANCH head, here is a patch.

Comment 21

[Liz Henry (:lizzard)]

This is too late to get into 56 now. We could consider it as a dot release ridealong. But it would need sec approval before landing anywhere.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8910776 [details] [diff] [review]
backout

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily.

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? 56 and newer.

If not all supported branches, which bug introduced the flaw? Bug 1355109.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This _IS_ a branch backport.

How likely is this patch to cause regressions; how much testing does it need?  Should be fairly safe.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1355109
[User impact if declined]: Likely-expoitable crashes
[Is this code covered by automated tests?]: Not well enough
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  Yes.
[List of other uplifts needed for the feature/fix]: See comment 4.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Just backs out a performance fix until we ca get it right.
[String changes made/needed]: None.

Comment 23

[Liz Henry (:lizzard)]

jesup, dan, al, do you think this issue should block the 56 release?

We have done a few release candidate builds already, but that doesn't have to stop us from doing another. 

The risk is that we may slip the release date for 56.  I worry that may also risk the 57 release date. Of course, weighed against whether we think we might end up having to chemspill for this issue.

We can optionally plan to have this be a release driver or ridealong issue for 56.0.x. 

My gut feeling here is not to delay the release at the last moment for a (partial?) backout from a patch that landed in July.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Just for context, this is a probably-exploitable crash that's apparently not too difficult to trigger with a commonly installed addon (GreaseMonkey).  I'm pretty sure it can manifest in various forms, so there's a good chance that there is more incidence of it than is mentioned in this bug.

The backout here is a full backout, not a partial one, to minimize risk.

I can't speak to the schedule risks or the exact exploitability risk; I don't have that context.

Comment 25

[Daniel Veditz [:dveditz]]

It's more than just Greasemonkey looking at the crash correlations (which makes sense given the nature of the problem). Likely any add-on or web extension that manipulates content in similar ways. Lastpass was another I saw show up a lot.

Crash volume seems like it's edging into stability-problem territory. Most of the crashes (84%) don't look exploitable (crash accessing 0x0, 0x8, or 0xffffffffffffffff) but the rest are clearly lost in the wild blue yonder.

We should take a clean back-out.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

I mostly defer to bz, but note a "probably-exploitable crash" that's easy to trigger on many installations is more concerning than an average generic UAF crash.  and the volume is concerning as well.  I'd say take a clean backout.

Comment 27

[Liz Henry (:lizzard)]

OK. Thanks y'all. Once we have sec approval let's go ahead and land this on m-c. (Then uplift).

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 8910776 [details] [diff] [review]
backout

sec-approval=dveditz

remember to reopen the bug this is backing out.

Comment 29

[Liz Henry (:lizzard)]

Ryan, if you can land this quickly today across all channels I will be ready to start another build on m-r once the tests look good.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backout landed on m-r.
https://hg.mozilla.org/releases/mozilla-release/rev/58e45fc51d63

Unfortunately, the patch doesn't apply cleanly to 57/58, so we'll either need a rebased backout patch or a fix for the underlying problem instead.

Comment 31

[Liz Henry (:lizzard)]

Comment on attachment 8910776 [details] [diff] [review]
backout

Taking this for 56 after irc and email discussion.

Comment 32

[Brian Hackett [Laid off!]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)
> Backout landed on m-r.
> https://hg.mozilla.org/releases/mozilla-release/rev/58e45fc51d63
> 
> Unfortunately, the patch doesn't apply cleanly to 57/58, so we'll either
> need a rebased backout patch or a fix for the underlying problem instead.

Does hg backout work on 57/58?  It is able to backout 072f8d4a9964 on m-i tip without any conflicts.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Ah yes, looks like that works with some fuzz. Probably just |hg import| being overly-strict. NI myself to get it landed in 57/58.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out from m-c and m-b as well. Fix will go out in 57b4 and the 3pm PT trunk nightlies.

https://hg.mozilla.org/mozilla-central/rev/39aaf54972cb
https://hg.mozilla.org/releases/mozilla-beta/rev/520e300d1266

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Re-landed on mozilla-release at the request of Release Management in order for the backout to get a bit more bake time on Nightly and Beta first (since at this point, 56 RC will basically not be getting any further testing on Beta now that we're shipping 57 there). It's still on the radar as a 56 dot release ride-along candidate, however.

https://hg.mozilla.org/releases/mozilla-release/rev/8fbf05f4b921

Comment 36

[Liz Henry (:lizzard)]

Keeping the blocking flag here so we will know to keep this in mind for a dot release.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Brian, I've been trying to reproduce (on 56, where the patch is still landed), and I can reproduce the test mentioned in comment 8 failing in an opt build not running under rr, but all other configurations I have (debug, debug + opt, either one under rr or not under rr, opt under rr) don't hit the assert.

I'll dig some more tomorrow, but the general answer to your question, I would think, is that Wrapper::finalizeInBackground should ensure that a wrapper and the thing it wraps are either both background-finalizable or both not-background-finalizable.  I would think that would make the assert you mention hold, as long as we swap between an object and its wrappers or between two wrappers for the same object.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Ah, I guess with remapwrapper we actually point the same wrapper to a different object?  That could be an issue if the two objects have different background-finalization states.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Oh, and I'm following the steps to reproduce from comment 5.

Comment 40

[Boris Zbarsky [:bzbarsky]]

OK, with my fix for bug 1403646 I no longer get the failing assert from comment 8.... (or rather in an opt build an equivalent test never tests false).  But I do still crash.

Comment 41

[Boris Zbarsky [:bzbarsky]]

So the most obvious reason we end up crashing is that we call into RemapWrapper with wobjArg being a dead object proxy.  So Wrapper::wrappedObject(wobj) returns null.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Analysis of what was actually going on here moved to bug 1403716.

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Note (can't see that other bug): this signature keeps rising

Comment 44

[Boris Zbarsky [:bzbarsky]]

Note that the actual fix is going to be in bug 1404107, by the way.

Comment 45

[Jason Orendorff [:jorendorff]]

OK, how do we address this in 56.0.1? I think our options are:

- do nothing;
- back out the ICs; or
- uplift the patches in bug 1404107 and bug 1396466 (just landed).

I favor backing out, unless we're planning to re-land the ICs in beta 57.

Plus, if we back out, I can just land the test case in bug 1384615 as a crashtest.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> I favor backing out, unless we're planning to re-land the ICs in beta 57.

We are so planning, though time is running out.  They're critical for web extension performance.

Comment 47

[Liz Henry (:lizzard)]

After further discussion with bz we are going to let this fix ride with 57.