Currently we use lookupDOMWrapper to find out a proper JS wrapper which allows access to window C++ counterpart and stuff. Let's access it immediately from holder (but that requires some duplication)
Created attachment 39183 [details] First pass Guys, I'd esp. ask to review security-related changes as my knowledge of this domain is limited. And sorry if svn-preparePatch put wrong paths---I'll fix it in a next round.
Created attachment 39184 [details] ChangeLog fixed
Comment on attachment 39184 [details] ChangeLog fixed I think this is ok. The dangerous piece is in installDOMWindow, but that should run before any evil JS has had a chance to screw up the jsWindow's prototype chain.
Comment on attachment 39184 [details] ChangeLog fixed Actually, I lied.
Can you add the following test case: Evil window creates an iframe to honest domain. Evil window then sets its __proto__ to the iframe's window object. Now, try to access properties that are normally on the WindowPrototype and see if you get the wrong answers w.r.t. security. It might be ok. I'll have to think about it more, but we should have the test case in any event.
I went over this this patch in detail with Anton. Everything looks fine except for the NAMED and INDEX access checks function. It's unclear whether |host| is always the same object as |holder|. If the objects are different, we might be access checking one object and operating on another object. He's going to investigate and write some tests.
Created attachment 39853 [details] Second iteration Another pass. It passes newly added layout tests. The major difference is lookupDOMWrapper is restored in accessors checks. I believe the reason is not security, but some technical details (if you're curious: the way v8 is implemented it installs security callback on global object proxy and I don't want to put pointers to window impl on it). I'll address those issues with a separate CL. IMPORTANT NOTE (sorry for the case): please, do not land this patch: if it passes a review, I'd have to add a helper method to public V8 API (to lookup real named property).
Comment on attachment 39853 [details] Second iteration This looks great. Much easier to understand than the last round. One question: +namespace v8 { +Handle<Value> GetRealNamedProperty( + Handle<Object> object, + Handle<String> key); +} Shouldn't this be in a v8 header file? As written, it looks like it's grabbing at some random symbol in V8.
(In reply to comment #8) > (From update of attachment 39853 [details]) > This looks great. Much easier to understand than the last round. One > question: > > +namespace v8 { > +Handle<Value> GetRealNamedProperty( > + Handle<Object> object, > + Handle<String> key); > +} > > Shouldn't this be in a v8 header file? As written, it looks like it's grabbing > at some random symbol in V8. You're right, Adam---I just didn't want to touch v8's header, but still have something compilable. And exactly for same reason I asked not to land this patch as I want to add this function into public V8 API. So I'll ask for another round when this lookup goes into v8. Thanks a lot for review, Adam!
Created attachment 40534 [details] Another iteration: now we have necessary method in V8 public API Guys, that's exactly the same patch, but now with the resolved dep on public V8 API
Comment on attachment 40534 [details] Another iteration: now we have necessary method in V8 public API I just saw your committer paperwork go by, so you can either land this one yourself, or we can put it though commit queue.
(In reply to comment #11) > (From update of attachment 40534 [details]) > I just saw your committer paperwork go by, so you can either land this one > yourself, or we can put it though commit queue. If you please, put it into commit queue---paper travels very slow over the ocean :)
Comment on attachment 40534 [details] Another iteration: now we have necessary method in V8 public API Rejecting patch 40534 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=40534 from bug 29031 failed to download and apply.
Comment on attachment 39853 [details] Second iteration Obsoleting an older revision of this patch.
Comment on attachment 40534 [details] Another iteration: now we have necessary method in V8 public API IIRC the CodeGeneratorV8.pm diff failed to apply.
(In reply to comment #15) > (From update of attachment 40534 [details]) > IIRC the CodeGeneratorV8.pm diff failed to apply. Eric, anything I can help with? I synced to 49163 and svn up'ed it all fine.
Created attachment 40796 [details] And another attempt, for WebKit@49221 Adam, Eric, that's just successfully compiled (on Mac) version of the same patch.
Comment on attachment 40796 [details] And another attempt, for WebKit@49221 Let's give it a spin.
Comment on attachment 40796 [details] And another attempt, for WebKit@49221 Clearing flags on attachment: 40796 Committed r49248: <http://trac.webkit.org/changeset/49248>
All reviewed patches have been landed. Closing bug.