394815 - Still possible to create __proto__ cycles with XOW

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

This crashes Firefox due to too much recursion:

<script>
var z = {};
window.__proto__ = z;
z.__proto__ = window;
for (var i in window) { }
</script>

The third line of the script should throw a "cyclic __proto__ value" error.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Brendan, I don't see any way to fix this without some sort of help from the JS engine. Furthermore, I don't see a way of fixing this that doesn't involve exposing the wrappers (through some sort of hook) to the engine.

Comment 2

[Brendan Eich [:brendan]]

Let's forbid setting __proto__ to anything but null, at least for XOWs. Good?

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 281027 [details] [diff] [review]
Proposed fix

Would it be better to do this before the wrappedObj's proto is changed, and fail for non-null newProto values? I.e. make XOWs stricter by not letting the wrapped object have a different (non-null) proto from the one set for the wrapper?

r+sr=me in case this does not make sense.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I wanted to do that, but I don't know ahead of time if the wrapped object's prototype is Object.prototype, so setting xow.__proto__ could simply be setting a property named __proto__ (if e.g., the proto chain had been severed earlier).

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on trunk.

Comment 7

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
> Let's forbid setting __proto__ to anything but null, at least for XOWs. Good?
> 

does __proto__ have legitimate usage?
it is not standard afaict.

Comment 8

[Jeff Walden [:Waldo]]

__proto__ is moderately well-supported by the non-IE browsers, I believe; I don't have access to any at the moment to check for certain.