[This is a spin-off of the bug 354499. Since that is a restricted bug, I mark it as security-related.] The current implementation allows to change the default native iterator via changing Iterator.prototype.next. As such this is a regression since previously changing Iterator.prototype.next would not affect the meaning of for-in loops. Moreover, this exports the default iterator to script, which must not happen by the design. A trivial example demonstrating the problem: ~/m/files> cat ~/m/files/y.js Iterator.prototype.next = function() { throw "This should not be thrown"; } for (var i in []); ~/m/files> js ~/m/files/y.js uncaught exception: This should not be thrown

The patch changes js_CallIteratorNext to always call iter_next when enumerating which made cachedIterObj unnecessary. AFAICS the only problematic area with the patch is that __iterator__ that points to the the default Iterator is marked as enumerating. Thus after the patch a script that override __iterator__ to return a default iterator but with the custom next method would not see the method called. But without the patch this again would expose the enumerating iterator to scripts. This can be farther resolved with marking as enumerating only iterators for objects without __iterator__.

A minimal fix to address the leackage. The patch removes Object.prototype.__iterator__ so the default prototype-searching enumerating iterator is created only for objects without __iterator__. The patch does not remove cachedIterObj as that can wait.

This is smaller and preserves the desired Object.prototype iterator-getter. If we remove that default iterator-getter, people could still hack around in prototype objects along deeper chains than length 2, as well as in direct objects of course, to break for-in semantics. /be

