Re: [webkit-dev] ExecState::thisObject()
That's correct. Other browser's get this case right. Here are a couple test cases you might find interesting: http://webblaze.org/abarth/tests/protoconfused/test1.html http://webblaze.org/abarth/tests/protoconfused/test2.html I tried these tests, with mixed results: IE8: Exception thrown during load. Firefox 3.0: mixture of passes and fails on test1.html. Exception thrown during load of test2.html. Chrome 2.0: Mixture of passes and fails. Yes. All the browsers suck on these tests. :) Would you like me to go look for more exploitable cases? It seems like the only reason not to fix this issue is because we're afraid of code churn. I'm just trying to clarify the issue. Based on your input, I started out thinking that there was a spec mandating this behavior, that other browsers followed the spec, and that failure to follow the spec was a security hole. Now I see that there is no spec, there is no clear shared behavior in other browsers, and the security holes we know about only pertain to cross-origin objects, which would specifically be excluded from this change. So, the motivation for this change is simply that it would establish a new, more logical model for cross-frame property access. That's a laudable goal. I hope we can make it happen. Since we're inventing the model, we have some freedom to do what we think is simplest and/or most efficient and/or most secure in certain edge cases. Also, once we've established the model, we'll need to propose it to some standards body -- probably HTML5. Also, we're free to fix the easy stuff now and the hard stuff later, since leaving some rough edges unfinished will not be a security problem. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Tue, Jul 14, 2009 at 12:31 PM, Geoffrey Garengga...@apple.com wrote: Also, once we've established the model, we'll need to propose it to some standards body -- probably HTML5. I believe the correct spec to describe this behavior is WebIDL, which controls how the abstract DOM interfaces are realized in ECMAScript. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Also, once we've established the model, we'll need to propose it to some standards body -- probably HTML5. I believe the correct spec to describe this behavior is WebIDL, which controls how the abstract DOM interfaces are realized in ECMAScript. Sounds good. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
I've made some initial stabs: https://bugs.webkit.org/show_bug.cgi?id=27276 The bindings need a bunch more cleanup before we can do much more than that. I've started filing bugs about the cleanup. On Tue, Jul 14, 2009 at 12:41 PM, Geoffrey Garengga...@apple.com wrote: Also, once we've established the model, we'll need to propose it to some standards body -- probably HTML5. I believe the correct spec to describe this behavior is WebIDL, which controls how the abstract DOM interfaces are realized in ECMAScript. Sounds good. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
1. Is it correct for the ExecState to carry the thisValue? As Adam realized later (I think), ExecState carries the value for this inside the calling function. It does not carry the object whose property is being accessed. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Re-sending from correct address. On Mon, Jul 13, 2009 at 1:23 PM, Eric Seidelesei...@google.com wrote: I'm looking at this more today. I'm first fixing JSCell::new subclasses to make sure they're always allocating in the correct heap. If we're to map from objects to the associated globalobject via the heap, we need to fix allocation first. I started by just removing all versions of new (and jsNumberCell, etc.) which used ExecState instead of JSGlobalData* I'm wondering if I should instead be changing ExecState to carry a current global object member (the global object which carries the heap that objects should be allocated in, and prototypes should be looked up from. This is different from either the lexical or global objects). That would require fixing many callsites, but probably fewer than my current approach. -eric On Fri, Jul 10, 2009 at 11:08 PM, Adam Barthaba...@webkit.org wrote: On Fri, Jul 10, 2009 at 6:29 PM, Maciej Stachowiakm...@apple.com wrote: On Jul 10, 2009, at 6:10 PM, Adam Barth wrote: That's correct. Other browser's get this case right. Here are a couple test cases you might find interesting: http://webblaze.org/abarth/tests/protoconfused/test1.html http://webblaze.org/abarth/tests/protoconfused/test2.html The question is how to compute the correct wrapper context in all cases. There are a bunch of approaches that cover 80% of the cases. The trick is finding an approach that works for 100% of the causes. Well, for DOM Nodes you can almost always chase backpointers all the way up to the Document and from there to the Window, but this could be inefficient. And there's objects in the DOM that are not Nodes at all, and can't readily reach a Node. We tried this approach for a while, but we came across CSSValue, which has no obvious back pointers. It's a bit unclear how to do this for all Nodes even. For example, DocType Nodes might not have an associated Document. It's possible we could change WebCore to have all the appropriate back pointers. However, it might be easier to have all the wrapper objects hold the global object directly (or indirectly via their heap placement). I will ask Sam and Geoff for their thoughts in person if they don't chime in on the list. Thanks. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Our current thinking is to add a parameter to toJS() to receive the JSGlobalObject in which to create the wrapper. Seems logical. Once we do that, the question is how to find the proper JSGlobalObject at each call site. In most cases, you have another JSObject sitting around that is from the right JSGlobalObject. In the document.body example, the getter for body has the JSDocument from which it gets the body property. We can then use Heap::heap(sittingAroundObject)-globalData() to get back to the right JSGlobalObject. Heap::heap() will give you a JSGlobalData pointer, but not a JSGlobalObject pointer. All JSGlobalObjects in WebCore share the same Heap and the same JSGlobalData. Using a different heap for each JSGlobalObject would be a pretty major change. An alternative is to add state to every JSDOMObject to remember with which JSGlobalObject it's associated. This might have memory overhead. Almost all DOM access proceeds from [ window ] - [ document ] - [ interior stuff ], so it's probably feasible to propagate a window pointer along all chains of property access. This would also be a pretty major change, though. In JavaScript, it's trivial to figure out the global object that originated execution, and the global object in which the currently executing function was defined. So, if script S calls function F, it's trivial to figure out the window in which S was loaded, and also the window in which F was defined. But figuring out the window from which an arbitrary object originated seems challenging. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Is it definitely right for document.body to make a wrapper using prototypes from the document's host window, rather than from the accessing function's window? What do other browsers do? That's correct. Other browser's get this case right. Is there a particular security or other benefit here, or do we just want to make this change to match other browsers? Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
I'm first fixing JSCell::new subclasses to make sure they're always allocating in the correct heap. If we're to map from objects to the associated globalobject via the heap, we need to fix allocation first. I'm not sure what you guys have been meaning by the phrase correct heap. Barring workers, all WebCore objects are allocated from the same heap. I'm wondering if I should instead be changing ExecState to carry a current global object member (the global object which carries the heap that objects should be allocated in, and prototypes should be looked up from. This is different from either the lexical or global objects). That would require fixing many callsites, but probably fewer than my current approach. If you want to pass an argument indicating where something should be allocated, I don't think ExecState is the right place to do it. ExecState is a read-only pointer into the calling function's stack. It can answer questions about the calling function's state, but that's it. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 1:36 PM, Geoffrey Garengga...@apple.com wrote:
I'm not sure what you guys have been meaning by the phrase correct heap.
Barring workers, all WebCore objects are allocated from the same heap.
We had wrongly assumed that each window got its own. OK. This
invalidates using heaps as a way to get back to a global object, thank
you.
If you want to pass an argument indicating where something should be
allocated, I don't think ExecState is the right place to do it. ExecState is
a read-only pointer into the calling function's stack. It can answer
questions about the calling function's state, but that's it.
It's important for objects to be allocated with the right prototype
chain, otherwise we will have bugs, some of them security related. :)
Example:
A.html:
iframe name=B href=B.html
scriptB.doSomething();/script
B.html:
iframe name=C href=C.html
scriptfunction doSomething() { return C.contentDocument.body; }/script
C.html:
body
In this example:
A is the dynamicGlobalObject
B is the lexicalGlobalObject
but C is the current global object (the one that the
JSHTMLBodyElement) should be allocated in and from which the
JSHTMLBodyElement prototype chain should come from. All browsers get
variants of this idiom wrong in different places.
There are two was I can see to fix this:
1. Pass a current global object through to all toJS calls (lots of
callsites changed)
2. Store a current global object off of the ExecState (set by the
JS engine before leaving into custom native code for property lookup
or function execution).
I tried #1 this morning, and now think #2 is the cleaner way to go.
I'm very interested in your thoughts.
-eric
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 1:57 PM, Eric Seidel e...@webkit.org wrote:
On Mon, Jul 13, 2009 at 1:36 PM, Geoffrey Garengga...@apple.com wrote:
I'm not sure what you guys have been meaning by the phrase correct
heap.
Barring workers, all WebCore objects are allocated from the same heap.
We had wrongly assumed that each window got its own. OK. This
invalidates using heaps as a way to get back to a global object, thank
you.
If you want to pass an argument indicating where something should be
allocated, I don't think ExecState is the right place to do it. ExecState
is
a read-only pointer into the calling function's stack. It can answer
questions about the calling function's state, but that's it.
It's important for objects to be allocated with the right prototype
chain, otherwise we will have bugs, some of them security related. :)
Example:
A.html:
iframe name=B href=B.html
scriptB.doSomething();/script
B.html:
iframe name=C href=C.html
scriptfunction doSomething() { return C.contentDocument.body; }/script
C.html:
body
In this example:
A is the dynamicGlobalObject
B is the lexicalGlobalObject
but C is the current global object (the one that the
JSHTMLBodyElement) should be allocated in and from which the
JSHTMLBodyElement prototype chain should come from. All browsers get
variants of this idiom wrong in different places.
There are two was I can see to fix this:
1. Pass a current global object through to all toJS calls (lots of
callsites changed)
2. Store a current global object off of the ExecState (set by the
JS engine before leaving into custom native code for property lookup
or function execution).
I tried #1 this morning, and now think #2 is the cleaner way to go.
I'm very interested in your thoughts.
I discussed this a bit with Darin and Geoff, and we came to the conclusion
that the correct fix is to have each JS DOMObject store a JSGlobalObject
pointer and augment the toJS methods to pass a global object instead of an
ExecState (close to you #1). I would not advocate storing more data on the
ExecState.
The tricky part will be getting cases the edge cases such as events and
callbacks correct.
-Sam
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 2:18 PM, Sam Weinigsam.wei...@gmail.com wrote: I discussed this a bit with Darin and Geoff, and we came to the conclusion that the correct fix is to have each JS DOMObject store a JSGlobalObject pointer and augment the toJS methods to pass a global object instead of an ExecState (close to you #1). There are classes in JavaScriptCore with mutable prototype chains which will not be covered by this fix, no? 1.__proto__ for instance, no? Adam would have to comment as to how much of a concern that would be. I would not advocate storing more data on the ExecState. The tricky part will be getting cases the edge cases such as events and callbacks correct. -Sam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
1. Pass a current global object through to all toJS calls (lots of callsites changed) 2. Store a current global object off of the ExecState (set by the JS engine before leaving into custom native code for property lookup or function execution). I discussed this a bit with Darin and Geoff, and we came to the conclusion that the correct fix is to have each JS DOMObject store a JSGlobalObject pointer and augment the toJS methods to pass a global object instead of an ExecState (close to you #1). I would not advocate storing more data on the ExecState. To add to what Sam just said, I don't think #2 is viable. Where would the JS engine get the current global object from? Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Jul 13, 2009, at 2:18 PM, Sam Weinig wrote: I discussed this a bit with Darin and Geoff, and we came to the conclusion that the correct fix is to have each JS DOMObject store a JSGlobalObject pointer and augment the toJS methods to pass a global object instead of an ExecState (close to you #1). You probably mean in addition to rather than instead of. - Maciej (As a side note, I'm not sure this is really a security issue, since we're primarily talking about same-origin access here. For the few cases where cross-origin access is allowed, we would *not* want to expose the home window's prototype chain. So for Window.postMessage for instance, cross-origin access need to give you a distinct wrapper.) ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
I discussed this a bit with Darin and Geoff, and we came to the conclusion that the correct fix is to have each JS DOMObject store a JSGlobalObject pointer and augment the toJS methods to pass a global object instead of an ExecState (close to you #1). You probably mean in addition to rather than instead of. We talked about instead of. We think the JSGlobalObject will render the ExecState irrelevant, since the ExecState was only used to access the heap, which JSGlobalObject can do, and to select a prototype, which this fix will do. (As a side note, I'm not sure this is really a security issue, since we're primarily talking about same-origin access here. For the few cases where cross-origin access is allowed, we would *not* want to expose the home window's prototype chain. So for Window.postMessage for instance, cross-origin access need to give you a distinct wrapper.) Yeah, probably not a security issue. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
I discussed this a bit with Darin and Geoff, and we came to the conclusion that the correct fix is to have each JS DOMObject store a JSGlobalObject pointer and augment the toJS methods to pass a global object instead of an ExecState (close to you #1). There are classes in JavaScriptCore with mutable prototype chains which will not be covered by this fix, no? After the engine properly constructs an object with the correct prototype, the author can change that prototype, yes. But I don't think that invalidates our strategy for property constructing objects with the correct prototype. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 1:33 PM, Geoffrey Garengga...@apple.com wrote: Is it definitely right for document.body to make a wrapper using prototypes from the document's host window, rather than from the accessing function's window? What do other browsers do? That's correct. Other browser's get this case right. Is there a particular security or other benefit here, or do we just want to make this change to match other browsers? Our current behavior is buggy, unpredictable, and out of spec. This has led to security bugs in the past and will lead to security bugs in the future. Is there a particular reason to keep our current behavior? Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 2:26 PM, Maciej Stachowiakm...@apple.com wrote: For the few cases where cross-origin access is allowed, we would *not* want to expose the home window's prototype chain. So for Window.postMessage for instance, cross-origin access need to give you a distinct wrapper. That's correct. In fact, Firefox had this exact bug as recently as a year ago. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Is there a particular security or other benefit here, or do we just want to make this change to match other browsers? Our current behavior is buggy, unpredictable, and out of spec. This has led to security bugs in the past and will lead to security bugs in the future. I don't disagree with you, but I'm not immediately convinced that a large design change will automatically reduce the bug count, either. Which spec did you have in mind? I'd like to read it. Is there a particular reason to keep our current behavior? No, I don't think so -- and I don't mean to object to this idea, either. I was just asking for clarification. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
I discussed this a bit with Darin and Geoff, and we came to the conclusion that the correct fix is to have each JS DOMObject store a JSGlobalObject pointer and augment the toJS methods to pass a global object instead of an ExecState (close to you #1). I would not advocate storing more data on the ExecState. Sam and I just noticed that, to fully support this behavior, any host object that lazily constructs function properties using the functions in lookup.h must either (a) change in the same way as DOM objects must change or (b) eagerly construct its function properties with the correct prototype. Consider this case, which does not involve a DOM object: frames[0].Array.prototype.push.__proto__ == Array.prototype.push.__proto__ Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 3:29 PM, Geoffrey Garengga...@apple.com wrote: Our current behavior is buggy, unpredictable, and out of spec. This has led to security bugs in the past and will lead to security bugs in the future. I don't disagree with you, but I'm not immediately convinced that a large design change will automatically reduce the bug count, either. Which spec did you have in mind? I'd like to read it. Essentially, the ECMAScript spec requires this. In spec-land, these objects are all created at the beginning of time. The fact that we create them lazily is what leads to this bug. Depending on who touches them first, they end up with different prototype chains, which doesn't make sense to ECMAScript. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Which spec did you have in mind? I'd like to read it. Essentially, the ECMAScript spec requires this. In spec-land, these objects are all created at the beginning of time. The fact that we create them lazily is what leads to this bug. Depending on who touches them first, they end up with different prototype chains, which doesn't make sense to ECMAScript. I agree that it's strange for an object's behavior to change based on which global object accesses it first, but I don't think the ECMA spec is a good guide here, since, according to ECMA 262, the very notion of more than one global object is invalid. That's correct. Other browser's get this case right. Here are a couple test cases you might find interesting: http://webblaze.org/abarth/tests/protoconfused/test1.html http://webblaze.org/abarth/tests/protoconfused/test2.html I tried these tests, with mixed results: IE8: Exception thrown during load. Firefox 3.0: mixture of passes and fails on test1.html. Exception thrown during load of test2.html. Chrome 2.0: Mixture of passes and fails. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Yep... My guess is that (a) is probably better, but you two are more expert on why we use lazy construction. Heh -- lazy construction in JSC is even older than I am! :) The goal is to reduce memory use and startup cost in cases where certain functions aren't used. I'm not sure how much of a difference it makes in practice. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Jul 13, 2009, at 3:36 PM, Geoffrey Garen wrote: I discussed this a bit with Darin and Geoff, and we came to the conclusion that the correct fix is to have each JS DOMObject store a JSGlobalObject pointer and augment the toJS methods to pass a global object instead of an ExecState (close to you #1). I would not advocate storing more data on the ExecState. Sam and I just noticed that, to fully support this behavior, any host object that lazily constructs function properties using the functions in lookup.h must either (a) change in the same way as DOM objects must change or (b) eagerly construct its function properties with the correct prototype. Consider this case, which does not involve a DOM object: frames[0].Array.prototype.push.__proto__ == Array.prototype.push.__proto__ Built-in classes work somewhat differently. I believe they use the calling function's global object (lexical global object) rather than having some notion of home object. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Consider this case, which does not involve a DOM object: frames[0].Array.prototype.push.__proto__ == Array.prototype.push.__proto__ Built-in classes work somewhat differently. I believe they use the calling function's global object (lexical global object) rather than having some notion of home object. In Firefox 3.0, built-ins use a home object. Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 4:01 PM, Geoffrey Garengga...@apple.com wrote: That's correct. Other browser's get this case right. Here are a couple test cases you might find interesting: http://webblaze.org/abarth/tests/protoconfused/test1.html http://webblaze.org/abarth/tests/protoconfused/test2.html I tried these tests, with mixed results: IE8: Exception thrown during load. Firefox 3.0: mixture of passes and fails on test1.html. Exception thrown during load of test2.html. Chrome 2.0: Mixture of passes and fails. Yes. All the browsers suck on these tests. :) Would you like me to go look for more exploitable cases? It seems like the only reason not to fix this issue is because we're afraid of code churn. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Jul 13, 2009, at 3:40 PM, Adam Barth wrote: On Mon, Jul 13, 2009 at 3:29 PM, Geoffrey Garengga...@apple.com wrote: Our current behavior is buggy, unpredictable, and out of spec. This has led to security bugs in the past and will lead to security bugs in the future. I don't disagree with you, but I'm not immediately convinced that a large design change will automatically reduce the bug count, either. Which spec did you have in mind? I'd like to read it. Essentially, the ECMAScript spec requires this. In spec-land, these objects are all created at the beginning of time. The fact that we create them lazily is what leads to this bug. Depending on who touches them first, they end up with different prototype chains, which doesn't make sense to ECMAScript. While the behavior you describe seems problematic, I don't think it is an ECMAScript violation, since ECMAScript essentially allows host objects to do anything. If this is defined by spec, the specs that are relevant would be HTML5 and Web IDL. I'm not sure if those clearly define the behavior. - Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 4:08 PM, Maciej Stachowiakm...@apple.com wrote: Built-in classes work somewhat differently. I believe they use the calling function's global object (lexical global object) rather than having some notion of home object. You might be thinking of boxing primitive values, which do indeed get their prototypes from the current lexical scope. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Jul 13, 2009, at 4:12 PM, Adam Barth wrote: On Mon, Jul 13, 2009 at 4:08 PM, Maciej Stachowiakm...@apple.com wrote: Built-in classes work somewhat differently. I believe they use the calling function's global object (lexical global object) rather than having some notion of home object. You might be thinking of boxing primitive values, which do indeed get their prototypes from the current lexical scope. Hmm. Exporting home global object behavior to some of the built-in JS types would be more challenging than doing it just for DOM objects. Some of the built-in classes are up against the JSC cell size limit. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Jul 13, 2009, at 4:11 PM, Adam Barth wrote: On Mon, Jul 13, 2009 at 4:01 PM, Geoffrey Garengga...@apple.com wrote: That's correct. Other browser's get this case right. Here are a couple test cases you might find interesting: http://webblaze.org/abarth/tests/protoconfused/test1.html http://webblaze.org/abarth/tests/protoconfused/test2.html I tried these tests, with mixed results: IE8: Exception thrown during load. Firefox 3.0: mixture of passes and fails on test1.html. Exception thrown during load of test2.html. Chrome 2.0: Mixture of passes and fails. Yes. All the browsers suck on these tests. :) Would you like me to go look for more exploitable cases? It seems like the only reason not to fix this issue is because we're afraid of code churn. My own interest would be in weighing the tradeoffs. In the Pro column: 1) Are there aspects of this issue that create security holes? 2) Are there aspects of this issue that create Web compatibility problems? 3) Would enforcing a new consistent behavior for this reduce the likelihood of future mistakes that cause material problems? In the potential Con column: A) Will there be a performance or memory penalty? B) Will the change increase code complexity? C) Is there a risk of introducing regressions with the change? If the benefits are high, then it's not that important to assess the risks - we will just live with them. If the benefits are low but the costs are also low, then I'd say having logical behavior wins out. It's only if the costs are high and the benefits are low that I'd be concerned about making the change at all. I wouldn't say code churn is categorically ruled out. Personally I am convinced that the answers to (3) and (C) are both yes, and I don't know about any of the other points. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 4:20 PM, Maciej Stachowiakm...@apple.com wrote: My own interest would be in weighing the tradeoffs. In the Pro column: 1) Are there aspects of this issue that create security holes? CVE-2009-1702 is an example of such as security hole. I'm sure that I can find more if I look for them. 2) Are there aspects of this issue that create Web compatibility problems? I don't know of any web compatibility problems arising from this behavior. The great deviation between browsers here makes this less likely 3) Would enforcing a new consistent behavior for this reduce the likelihood of future mistakes that cause material problems? You're already convinced the answer here is yes. In the potential Con column: A) Will there be a performance or memory penalty? I suspect the new behavior will take more memory, but I don't know how much. B) Will the change increase code complexity? In some cases, the change might actually reduce code complexity. For example, if you look at the bindings code around events and database transactions, we do a lot of machinating to find the right security context. Much of this code is buggy. Having the appropriate JSGlobalObject nearby might help. C) Is there a risk of introducing regressions with the change? There is indeed a chance of regressions. We'll want to test thoroughly. Personally I am convinced that the answers to (3) and (C) are both yes, and I don't know about any of the other points. I can try to provide more information as best I can. Another option is to make this change incrementally and assess the costs as we go. For example, we could fix the SVG prototypes first. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Jul 13, 2009, at 4:34 PM, Adam Barth wrote: CVE-2009-1702 is an example of such as security hole. I'm sure that I can find more if I look for them. I think objects attached to the global object and accessible cross- origin are a special case here. (The advisory cited is about Location and History.) Values that can be accessed cross-origin are the most likely locus for security issues in this area. And they also likely need behavior that's different from the generally proposed pattern, since revealing or allowing the mutation of home origin prototypes is a security risk. If security is one motivation for this work, then I'd like us to understand the pattern we want to use for cross-origin-accessible objects. Should they use the home global object prototype but protect it from mutation or access to extended properties, should they use the prototype of the referencing frame (lexical global object) or something else? Personally I am convinced that the answers to (3) and (C) are both yes, and I don't know about any of the other points. I can try to provide more information as best I can. Another option is to make this change incrementally and assess the costs as we go. For example, we could fix the SVG prototypes first. Doing the change incrementally seems wise, if it is feasible to do so. Maybe even a patch demonstrating how it would work for a single class could be a good way to evaluate the change. Perhaps separate trailblazing examples could be given for both an ordinary class and one that is cross-origin accessible. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Mon, Jul 13, 2009 at 4:59 PM, Maciej Stachowiakm...@apple.com wrote: If security is one motivation for this work, then I'd like us to understand the pattern we want to use for cross-origin-accessible objects. Should they use the home global object prototype but protect it from mutation or access to extended properties, should they use the prototype of the referencing frame (lexical global object) or something else? I can study this question, but I believe Firefox solves this problem by having cross-origin viewers of these properties see a fresh copy of the object that isn't === the object as seen by same-origin viewers. The fresh copy ignores any changes the page might has made to the object and has a prototype chain connects to the viewer's prototypes. If two different cross-origin viewers look at the same object, they each see fresh copies. Doing the change incrementally seems wise, if it is feasible to do so. Maybe even a patch demonstrating how it would work for a single class could be a good way to evaluate the change. Perhaps separate trailblazing examples could be given for both an ordinary class and one that is cross-origin accessible. Sounds like a plan. Thanks for all your input on this topic. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Jul 13, 2009, at 5:34 PM, Adam Barth wrote: On Mon, Jul 13, 2009 at 4:59 PM, Maciej Stachowiakm...@apple.com wrote: If security is one motivation for this work, then I'd like us to understand the pattern we want to use for cross-origin-accessible objects. Should they use the home global object prototype but protect it from mutation or access to extended properties, should they use the prototype of the referencing frame (lexical global object) or something else? I can study this question, but I believe Firefox solves this problem by having cross-origin viewers of these properties see a fresh copy of the object that isn't === the object as seen by same-origin viewers. The fresh copy ignores any changes the page might has made to the object and has a prototype chain connects to the viewer's prototypes. If two different cross-origin viewers look at the same object, they each see fresh copies. That pattern sounds workable. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Fri, Jul 10, 2009 at 6:29 PM, Maciej Stachowiakm...@apple.com wrote: On Jul 10, 2009, at 6:10 PM, Adam Barth wrote: That's correct. Other browser's get this case right. Here are a couple test cases you might find interesting: http://webblaze.org/abarth/tests/protoconfused/test1.html http://webblaze.org/abarth/tests/protoconfused/test2.html The question is how to compute the correct wrapper context in all cases. There are a bunch of approaches that cover 80% of the cases. The trick is finding an approach that works for 100% of the causes. Well, for DOM Nodes you can almost always chase backpointers all the way up to the Document and from there to the Window, but this could be inefficient. And there's objects in the DOM that are not Nodes at all, and can't readily reach a Node. We tried this approach for a while, but we came across CSSValue, which has no obvious back pointers. It's a bit unclear how to do this for all Nodes even. For example, DocType Nodes might not have an associated Document. It's possible we could change WebCore to have all the appropriate back pointers. However, it might be easier to have all the wrapper objects hold the global object directly (or indirectly via their heap placement). I will ask Sam and Geoff for their thoughts in person if they don't chime in on the list. Thanks. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Re-sent with correct address. On Fri, Jul 10, 2009 at 5:55 PM, Adam Barthaba...@eecs.berkeley.edu wrote: Eric and I spent some more time this afternoon looking at this. We don't think the ExecState::thisValue() approach is going to work. We implemented hacky version to experiment with, but the problem is with cases like this: document.body In evaluating this case, the this value isn't what we want because our computed thisValue is for the running function, not for the getter. Our current thinking is to add a parameter to toJS() to receive the JSGlobalObject in which to create the wrapper. Once we do that, the question is how to find the proper JSGlobalObject at each call site. In most cases, you have another JSObject sitting around that is from the right JSGlobalObject. In the document.body example, the getter for body has the JSDocument from which it gets the body property. We can then use Heap::heap(sittingAroundObject)-globalData() to get back to the right JSGlobalObject. Of course, this requires that objects get allocated in the right heap, which isn't quite the case at the moment because the new operator takes an ExecState and chooses the heap based on the lexicalGlobalObject. An alternative is to add state to every JSDOMObject to remember with which JSGlobalObject it's associated. This might have memory overhead. Thoughts? Adam On Fri, Jul 10, 2009 at 3:59 PM, Eric Seidele...@webkit.org wrote: Geoff, Gavin, Sam, Maciej (and any other JSC experts): Adam and I are fixing: https://bugs.webkit.org/show_bug.cgi?id=27088 Fix: toJS needs to use the correct global object. The correct global object should come from whatever this is calling into the native code which is using toJS. (e.g. document.body. It doesn't matter how/when you access it, the correct prototype comes from whichever global object which the document was created from.) BACKGROUND EXAMPLE: scriptframes[0].document.createElement(foo)/script Should use thisValue as passed into: JSValue JSC_HOST_CALL jsDocumentPrototypeFunctionCreateElement(ExecState* exec, JSObject*, JSValue thisValue, const ArgList args) We believe the easiest way to get the correct globalData is via: *Heap::heap(thisValue)-globalData() QUESTIONS: We do not need to change the signature of all toJS implementations if the ExecState can carry the thisValue for us. ExecState::thisValue() already exists, but ExecState::codeBlock() seems NULL in our case. (thisValue() seems used by the debugger). 1. Is it correct for the ExecState to carry the thisValue? 2. If ExecState is OK to carry thisValue how should we fix it to be non-NULL in jsDocumentPrototypeFunctionCreateElement (and other places)? Thanks, Eric Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
On Jul 10, 2009, at 5:55 PM, Adam Barth wrote: Eric and I spent some more time this afternoon looking at this. We don't think the ExecState::thisValue() approach is going to work. We implemented hacky version to experiment with, but the problem is with cases like this: document.body In evaluating this case, the this value isn't what we want because our computed thisValue is for the running function, not for the getter. Is it definitely right for document.body to make a wrapper using prototypes from the document's host window, rather than from the accessing function's window? What do other browsers do? - Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
Sent again from the right address. Gmail hates me today. On Fri, Jul 10, 2009 at 6:10 PM, Adam Barthaba...@eecs.berkeley.edu wrote: On Fri, Jul 10, 2009 at 6:04 PM, Maciej Stachowiakm...@apple.com wrote: On Jul 10, 2009, at 5:55 PM, Adam Barth wrote: Eric and I spent some more time this afternoon looking at this. We don't think the ExecState::thisValue() approach is going to work. We implemented hacky version to experiment with, but the problem is with cases like this: document.body In evaluating this case, the this value isn't what we want because our computed thisValue is for the running function, not for the getter. Is it definitely right for document.body to make a wrapper using prototypes from the document's host window, rather than from the accessing function's window? What do other browsers do? That's correct. Other browser's get this case right. Here are a couple test cases you might find interesting: http://webblaze.org/abarth/tests/protoconfused/test1.html http://webblaze.org/abarth/tests/protoconfused/test2.html The question is how to compute the correct wrapper context in all cases. There are a bunch of approaches that cover 80% of the cases. The trick is finding an approach that works for 100% of the causes. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] ExecState::thisObject()
(Trimming Cc's since the relevant people are on webkit-dev anyway.) On Jul 10, 2009, at 6:10 PM, Adam Barth wrote: On Fri, Jul 10, 2009 at 6:04 PM, Maciej Stachowiakm...@apple.com wrote: Is it definitely right for document.body to make a wrapper using prototypes from the document's host window, rather than from the accessing function's window? What do other browsers do? That's correct. Other browser's get this case right. Here are a couple test cases you might find interesting: http://webblaze.org/abarth/tests/protoconfused/test1.html http://webblaze.org/abarth/tests/protoconfused/test2.html The question is how to compute the correct wrapper context in all cases. There are a bunch of approaches that cover 80% of the cases. The trick is finding an approach that works for 100% of the causes. Well, for DOM Nodes you can almost always chase backpointers all the way up to the Document and from there to the Window, but this could be inefficient. And there's objects in the DOM that are not Nodes at all, and can't readily reach a Node. I will ask Sam and Geoff for their thoughts in person if they don't chime in on the list. - Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
