Do proper security checks when accessing elements with getOwnPropertyDescriptor.

This extends logic applied to regular properties to elements.

Review URL: http://codereview.chromium.org/6246055

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6626 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/runtime.cc

@@ -715,6 +715,19 @@ static bool CheckAccess(JSObject* obj,
 }
 
 
+// TODO(1095): we should traverse hidden prototype hierachy as well.
+static bool CheckElementAccess(JSObject* obj,
+                               uint32_t index,
+                               v8::AccessType access_type) {
+  if (obj->IsAccessCheckNeeded() &&
+      !Top::MayIndexedAccess(obj, index, access_type)) {
+    return false;
+  }
+
+  return true;
+}
+
+
 // Enumerator used as indices into the array returned from GetOwnProperty
 enum PropertyDescriptorIndices {
   IS_ACCESSOR_INDEX,
@@ -757,7 +770,7 @@ static MaybeObject* Runtime_GetOwnProperty(Arguments args) {
         // subsequent cases.
         Handle<JSValue> js_value = Handle<JSValue>::cast(obj);
         Handle<String> str(String::cast(js_value->value()));
-        Handle<String> substr = SubString(str, index, index+1, NOT_TENURED);
+        Handle<String> substr = SubString(str, index, index + 1, NOT_TENURED);
 
         elms->set(IS_ACCESSOR_INDEX, Heap::false_value());
         elms->set(VALUE_INDEX, *substr);
@@ -770,8 +783,7 @@ static MaybeObject* Runtime_GetOwnProperty(Arguments args) {
       case JSObject::INTERCEPTED_ELEMENT:
       case JSObject::FAST_ELEMENT: {
         elms->set(IS_ACCESSOR_INDEX, Heap::false_value());
-        Handle<Object> element = GetElement(Handle<Object>(obj), index);
-        elms->set(VALUE_INDEX, *element);
+        elms->set(VALUE_INDEX, *GetElement(obj, index));
         elms->set(WRITABLE_INDEX, Heap::true_value());
         elms->set(ENUMERABLE_INDEX,  Heap::true_value());
         elms->set(CONFIGURABLE_INDEX, Heap::true_value());
@@ -779,13 +791,14 @@ static MaybeObject* Runtime_GetOwnProperty(Arguments args) {
       }
 
       case JSObject::DICTIONARY_ELEMENT: {
+        Handle<JSObject> holder = obj;
         if (obj->IsJSGlobalProxy()) {
           Object* proto = obj->GetPrototype();
           if (proto->IsNull()) return Heap::undefined_value();
           ASSERT(proto->IsJSGlobalObject());
-          obj = Handle<JSObject>(JSObject::cast(proto));
+          holder = Handle<JSObject>(JSObject::cast(proto));
         }
-        NumberDictionary* dictionary = obj->element_dictionary();
+        NumberDictionary* dictionary = holder->element_dictionary();
         int entry = dictionary->FindEntry(index);
         ASSERT(entry != NumberDictionary::kNotFound);
         PropertyDetails details = dictionary->DetailsAt(entry);
@@ -795,14 +808,18 @@ static MaybeObject* Runtime_GetOwnProperty(Arguments args) {
             FixedArray* callbacks =
                 FixedArray::cast(dictionary->ValueAt(entry));
             elms->set(IS_ACCESSOR_INDEX, Heap::true_value());
-            elms->set(GETTER_INDEX, callbacks->get(0));
-            elms->set(SETTER_INDEX, callbacks->get(1));
+            if (CheckElementAccess(*obj, index, v8::ACCESS_GET)) {
+              elms->set(GETTER_INDEX, callbacks->get(0));
+            }
+            if (CheckElementAccess(*obj, index, v8::ACCESS_SET)) {
+              elms->set(SETTER_INDEX, callbacks->get(1));
+            }
             break;
           }
           case NORMAL:
             // This is a data property.
             elms->set(IS_ACCESSOR_INDEX, Heap::false_value());
-            elms->set(VALUE_INDEX, dictionary->ValueAt(entry));
+            elms->set(VALUE_INDEX, *GetElement(obj, index));
             elms->set(WRITABLE_INDEX, Heap::ToBoolean(!details.IsReadOnly()));
             break;
           default:

test/cctest/test-api.cc

@@ -5323,7 +5323,8 @@ static bool IndexedAccessBlocker(Local<v8::Object> global,
                                  uint32_t key,
                                  v8::AccessType type,
                                  Local<Value> data) {
-  return Context::GetCurrent()->Global()->Equals(global);
+  return Context::GetCurrent()->Global()->Equals(global) ||
+      allowed_access_type[type];
 }
 
 
@@ -5390,6 +5391,18 @@ TEST(AccessControl) {
   Local<Value> getter = global0->Get(v8_str("getter"));
   Local<Value> setter = global0->Get(v8_str("setter"));
 
+  // And define normal element.
+  global0->Set(239, v8_str("239"));
+
+  // Define an element with JS getter and setter.
+  CompileRun(
+      "function el_getter() { return 'el_getter'; };\n"
+      "function el_setter() { return 'el_setter'; };\n"
+      "Object.defineProperty(this, '42', {get: el_getter, set: el_setter});");
+
+  Local<Value> el_getter = global0->Get(v8_str("el_getter"));
+  Local<Value> el_setter = global0->Get(v8_str("el_setter"));
+
   v8::HandleScope scope1;
 
   v8::Persistent<Context> context1 = Context::New();
@@ -5398,7 +5411,7 @@ TEST(AccessControl) {
   v8::Handle<v8::Object> global1 = context1->Global();
   global1->Set(v8_str("other"), global0);
 
-  // Access blocked property
+  // Access blocked property.
   CompileRun("other.blocked_prop = 1");
 
   ExpectUndefined("other.blocked_prop");
@@ -5416,6 +5429,23 @@ TEST(AccessControl) {
   ExpectTrue("propertyIsEnumerable.call(other, 'blocked_prop')");
   allowed_access_type[v8::ACCESS_HAS] = false;
 
+  // Access blocked element.
+  CompileRun("other[239] = 1");
+
+  ExpectUndefined("other[239]");
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '239')");
+  ExpectFalse("propertyIsEnumerable.call(other, '239')");
+
+  // Enable ACCESS_HAS
+  allowed_access_type[v8::ACCESS_HAS] = true;
+  ExpectUndefined("other[239]");
+  // ... and now we can get the descriptor...
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '239').value");
+  // ... and enumerate the property.
+  ExpectTrue("propertyIsEnumerable.call(other, '239')");
+  allowed_access_type[v8::ACCESS_HAS] = false;
+
+  // Access a property with JS accessor.
   CompileRun("other.js_accessor_p = 2");
 
   ExpectUndefined("other.js_accessor_p");
@@ -5480,6 +5510,58 @@ TEST(AccessControl) {
   allowed_access_type[v8::ACCESS_GET] = false;
   allowed_access_type[v8::ACCESS_HAS] = false;
 
+  // Access an element with JS accessor.
+  CompileRun("other[42] = 2");
+
+  ExpectUndefined("other[42]");
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42')");
+
+  // Enable ACCESS_HAS.
+  allowed_access_type[v8::ACCESS_HAS] = true;
+  ExpectUndefined("other[42]");
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').get");
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').set");
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').value");
+  allowed_access_type[v8::ACCESS_HAS] = false;
+
+  // Enable both ACCESS_HAS and ACCESS_GET.
+  allowed_access_type[v8::ACCESS_HAS] = true;
+  allowed_access_type[v8::ACCESS_GET] = true;
+
+  ExpectString("other[42]", "el_getter");
+  ExpectObject("Object.getOwnPropertyDescriptor(other, '42').get", el_getter);
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').set");
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').value");
+
+  allowed_access_type[v8::ACCESS_GET] = false;
+  allowed_access_type[v8::ACCESS_HAS] = false;
+
+  // Enable both ACCESS_HAS and ACCESS_SET.
+  allowed_access_type[v8::ACCESS_HAS] = true;
+  allowed_access_type[v8::ACCESS_SET] = true;
+
+  ExpectUndefined("other[42]");
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').get");
+  ExpectObject("Object.getOwnPropertyDescriptor(other, '42').set", el_setter);
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').value");
+
+  allowed_access_type[v8::ACCESS_SET] = false;
+  allowed_access_type[v8::ACCESS_HAS] = false;
+
+  // Enable both ACCESS_HAS, ACCESS_GET and ACCESS_SET.
+  allowed_access_type[v8::ACCESS_HAS] = true;
+  allowed_access_type[v8::ACCESS_GET] = true;
+  allowed_access_type[v8::ACCESS_SET] = true;
+
+  ExpectString("other[42]", "el_getter");
+  ExpectObject("Object.getOwnPropertyDescriptor(other, '42').get", el_getter);
+  ExpectObject("Object.getOwnPropertyDescriptor(other, '42').set", el_setter);
+  ExpectUndefined("Object.getOwnPropertyDescriptor(other, '42').value");
+
+  allowed_access_type[v8::ACCESS_SET] = false;
+  allowed_access_type[v8::ACCESS_GET] = false;
+  allowed_access_type[v8::ACCESS_HAS] = false;
+
   v8::Handle<Value> value;
 
   // Access accessible property