Cross-frame for-in is broken on Firefox 37, 38 beta

RESOLVED FIXED in Firefox 38

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Mark S. Miller, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {dev-doc-complete, regression, site-compat})

37 Branch
mozilla40
x86_64
Mac OS X
dev-doc-complete, regression, site-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38+ fixed, firefox38.0.5 fixed, firefox39+ fixed, firefox40+ fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Originally reported by Kevin Reid at https://code.google.com/p/google-caja/issues/detail?id=1962

Test case:


<iframe id="tmf" src="about:blank"></iframe>
<script>
var frame = document.getElementById('tmf');
var frwin = frame.contentWindow;

var iteratorSym = frwin.Symbol.iterator;
var arrayIter = (new frwin.Array())[iteratorSym]();
var ArrayIteratorPrototype = Object.getPrototypeOf(arrayIter);
var arrayIterProtoBase = Object.getPrototypeOf(ArrayIteratorPrototype);
var IteratorPrototype = arrayIterProtoBase;
console.log(IteratorPrototype.next);
delete IteratorPrototype.next;
console.log(IteratorPrototype.next);

var tam = frwin.eval('({})');
console.log('(1) About to fail');
for (var x in tam) { console.log('(1)', x); }
console.log('(1) Didn\'t fail');
</script>
(Assignee)

Updated

3 years ago
Blocks: 694100
(Assignee)

Comment 1

3 years ago
for .. in uses Iterators in ES6 and you just broke the iterator object that we use for this. I am not sure our prototype set-up is completely correct, but maybe you shouldn't be doing this.
(Reporter)

Comment 2

3 years ago
The ES6 spec requires for/of to use the iteration protocol. It requires for/in not to. By having for/in rely on the iteration protocol, you violate the spec and deviate from the behavior of all other browsers.

This resembles a pattern of bug we had in some early attempts at Function.prototype.bind, where some implementations made use of Function.prototype.apply and broke if it was deleted.

https://code.google.com/p/v8/issues/detail?id=892
https://code.google.com/p/v8/issues/detail?id=828

For both of these bugs, the problem is not just breakage on deletion. If an alternate .next or .apply is provided, does this affect what for/in enumerates? The spec says it must not, and it does not on all other browsers.
(Reporter)

Comment 4

3 years ago
> maybe you shouldn't be doing this

A general reason such edge cases must work reliably:
http://wiki.ecmascript.org/doku.php?id=conventions:safe_meta_programming

The .bind bug in particular broke this.


The specific reason why we SES currently deletes .next, from
https://code.google.com/p/google-caja/wiki/SecurityAdvisory20150313

> Caja's taming of JavaScript is based on a whitelist, which lists 
> all the objects, properties, and methods of the JavaScript API 
> that we decide are safe to expose. This includes all of the ES5 
> API and some of the ES6 API as they get deployed and we deem 
> them safe. Until ES6 deployments are more mature, Caja's focus 
> for now remains to provide a safe subset of ES5.
> 
> We had assumed that elements of ES6 we omit from our whitelist 
> were also elements whose taming we could postpone reasoning about.


By deleting .next, we better approximate an ES5 environment that we already know how to tame. Although off the top of my head I expect .nest is safe, we won't whitelist .next until we're sure that it does not open up any exploits. See the rest of that security advisory for examples of the kinds of trouble we might need to worry about.
(Reporter)

Comment 5

3 years ago
Oh. Apologies. It looks likely that I am wrong, that ES6 broke ES5 code (like SES) in this regard, and that Firefox is the only browser that conforms. Cc'ing Allen.

