714547 - Array passed from another domain behaves like Object

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[ivan]

In Firefox 10+, an Array passed from another domain (must be another domain, not just an iframe) behaves like an Object, breaking things like Array.prototype.concat and JSON.stringify.

See this test page:
http://ludios.net/browser_bugs/ff10_array_concat_bug.html

It should display:

[object Array] of length 2: ["one","two"]
[object Array] of length 2: ["one","two"]

but Firefox 10.0b2 and 11.0a2 (2012-01-01) display:

[object Array] of length 2: {"0":"one","1":"two"}
[object Array] of length 1: [{"0":"one","1":"two"}]

Comment 1

[Boris Zbarsky [:bzbarsky]]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ccea01542d0b&tochange=e0ae39a3298e

Luke, could the scope chain changes be responsible?

Nominating, since this seems like something we need to fix before ship...

Comment 2

[Boris Zbarsky [:bzbarsky]]

ccing luke too.

Comment 3

[Jeff Walden [:Waldo]]

Luke, we're getting a SecurityWrapper<CrossCompartmentWrapper> here.  Its objectClassIs implementation is this:

868	template <class Base>
869	bool
870	SecurityWrapper<Base>::objectClassIs(JSObject *obj, ESClassValue classValue, JSContext *cx)
871	{
872	    /* Let ProxyHandler say 'no'. */
873	    bool ret = ProxyHandler::objectClassIs(obj, classValue, cx);
874	    JS_ASSERT(!ret && !cx->isExceptionPending());
875	    return ret;
876	}

ProxyHandler::objectClassIs just says "no".

344	bool
345	ProxyHandler::objectClassIs(JSObject *proxy, ESClassValue classValue, JSContext *cx)
346	{
347	    JS_ASSERT(OperationInProgress(cx, proxy));
348	    return false;
349	}

What gives here?  Should SecurityWrapper be asking Base::objectClassIs, maybe?  I'm having difficulty understanding when it could ever be correct to call ProxyHandler::objectClassIs, honestly, with the semantics it implements.

Comment 4

[Luke Wagner [:luke]]

This is technically working by design: a SecurityWrapper is not supposed to expose information about cross-domain objects.  Actually, from discussions with mrbkap, I thought it wasn't supposed to be possible for vanilla JS objects to be exposed cross-domain (but rather a select few things like Window, Location, etc), though clearly I'm wrong.  Blake, care to weigh in here?

Comment 5

[Jeff Walden [:Waldo]]

They're cross-domain, but by both pages setting document.domain to a truncation ("ludios.net"), access is permitted.

