[V8] document.all gets confused about its prototype chain

https://bugs.webkit.org/show_bug.cgi?id=68393

Reviewed by Eric Seidel.

Source/WebCore:

GetRealNamedPropertyInPrototypeChain doesn't call interceptors, so it's
not a good idea to use its return value.  It turns out that all the
callers of the API only cared about whether it returns a null handle.

Test: http/tests/security/document-all.html

* bindings/v8/V8Collection.h:
(WebCore::collectionNamedPropertyGetter):
* bindings/v8/custom/V8DOMStringMapCustom.cpp:
(WebCore::V8DOMStringMap::namedPropertyDeleter):
(WebCore::V8DOMStringMap::namedPropertySetter):
* bindings/v8/custom/V8HTMLAllCollectionCustom.cpp:
(WebCore::V8HTMLAllCollection::namedPropertyGetter):
* bindings/v8/custom/V8HTMLCollectionCustom.cpp:
(WebCore::V8HTMLCollection::namedPropertyGetter):
* bindings/v8/custom/V8NamedNodeMapCustom.cpp:
(WebCore::V8NamedNodeMap::namedPropertyGetter):
* bindings/v8/custom/V8StorageCustom.cpp:
(WebCore::storageSetter):

LayoutTests:

Test how document.all behaves when you change its prototype chain.

* http/tests/security/document-all-expected.txt: Added.
* http/tests/security/document-all.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@95489 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index c24aeba..2258026 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,5 +1,17 @@
 2011-09-19  Adam Barth  <abarth@webkit.org>
 
+        [V8] document.all gets confused about its prototype chain
+        https://bugs.webkit.org/show_bug.cgi?id=68393
+
+        Reviewed by Eric Seidel.
+
+        Test how document.all behaves when you change its prototype chain.
+
+        * http/tests/security/document-all-expected.txt: Added.
+        * http/tests/security/document-all.html: Added.
+
+2011-09-19  Adam Barth  <abarth@webkit.org>
+
         Named property confusion with __proto__
         https://bugs.webkit.org/show_bug.cgi?id=68221
 

diff --git a/LayoutTests/http/tests/security/document-all-expected.txt b/LayoutTests/http/tests/security/document-all-expected.txt
new file mode 100644 (file)
index 0000000..89704c4
--- /dev/null
+++ b/LayoutTests/http/tests/security/document-all-expected.txt
@@ -0,0 +1,2 @@
+CONSOLE MESSAGE: line 1: Uncaught TypeError: Illegal constructor
+

diff --git a/LayoutTests/http/tests/security/document-all.html b/LayoutTests/http/tests/security/document-all.html
new file mode 100644 (file)
index 0000000..d673ad4
--- /dev/null
+++ b/LayoutTests/http/tests/security/document-all.html
@@ -0,0 +1,28 @@
+<script>
+if (window.layoutTestController) {
+    layoutTestController.dumpAsText();
+    layoutTestController.waitUntilDone();
+}
+
+window.onload = function()
+{
+    frame = document.body.appendChild(document.createElement("iframe"));
+    frame.src = "http://localhost:8080/security/resources/innocent-victim-with-iframe.html";
+    frame.onload = function() {
+        frame.onload = null;
+
+        frame.contentWindow[0].location = "data:text/html,<script>(" + function() {
+
+            setTimeout(function() {
+                if (window.layoutTestController)
+                    layoutTestController.notifyDone();
+            }, 0);
+
+            window.name = "alert";
+            obj = document.all;
+            obj.__proto__ = parent;
+            alert(obj.alert.constructor("return document.body.innerHTML")());
+        } + ")()</scr" + "ipt>";
+    }
+}
+</script>

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 14d7f89..a613b1c 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,5 +1,32 @@
 2011-09-19  Adam Barth  <abarth@webkit.org>
 