Allen, is this breaking change from ES5 intentional? Sorry I missed it before it was too late. :(
(Assignee)

Comment 6

3 years ago
We can probably make it so that we always create a new object with a next method internally, so Caja wouldn't be able to mess with it.
(Reporter)

Comment 7

3 years ago
Hi Tom, thanks for suggesting that. But given the https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-enumerate surprise, I'm not sure which way would be correct. If Firefox's current behavior is indeed correct by ES6, we should leave it as is and Caja should adapt.

Apologies again for my ignorance of this spec change.

Allen, which treatment of .next is correct according to the spec?
Flags: needinfo?(allen)
Flags: needinfo?(allen.wirfsbrock)

Comment 8

3 years ago
The suggestion in comment 6 would be in conformance with the ES6 spec.  The spec. doesn't require that all objects returned from [[Enumerate]] share a common 'next' method, just that they all inherit from %IteratorPrototype%.

For ES2016 we might want to strengthen  the spec. to make to it an requirement that, for ordinary objects,  the object returned by [[Enumberate]] has 'next'  as an own property.

In the meantime, Mark,  you can evangelize that implementation approach.
Flags: needinfo?(allen)
Flags: needinfo?(allen.wirfsbrock)
(Reporter)

Comment 9

3 years ago
Allen, ignoring Caja and SES, if you were going to remove this ambiguity in the spec for ES2016, which way would you rather resolve it?

Comment 10

3 years ago
(In reply to Mark S. Miller from comment #9)
> Allen, ignoring Caja and SES, if you were going to remove this ambiguity in
> the spec for ES2016, which way would you rather resolve it?

I'd probably go with the own property.  But note that the result of for-in's application of [[Enumerate]] to an ordinary object is not directly observable.  So there is no actual requirement that an iterator object gets created or that a 'next' method actually exists. This could all be special cased and inlined  by an implementation.
(Reporter)

Comment 11

3 years ago
Good. Let's process with an own property as Tom suggested.
(Reporter)

Comment 12

3 years ago
Meant "proceed"
(Reporter)

Comment 13

3 years ago
Something that remains unexplained from what is said in this bugzilla thread. In Kevin's comment #4 on the original Caja bug thread https://code.google.com/p/google-caja/issues/detail?id=1962#c4 , he writes:

> (Oh, also note that it only occurs cross-frame; for-in 
> does not throw when evaluated in the frame which has 
> had the property deleted.)

Why would that be?

Comment 14

3 years ago
(In reply to Mark S. Miller from comment #13)
> Something that remains unexplained from what is said in this bugzilla
> thread. In Kevin's comment #4 on the original Caja bug thread
> https://code.google.com/p/google-caja/issues/detail?id=1962#c4 , he writes:
> 
> > (Oh, also note that it only occurs cross-frame; for-in 
> > does not throw when evaluated in the frame which has 
> > had the property deleted.)
> 
> Why would that be?

Note sure.  Of course, since none of this is specified it's really implementation specific behavior.

One thing I notices in the test case was this:
> var iteratorSym = frwin.Symbol.iterator;

I don't know why Symbol was qualified by frwin as Symbol.iterator is not supposed to be a realm specific value.  I wonder if somebody's implementation might have a bug in that regard??
> I don't know why Symbol was qualified by frwin

Doesn't matter.  Using Symbol.iterator there has the same effect.

> Why would that be?

Quite simple, for some values of simple.  js::IteratorMore, which is what's invoked for the for-in loop step, has this bit:

1271     // Fast path for native iterators.
1272     if (iterobj->is<PropertyIteratorObject>()) {

which then directly invokes NativeIteratorNext.  In other words, it's directly poking at the iterator guts instead of doing a [[Get]] for "next" on it.

In the cross-global case, iterobj is not a PropertyIteratorObject; instead it's a  proxy with a js::CrossCompartmentWrapper handler.  In other words, one of the proxies we use for security membranes.  So the fast path is not taken and we end up getting .next and so forth.

Tom, is there a reason we can't just detect this case and still take the fast path (CheckedUnwrap, enter that compartment, NativeIteratorNext, and then wrapValue back into the original compartment)?
Flags: needinfo?(evilpies)
Oh, and document that it's not just a fast path but a correctness path?
(Assignee)

Comment 17

3 years ago
That seems more of a short term fix, maybe for Aurora or Beta? Otherwise I would prefer to do what I suggested.
Flags: needinfo?(evilpies)
[Tracking Requested - why for this release]:  Web compat regression

Hmm.  I guess this is a regression?  Given that, fixing this on Aurora and Beta might in fact be worth it...
status-firefox38: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
tracking-firefox38: --- → ?
tracking-firefox39: --- → ?
tracking-firefox40: --- → ?
(Reporter)

Comment 19

3 years ago
(In reply to Allen Wirfs-Brock from comment #8)
> The suggestion in comment 6 would be in conformance with the ES6 spec.  The
> spec. doesn't require that all objects returned from [[Enumerate]] share a
> common 'next' method, just that they all inherit from %IteratorPrototype%.
> 
> For ES2016 we might want to strengthen  the spec. to make to it an
> requirement that, for ordinary objects,  the object returned by
> [[Enumberate]] has 'next'  as an own property.
> 
> In the meantime, Mark,  you can evangelize that implementation approach.

I notice at http://people.mozilla.org/~jorendorff/figure-2.png that %IteratorPrototype% does not have a .next method. In that case, I don't see how FF's current cross-frame behavior can be interpreted as conforming to the spec. Deleting an unspeced property should not be able to cause a violation of specced behavior. Opinions?

Since FF plans to fix this anyway and we'll be evangelizing clearer spec language in ES7 anyway, this question may seem academic. But the normative question at stake determines whether we can validly add a test262 test for this issue. More immediately for me, it determines how I document the repair needed to SES (and thus Caja) so that it works on the current FF betas in the interim.

Comment 20

3 years ago
(In reply to Mark S. Miller from comment #19)
> (In reply to Allen Wirfs-Brock from comment #8)
> 
> I notice at http://people.mozilla.org/~jorendorff/figure-2.png that
> %IteratorPrototype% does not have a .next method. In that case, I don't see
> how FF's current cross-frame behavior can be interpreted as conforming to
> the spec. Deleting an unspeced property should not be able to cause a
> violation of specced behavior. Opinions?

Figure 2 is not normative, so you can't really justify anything from it.  Also, it is primarily about generators and only shows the object structure that is actually specified (normatively) in the proses+algorithms of the spec.  It doesn't say that a valid Iterator has to fit into that structure.

In particular, when the spec. says that all objects returned fom [Enumerate]] inherit from %IteratorPrototupe% that doesn't means that they must /directly/ inherit from it.  They are perfectly free (according to the current spec language) to have an implementation defined intermediate prototype that provides the 'next' property.
So...  Given that Reflect.enumerate directly exposes [[Enumerate]], we have the problem either way in spec terms if the .next is not required to live directly on the object returned by [[Enumerate]] (or on some prototype not shared by any other objects).  In particular, you could Reflect.enumerate() and then delete .next from everything on the resulting proto chain....

But yes, I mostly agree with Mark: having a .next on IteratorPrototype at all seems like a bug to me.
(Assignee)

Comment 22

3 years ago
Created attachment 8591844 [details] [diff] [review]
v1 Redefine next() as an own property

Here this is my idea that I already described.
(Assignee)

Updated

3 years ago
Assignee: nobody → evilpies
(Assignee)

Updated

3 years ago
Attachment #8591844 - Flags: review?(jwalden+bmo)
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

bz, is this something we should expect to uplift to 38?  What is the impact of this issue on users? 

Florin, can you check to see if this is a regression from before 37? It might be useful to know if this has always been an issue or if it regressed within the last year or so. Thanks.
tracking-firefox38: ? → +
tracking-firefox39: ? → +
tracking-firefox40: ? → +
> bz, is this something we should expect to uplift to 38?

I think so, yes.

> What is the impact of this issue on users? 

Some websites using SES may be broken, if I understand this correctly.

> Florin, can you check to see if this is a regression from before 37?

This particular testcase using Symbol will only break on 36 and later, I'd think, since we only enabled Symbol in 36.

The testcase likely needs to be modified to test things properly on 35 and earlier.
(Reporter)

Comment 25

3 years ago
> Some websites using SES may be broken, if I understand this correctly.

Not to worry. 
* It does not affect any currently-deployed-at-scale customers that we are aware of.
* On those customers it does affect, it fails safe rather than causing a vulnerability.
* https://codereview.appspot.com/222570043/ is already LGTMed and will be submitted as soon as I add tests, so coming customer dependencies on this bug will become a non-issue anyway.

So, yes, please fix this of course. But no rush as far as we're concerned, and no need for heroic efforts to uplift to earlier releases.
> * On those customers it does affect, it fails safe rather than causing a vulnerability.

Just so we're on the same page, "fails safe" means "site does not work", yes?  As in, the _site_ is not exploitable, but the user can't use the site?

I don't think it would take anything particularly heroic to uplift this to 38/39.
(Reporter)

Comment 27

3 years ago
(In reply to Not doing reviews right now from comment #26)
> > * On those customers it does affect, it fails safe rather than causing a vulnerability.
> 
> Just so we're on the same page, "fails safe" means "site does not work",
> yes?  As in, the _site_ is not exploitable, but the user can't use the site?

Yes, until they upgrade to https://codereview.appspot.com/222570043/


> I don't think it would take anything particularly heroic to uplift this to
> 38/39.

That would of course be great, thanks! I posted my previous "No worries" note in the interest of not crying wolf. When we have needed heroic efforts, you folks have been awesome! I want to be careful not to abuse that. When/if we do need heroic efforts again, we'll let you know ;).
Comment on attachment 8591844 [details] [diff] [review]
v1 Redefine next() as an own property

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

I'm dubious about perf, but if it flies, great.

::: js/src/jsiter.cpp
@@ +511,5 @@
> +            return nullptr;
> +
> +        RootedValue value(cx, ObjectValue(*next));
> +        if (!DefineProperty(cx, res, cx->names().next, value))
> +            return nullptr;

So this seems fine and all, but I'm a little dubious about it being fast enough.  If it's not, we'll probably have to do something similar to what we do for String objects and (maybe formerly?) RegExp objects, in terms of creating them with a "next" property built in, set to the right function by direct slot-set.
Attachment #8591844 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 30

3 years ago
Comment on attachment 8591844 [details] [diff] [review]
v1 Redefine next() as an own property

Cross-cross compartment for this case is probably not important enough to really worry about performance and nothing changes in the same compartment case.

Approval Request Comment
[Feature/regressing bug #]: bug 783829
[User impact if declined]: Some websites using Caja/SES might be broken for a while.
[Describe test coverage new/current, TreeHerder]: Added a jit-test for this specific case. I however would like to wait a few days with the patch in Nightly.
[Risks and why]: I don't see any big risks, this fix it pretty targeted.
[String/UUID change made/needed]: none
Attachment #8591844 - Flags: approval-mozilla-beta?
Attachment #8591844 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/fbc227fa2f19
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8591844 [details] [diff] [review]
v1 Redefine next() as an own property

Taking this patch in aurora to improve the coverage.
Attachment #8591844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab9325734d9f
status-firefox39: affected → fixed
Flags: in-testsuite+
Comment on attachment 8591844 [details] [diff] [review]
v1 Redefine next() as an own property

[Triage Comment]
No regression noticed yet, let's take it in 38 beta 7. This will give us time to fix potential regressions.
Attachment #8591844 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-release/rev/d20a4e36e508

https://treeherder.mozilla.org/logviewer.html#?job_id=55079&repo=mozilla-release

Looks like it needs rebasing around bug 1140586.
status-firefox38: fixed → affected
Flags: needinfo?(evilpies)
(Assignee)

Comment 37

3 years ago
Created attachment 8597352 [details] [diff] [review]
Patch for Beta

Successfully built locally.
Flags: needinfo?(evilpies)
status-firefox38.0.5: --- → fixed

Updated

3 years ago
Keywords: dev-doc-needed, regression, site-compat
Posted the site compatibility doc for record: https://www.fxsitecompat.com/en-US/docs/2015/cross-frame-for-in-loop-is-not-working/
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.