(What happens if the two pages modify document.domain to the same value, exchange some object, then one modifies to a different string?  "'That's not my department', says Werner von Braun."  I think historically we then allowed access forever, since a modification by one could be mirrored by the other, but I don't know if we do that now, or if we consider that policy sane.)

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Luke Wagner [:luke] from comment #4)
> with mrbkap, I thought it wasn't supposed to be possible for vanilla JS
> objects to be exposed cross-domain (but rather a select few things like
> Window, Location, etc), though clearly I'm wrong.  Blake, care to weigh in
> here?

Yeah. Everything I've said has a giant asterisk next to it saying "document.domain makes most of this not apply!" We only realized how badly document.domain was broken at the end of compartments, so we hacked in a solution that mostly works.

(In reply to Jeff Walden (remove +bmo to email) from comment #5)
> (What happens if the two pages modify document.domain to the same value,
> exchange some object, then one modifies to a different string? "'That's not
> my department', says Werner von Braun."

Actually, it's my department!

Historically, security checks only happened when calling getters, when getting certain properties (such as __proto__ and .constructor) and when entering XPConnect (that is, calling C++ functions from JS). Therefore, if you had set document.domain on two documents, gotten some objects out of them and then reset document.domain, you'd have a bunch of objects that somewhat worked, but would occasionally throw exceptions depending on which properties were being accessed.

Right now, because we assume that you can't have a vanilla object cross-origin, some objects (such as arrays) will continue to work fine, but you won't be able to access DOM objects any more (since they no longer consider themselves to be same origin).

The plan going forward for this is that when we have compartment-per-global, we will be able to do brain transplants for all of the objects in the compartment that sets document.domain, properly revoking access to the old objects. That will also have the side-effect of fixing this bug, since we'll be using the right kind of security wrappers all the time.

Comment 7

[Luke Wagner [:luke]]

Hah, I almost added a "Is there some document.domain afoot here?".  Thanks for the explanation Blake.  Would a more immediate fix (than c-p-g) be for FilteringWrapper to override objectClassIs and then provide an exception for AccessCheck::documentDomainMakesSameOrigin?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Luke Wagner [:luke] from comment #7)
> Hah, I almost added a "Is there some document.domain afoot here?".  Thanks
> for the explanation Blake.  Would a more immediate fix (than c-p-g) be for
> FilteringWrapper to override objectClassIs and then provide an exception for
> AccessCheck::documentDomainMakesSameOrigin?

This would definitely fix this bug... A second question, though, is whether or not we're actually protecting ourselves from anything here. I tend to lean towards "no". I can't actually think of an attack where letting foreign code know that something is a native array (or date object, etc) would actually open up a security hole.

Comment 9

[Jeff Walden [:Waldo]]

Seems to me it's an information leak all the same.  Although, rather than say "no" when asked for security things, it seems to be the proper thing to do would be to just throw an exception.

Comment 10

[Luke Wagner [:luke]]

(In reply to Blake Kaplan (:mrbkap) from comment #8)
> I can't actually think of an attack where letting foreign
> code know that something is a native array (or date object, etc) would
> actually open up a security hole.

To your question: it seems like we go through great lengths to avoid exposing any cross-origin information, and I think "is this object an array/string/regexp/etc" qualifies as a few bits of information.

However, your question isn't the situation we have: our assumption is that these SecurityWrapper'd objects are only being exposed in document.domain situations (where it is  ok for them to be transparent).  So, for that reason, I'm happy to neuter SecurityWrapper as a quick fix.  But it seems like, when we tighten up the document.domain situation after c-p-g, we'll want the proposed fix in comment 7.

Comment 11

[Luke Wagner [:luke]]

Oops, missed comment 9: maybe I'm missing something, but saying 'no' (unconditionally) doesn't seem to leak any more information than throwing a security exception, but the latter does sound better.

Comment 12

[Jeff Walden [:Waldo]]

I was responding to "an attack where letting foreign code know that something is a native array (or date object, etc)", not to what we currently do.  Seems better to hold a line than to allow some things to work (but to what end, if immediately post-check we'd start looking up properties on it in [].concat and fail anyway?).

Comment 13

[Luke Wagner [:luke]]

(In reply to Jeff Walden (remove +bmo to email) from comment #12)
> Seems better to hold a line than to allow some things to work (but to
> what end, if immediately post-check we'd start looking up properties on it
> in [].concat and fail anyway?).

No, that should all go back to working the way it did before.  (Actually, this bug notwithstanding, FF10+ should work *better* with cross-origin wrappers b/c of bug 683361!)

Comment 14

[Luke Wagner [:luke]]

Attachment: as described

As discussed above, this patch just let nativeCall/objectClassIs pass through.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 587100 [details] [diff] [review]
as described

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

::: js/src/jswrapper.cpp
@@ +860,5 @@
>                                    CallArgs args)
>  {
> +    /*
> +     * Let this through until compartment-per-global let's us have stronger
> +     * invariants wrt document.domain (bug 714547).

Either this or actually checking document.domain work for me.

Nit: let's -> lets.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Could you file a followup depending on the cpg bug, so we don't lose track?

Comment 17

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81cb240e14c4

(In reply to Ms2ger from comment #16)
> Could you file a followup depending on the cpg bug, so we don't lose track?

Blake: is there already a "fix document.domain" bug?  This is really just a small addition to that general task.

Comment 18

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/81cb240e14c4

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Luke Wagner [:luke] from comment #17)
> Blake: is there already a "fix document.domain" bug?  This is really just a
> small addition to that general task.

bug 655649 is that bug.

Comment 20

[Alex Keybl [:akeybl]]

Is this still a major concern for Firefox 10 (web/add-on compatibility)? If so, please nominate for aurora/beta approval ASAP. We're cutting our second to last beta today.

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 587100 [details] [diff] [review]
as described

[Approval Request Comment]
Regression caused by (bug #): 690825
User impact if declined: Two sites seem to have been broken so far.  The fix is on aurora, so it would be better to wait 6 weeks instead of 12.
Testing completed (on m-c, etc.): on aurora
Risk to taking this patch (and alternatives if risky): low, on aurora

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 587100 [details] [diff] [review]
as described

[Triage Comment]
Breaking websites - let's get this in for our second Beta on Tuesday. Approved.

Comment 24

[Alex Keybl [:akeybl]]

Minusing for tracking-esr10. It's affected, but we will not fix minor functional regressions unless they affect enterprise users significantly.

Comment 25

[j.j.]

> Minusing for tracking-esr10. It's affected, but we will not fix minor
> functional regressions unless they affect enterprise users significantly.

How will yo know whether it affects enterprise users if you don't track it?
[this kind of decisions pushed me away from this project, among other related things]

Comment 26

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-beta/rev/73f897fa60a5

Comment 27

[Jeff Walden [:Waldo]]

(In reply to Alex Keybl [:akeybl] from comment #24)
> we will not fix minor functional regressions unless they affect enterprise users
> significantly.

I'm not entirely convinced this is the right decision.

Array.isArray was added to the spec because it addresses a real problem authors face: that instanceof is insufficient for testing the kind ("class") of an object.  We've supported and advocated its use for a year and a half now, particularly for the very case at issue here and in duplicate bugs.

I don't think I can call this a "minor functional regression".  That description is arguably accurate for this bug's symptoms outside of Array.isArray.  But Array.isArray not working in this way means that API is fundamentally broken.  Its *entire purpose* is to answer that question correctly; this mistake isn't an edge case.  If it's not answering the question correctly in a fairly basic scenario, that's not a minor functional regression.  It means the API is unreliable, untrustworthy, and potentially even unusable.

I think we should reconsider this decision.

Comment 28

[Alex Keybl [:akeybl]]

(In reply to j.j. (inactive in 2012) from comment #25)
> > Minusing for tracking-esr10. It's affected, but we will not fix minor
> > functional regressions unless they affect enterprise users significantly.
> 
> How will yo know whether it affects enterprise users if you don't track it?

We don't track issues until it's clear the issue meets our ESR landing criteria - when that occurs, a user/developer can set the tracking-firefox-esr10 flag to ?. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

When I commented previously, it wasn't clear we needed to take a fix. Given the site pain we're now seeing in 723388, we've reconsidered. Tracking for the ESR release shipped lock-step with FF11.

Luke - please land on mozilla-esr10 as soon as possible. Thanks!

Comment 29

[Luke Wagner [:luke]]

Alex: mozilla-esr10 seems closed and all patches have CLOSED TREE.  I tried landing with CLOSE D TREE (per comment 28) but got some error:

  remote: Unrecognized tree!  I don't know how to check closed status for mozilla-esr10.

Any hints?

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Luke Wagner [:luke] from comment #29)
> Alex: mozilla-esr10 seems closed and all patches have CLOSED TREE.  I tried
> landing with CLOSE D TREE (per comment 28) but got some error:
> 
>   remote: Unrecognized tree!  I don't know how to check closed status for
> mozilla-esr10.
> 
> Any hints?

the hook was not updated when it was originally applied to this repo (bug 722310) and it is now being updated so you should be able to try again in ~5 mins.

Comment 31

[Luke Wagner [:luke]]

Great, thanks
https://hg.mozilla.org/releases/mozilla-esr10/rev/672934cac449

Comment 32

[Phil Ringnalda (:philor)]

Sadly, down there on the releasing edge, you need to hg up -r default to be sure you aren't on whatever relbranch was pushed to last - you pushed to the Thunderbird/SeaMonkey relbranch.

Comment 33

[Luke Wagner [:luke]]

Good catch philor!
https://hg.mozilla.org/releases/mozilla-esr10/rev/771afd1e6247

Comment 34

[Paul Silaghi, QA [:pauly]]

This is verified fixed on FF 11b3:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0

But still an issue on FF 10.0.2 ESR:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Mozilla/5.0 (X11; Linux i686; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0.2) Gecko/20100101 Firefox/10.0.2

Comment 35

[Luke Wagner [:luke]]

Does FF 10.0.2 ESR contain https://hg.mozilla.org/releases/mozilla-esr10/rev/771afd1e6247?

Comment 36

[Matt Brubeck (:mbrubeck)]

(In reply to Luke Wagner [:luke] from comment #35)
> Does FF 10.0.2 ESR contain
> https://hg.mozilla.org/releases/mozilla-esr10/rev/771afd1e6247?

No, it does not:
https://hg.mozilla.org/releases/mozilla-esr10/file/FIREFOX_10_0_2esr_RELEASE/js/src/jswrapper.cpp#l864

It looks like 10.0.2 ESR was built from GECKO1001_2012020805_RELBRANCH:
https://hg.mozilla.org/releases/mozilla-esr10/rev/14af4fac0bc5

I'm not sure why this happened, since that was the 10.0.1 ESR relbranch.  This means 10.0.2 ESR missed this patch which landed on the default branch rather than the relbranch.

Comment 37

[Luke Wagner [:luke]]

Will there be a 10.0.3 ESR and will this patch be on it?

Comment 38

[Matt Brubeck (:mbrubeck)]

(In reply to Luke Wagner [:luke] from comment #37)
> Will there be a 10.0.3 ESR and will this patch be on it?

Yes and yes.

Comment 39

[Daniel Veditz [:dveditz]]

10.0.2 was a chemspill, so it was built from the previous _relbranch to keep the release focused and testing limited. Future ESR releases will be from the mainline and pick up this fix. Currently this is scheduled to ship in "whatever 10.x ESR matches Firefox 11". We expect that to be 10.0.3 unless there's another chemspill release in the meantime.

Comment 40

[u279076]

(In reply to Daniel Veditz [:dveditz] from comment #39)
> Currently this is scheduled to ship in
> "whatever 10.x ESR matches Firefox 11". We expect that to be 10.0.3 unless
> there's another chemspill release in the meantime.

Not to quibble over version numbers but I would expect the Firefox 11 ESR to be 10.1 -- right?

Comment 41

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #40)
> (In reply to Daniel Veditz [:dveditz] from comment #39)
> > Currently this is scheduled to ship in
> > "whatever 10.x ESR matches Firefox 11". We expect that to be 10.0.3 unless
> > there's another chemspill release in the meantime.
> 
> Not to quibble over version numbers but I would expect the Firefox 11 ESR to
> be 10.1 -- right?

Nope, we do 10.0.x until we move to 17 then we do 17.0.x, etc (see https://wiki.mozilla.org/Enterprise/Firefox/ExtendedSupport:Proposal)

Comment 42

[Paul Silaghi, QA [:pauly]]

Actual results:
[object Array] of length 2: ["one","two"]
[object Array] of length 2: ["one","two"]

Verified fixed on FF 10.0.3 ESR on Win 7, Ubuntu 11.10 and Mac OS X 10.6.8