+        [V8] document.all gets confused about its prototype chain
+        https://bugs.webkit.org/show_bug.cgi?id=68393
+
+        Reviewed by Eric Seidel.
+
+        GetRealNamedPropertyInPrototypeChain doesn't call interceptors, so it's
+        not a good idea to use its return value.  It turns out that all the
+        callers of the API only cared about whether it returns a null handle.
+
+        Test: http/tests/security/document-all.html
+
+        * bindings/v8/V8Collection.h:
+        (WebCore::collectionNamedPropertyGetter):
+        * bindings/v8/custom/V8DOMStringMapCustom.cpp:
+        (WebCore::V8DOMStringMap::namedPropertyDeleter):
+        (WebCore::V8DOMStringMap::namedPropertySetter):
+        * bindings/v8/custom/V8HTMLAllCollectionCustom.cpp:
+        (WebCore::V8HTMLAllCollection::namedPropertyGetter):
+        * bindings/v8/custom/V8HTMLCollectionCustom.cpp:
+        (WebCore::V8HTMLCollection::namedPropertyGetter):
+        * bindings/v8/custom/V8NamedNodeMapCustom.cpp:
+        (WebCore::V8NamedNodeMap::namedPropertyGetter):
+        * bindings/v8/custom/V8StorageCustom.cpp:
+        (WebCore::storageSetter):
+
+2011-09-19  Adam Barth  <abarth@webkit.org>
+
         Named property confusion with __proto__
         https://bugs.webkit.org/show_bug.cgi?id=68221
 

diff --git a/Source/WebCore/bindings/v8/V8Collection.h b/Source/WebCore/bindings/v8/V8Collection.h
index 1757c85..38a7c8c 100644 (file)
--- a/Source/WebCore/bindings/v8/V8Collection.h
+++ b/Source/WebCore/bindings/v8/V8Collection.h
@@ -73,15 +73,11 @@ template<class Collection, class ItemType> static v8::Handle<v8::Value> getNamed
 // A template of named property accessor of collections.
 template<class Collection, class ItemType> static v8::Handle<v8::Value> collectionNamedPropertyGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
 {
-    v8::Handle<v8::Value> value = info.Holder()->GetRealNamedPropertyInPrototypeChain(name);
-
-    if (!value.IsEmpty())
-        return value;
-
-    // Search local callback properties next to find IDL defined
-    // properties.
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
+        return notHandledByInterceptor();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return notHandledByInterceptor();
+
     return getNamedPropertyOfCollection<Collection, ItemType>(name, info.Holder());
 }
 