Comment on attachment 240752 [details] [diff] [review] alterna-fix > static JSFunctionSpec generator_methods[] = { >- {js_iterator_str, iterator_self, 0,0,0}, >- {js_next_str, generator_next, 0,0,0}, >+ {js_iterator_str, iterator_self, 0,JSPROP_READONLY|JSPROP_PERMANENT,0}, >+ {js_next_str, generator_next, 0,JSPROP_READONLY|JSPROP_PERMANENT,0}, > {js_send_str, generator_send, 1,0,0}, > {js_throw_str, generator_throw, 1,0,0}, > {js_close_str, generator_close, 0,JSPROP_READONLY|JSPROP_PERMANENT,0}, I should have made send and throw immutable too -- please review assuming I did that. /be

(In reply to bug 354945 comment #4) > Is this sufficient to fix this bug? The test in comment 0 doesn't hack on any > prototype __iterator__. Is it necessary? The current code is broken not only because it leaks the enumerating iterator but also because it does improper optimization of using cached iterator that does not take into account that Iterator.next can change. Due to the latter one can not assume that the iterator is enumerating just by the fact that __iterator__ returns the instance of the default iterator. > For ES4/JS2 (with change of name to > use namespaces instead of __ugly__ names), we want Object.prototype to contain > the default iterator-getter. Why? I do not see what is wrong with the saying that if there is no property, the use the default as the price to pay for compatibility.

Comment on attachment 240752 [details] [diff] [review] alterna-fix This indeed addresses the problem. Still IMO it is cleaner to say that the only way to get prototype searching default iterator is via not defining __iterator__ at all then then to explain why after "obj.__iterator__ = function () { return new Iterator(this) }" "for (var in obj)" still searched the prototype while for (var in Iterator(obj)) does not.

(In reply to comment #7) > (From update of attachment 240752 [details] [diff] [review] [edit]) > This indeed addresses the problem. It also fits the nominal types used to implement these classes in the proposed ES4/JS2 spec. > Still IMO it is cleaner to say that the only > way to get prototype searching default iterator is via not defining > __iterator__ at all then then to explain why after "obj.__iterator__ = function > () { return new Iterator(this) }" "for (var in obj)" still searched the > prototype while for (var in Iterator(obj)) does not. Partly "cleaner" depends on fewer rules: if I have a DOM collection derived from nodelist and I set nodelist.prototype.__iterator__ = somefunction, I can change for-in behavior for all derived types. Why not likewise for Object.prototype? Partly it's a matter of taste that can trump minimizing rules. new Iterator vs. Iterator is a difference we can explain by referring to existing built-in classes, but you are right that it's a difference between two rules and one, so it takes back the savings of having __iterator__ be on Object.prototype in order to implement default iteration as enumeration. What I hope to do with the type system that's coming is make an Enumerator class do what new Iterator(obj) does in JS1.7, so the distinction is clearer. Then we would not need Iterator as a constructor at all. /be

Comment on attachment 240751 [details] [diff] [review] Fix v2 for real Hate to minus code removal but JS1.7 till now, and ES4/JS2 still, want Object.prototype.<some-name-for-the-iterator-getter>. /be

Checking this into the trunk now. /be

Here is another reason for Object.prototyoe without __iterator__. Consider the following question: Why "for (i in iteratorInstance)" does not search iteratorInstance and its prototype chain for properties but rather calls iteratorInstance.next despite the fact that iteratorInstance.__iterator__() gives the same result as Object.prototype.__iterator__()? The current answer is that instances of Iterator are special. If Object.prototyoe would not contain __iterator__, then the question becomes: Why "for (i in iteratorInstance)" does not search iteratorInstance and its prototype chain for properties? and the answer: because it defines __iterator__!

Fixed on trunk: Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.43; previous revision: 3.42 done Thanks to Igor for finding this. For both security and language soundness we should take the alterna-fix, v2 patch for 1.8.1. It is a safe fix that locks up native methods against being written by a property assignment operation, and against being replaced by deletion and recreation. /be

(In reply to comment #11) > Here is another reason for Object.prototyoe without __iterator__. Consider the > following question: > > Why "for (i in iteratorInstance)" does not search iteratorInstance and its > prototype chain for properties but rather calls iteratorInstance.next despite > the fact that iteratorInstance.__iterator__() gives the same result as > Object.prototype.__iterator__()? > > The current answer is that instances of Iterator are special. If > Object.prototyoe would not contain __iterator__, then the question becomes: > > Why "for (i in iteratorInstance)" does not search iteratorInstance and its > prototype chain for properties? > > and the answer: because it defines __iterator__! You're right, that Iterator is special is exactly the difference between enumeration and iteration now. Let me take back the r- I just put on the patch and think some more. /be

(In reply to comment #8) > > Still IMO it is cleaner to say that the only > > way to get prototype searching default iterator is via not defining > > __iterator__ at all then then to explain why after "obj.__iterator__ = function > > () { return new Iterator(this) }" "for (var in obj)" still searched the > > prototype while for (var in Iterator(obj)) does not. > > Partly "cleaner" depends on fewer rules: if I have a DOM collection derived > from nodelist and I set nodelist.prototype.__iterator__ = somefunction, I can > change for-in behavior for all derived types. Why not likewise for > Object.prototype? If Object.prototype.__iterator__ would not exist, one still can define Object.prototype.__iterator__ changing behavior for all built-in types. It would also allow to write: Object.prototype.__iterator__ = function() { return new Iterator(this); } to disable prototype lookup. Currently especially after making Iterator.protype.next read-only it would require much more complex code.

Comment on attachment 240758 [details] [diff] [review] alterna-fix, v2 Approved for RC2.

Comment on attachment 240751 [details] [diff] [review] Fix v2 for real Winning code removal, better language semantics as Igor as argued. /be

Igor's patch landed on trunk: $ cvs ci -m"Igor's v2 patch for 354750, r=me." jsiter.c jsobj.c Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.44; previous revision: 3.43 done Checking in jsobj.c; /cvsroot/mozilla/js/src/jsobj.c,v <-- jsobj.c new revision: 3.290; previous revision: 3.289 done /be

Alterna-fix, v2 patch landed on 1.8 branch: Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.17.2.18; previous revision: 3.17.2.17 done Not marking fixed1.8.1 yet pending approval of Igor's "Fix v2 for real" patch. /be

(In reply to comment #16) > Winning code removal, better language semantics as Igor as argued. Thanks. BTW, in Python __iter__ is exactly the method that the containers must provide to participate in the iteration protocol, http://docs.python.org/lib/typeiter.html and there is no default value for it.

(In reply to comment #19) > (In reply to comment #16) > > Winning code removal, better language semantics as Igor as argued. > > Thanks. BTW, in Python __iter__ is exactly the method that the containers must > provide to participate in the iteration protocol, > http://docs.python.org/lib/typeiter.html and there is no default value for it. I knew that but the lure of prototype default value hooked me. Would you please file a followup bug (or is there already one) on cleaning up cachedIterObj, etc.? Thanks, /be

I filed bug 354982 about iter cleanup.

Comment on attachment 240751 [details] [diff] [review] Fix v2 for real This doesn't need sr, but mrbkap is welcome to add a 2nd review. /be

Comment on attachment 240751 [details] [diff] [review] Fix v2 for real Sorry - didn't catch you wanted both. Approved for RC2.

Landed on the 1.8 branch: $ cvs ci -m"Igor's 'Fix v2 for real' patch for 354750, a=schrep." jsiter.c jsobj.c Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.17.2.19; previous revision: 3.17.2.18 done Checking in jsobj.c; /cvsroot/mozilla/js/src/jsobj.c,v <-- jsobj.c new revision: 3.208.2.35; previous revision: 3.208.2.34 done /be

This is js1.7, not needed in 1.8.0.8 right? If I've gotten that wrong please mark the bug blocking? or request approval?

This is just a bug, not a security problem, right? The damage should be limited to the user's own context and shouldn't be run at elevated privilege.

(In reply to comment #26) > This is just a bug, not a security problem, right? The damage should be limited > to the user's own context and shouldn't be run at elevated privilege. It is a security problem since the code assumes that the internal enumerating iterator that implements for-in is never exposed to scripts and do some optimizations based on that. Before the bug was fixed it was possible for a script to get access to that internal iterator and do the damage. For example, the second examples in bug 354499 uses that to expose a GC-hazard.

verified fixed 1.8 1.8 20061002 windows/linux 1.8 macppc

/cvsroot/mozilla/js/tests/js1_7/iterable/regress-354750-01.js,v <-- regress-354750-01.js