diff --git a/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp b/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp
index 082be81..cebfae3 100644 (file)
--- a/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8DOMStringMapCustom.cpp
@@ -66,8 +66,8 @@ v8::Handle<v8::Array> V8DOMStringMap::namedPropertyEnumerator(const v8::Accessor
 v8::Handle<v8::Boolean> V8DOMStringMap::namedPropertyDeleter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
 {
     INC_STATS("DOM.DOMStringMap.NamedPropertyDeleter");
-    v8::Handle<v8::Value> value = info.Holder()->GetRealNamedPropertyInPrototypeChain(name);
-    if (!value.IsEmpty())
+
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
         return v8::False();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return v8::False();
@@ -82,8 +82,8 @@ v8::Handle<v8::Boolean> V8DOMStringMap::namedPropertyDeleter(v8::Local<v8::Strin
 v8::Handle<v8::Value> V8DOMStringMap::namedPropertySetter(v8::Local<v8::String> name, v8::Local<v8::Value> value, const v8::AccessorInfo& info)
 {
     INC_STATS("DOM.DOMStringMap.NamedPropertySetter");
-    v8::Handle<v8::Value> property = info.Holder()->GetRealNamedPropertyInPrototypeChain(name);
-    if (!property.IsEmpty())
+
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
         return value;
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return value;

diff --git a/Source/WebCore/bindings/v8/custom/V8HTMLAllCollectionCustom.cpp b/Source/WebCore/bindings/v8/custom/V8HTMLAllCollectionCustom.cpp
index ef428a9..a2b3cd2 100644 (file)
--- a/Source/WebCore/bindings/v8/custom/V8HTMLAllCollectionCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8HTMLAllCollectionCustom.cpp
@@ -74,18 +74,12 @@ static v8::Handle<v8::Value> getItem(HTMLAllCollection* collection, v8::Handle<v
 v8::Handle<v8::Value> V8HTMLAllCollection::namedPropertyGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
 {
     INC_STATS("DOM.HTMLAllCollection.NamedPropertyGetter");
-    // Search the prototype chain first.
-    v8::Handle<v8::Value> value = info.Holder()->GetRealNamedPropertyInPrototypeChain(name);
 
-    if (!value.IsEmpty())
-        return value;
-
-    // Search local callback properties next to find IDL defined
-    // properties.
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
+        return notHandledByInterceptor();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
-        return v8::Handle<v8::Value>();
+        return notHandledByInterceptor();
 
-    // Finally, search the DOM structure.
     HTMLAllCollection* imp = V8HTMLAllCollection::toNative(info.Holder());
     return getNamedItems(imp, v8StringToAtomicWebCoreString(name));
 }

diff --git a/Source/WebCore/bindings/v8/custom/V8HTMLCollectionCustom.cpp b/Source/WebCore/bindings/v8/custom/V8HTMLCollectionCustom.cpp
index 3d64edd..050a268 100644 (file)
--- a/Source/WebCore/bindings/v8/custom/V8HTMLCollectionCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8HTMLCollectionCustom.cpp
@@ -77,18 +77,12 @@ static v8::Handle<v8::Value> getItem(HTMLCollection* collection, v8::Handle<v8::
 v8::Handle<v8::Value> V8HTMLCollection::namedPropertyGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
 {
     INC_STATS("DOM.HTMLCollection.NamedPropertyGetter");
-    // Search the prototype chain first.
-    v8::Handle<v8::Value> value = info.Holder()->GetRealNamedPropertyInPrototypeChain(name);
 
-    if (!value.IsEmpty())
-        return value;
-
-    // Search local callback properties next to find IDL defined
-    // properties.
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
+        return notHandledByInterceptor();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
-        return v8::Handle<v8::Value>();
+        return notHandledByInterceptor();
 
-    // Finally, search the DOM structure.
     HTMLCollection* imp = V8HTMLCollection::toNative(info.Holder());
     return getNamedItems(imp, v8StringToAtomicWebCoreString(name));
 }

diff --git a/Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp b/Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp
index c9014c0..4782dcc 100644 (file)
--- a/Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8NamedNodeMapCustom.cpp
@@ -57,16 +57,12 @@ v8::Handle<v8::Value> V8NamedNodeMap::indexedPropertyGetter(uint32_t index, cons
 v8::Handle<v8::Value> V8NamedNodeMap::namedPropertyGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info)
 {
     INC_STATS("DOM.NamedNodeMap.NamedPropertyGetter");
-    // Search the prototype chain first.
-    v8::Handle<v8::Value> value = info.Holder()->GetRealNamedPropertyInPrototypeChain(name);
-    if (!value.IsEmpty())
-        return value;
 
-    // Then look for IDL defined properties on the object itself.
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())
+        return notHandledByInterceptor();
     if (info.Holder()->HasRealNamedCallbackProperty(name))
         return notHandledByInterceptor();
 
-    // Finally, search the DOM.
     NamedNodeMap* imp = V8NamedNodeMap::toNative(info.Holder());
     RefPtr<Node> result = imp->getNamedItem(toWebCoreString(name));
     if (!result)

diff --git a/Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp b/Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp
index d0a0d92..a252410 100644 (file)
--- a/Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp
+++ b/Source/WebCore/bindings/v8/custom/V8StorageCustom.cpp
@@ -102,8 +102,7 @@ static v8::Handle<v8::Value> storageSetter(v8::Local<v8::String> v8Name, v8::Loc
     if (name == "length")
         return v8Value;
 
-    v8::Handle<v8::Value> prototypeValue = info.Holder()->GetRealNamedPropertyInPrototypeChain(v8Name);
-    if (!prototypeValue.IsEmpty())
+    if (!info.Holder()->GetRealNamedPropertyInPrototypeChain(v8Name).IsEmpty())
         return notHandledByInterceptor();
 
     ExceptionCode ec = 0;