Better security checks when accessing named properties via Object.getOwnPropertyDescr...

Current approach returns undefined descriptor if caller is not granted v8::HAS_ACCESS.
If the caller has v8::HAS_ACCESS, for no JS accessors regular v8::GET_ACCESS check is
performed and value property of the descriptor is set to undefined if caller doesn't
have proper access.  For JS accessors both v8::GET_ACCESS and v8::SET_ACCESS are checked
and affect if getter and setter would be stored in the descriptor.

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

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

diff --git a/src/runtime.cc b/src/runtime.cc
index d55a201..9911e1e 100644 (file)
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -644,6 +644,77 @@ static void GetOwnPropertyImplementation(JSObject* obj,
 }
 
 
+static bool CheckAccessException(LookupResult* result,
+                                 v8::AccessType access_type) {
+  if (result->type() == CALLBACKS) {
+    Object* callback = result->GetCallbackObject();
+    if (callback->IsAccessorInfo()) {
+      AccessorInfo* info = AccessorInfo::cast(callback);
+      bool can_access =
+          (access_type == v8::ACCESS_HAS &&
+              (info->all_can_read() || info->all_can_write())) ||
+          (access_type == v8::ACCESS_GET && info->all_can_read()) ||
+          (access_type == v8::ACCESS_SET && info->all_can_write());
+      return can_access;
+    }
+  }
+
+  return false;
+}
+
+
+static bool CheckAccess(JSObject* obj,
+                        String* name,
+                        LookupResult* result,
+                        v8::AccessType access_type) {
+  ASSERT(result->IsProperty());
+
+  JSObject* holder = result->holder();
+  JSObject* current = obj;
+  while (true) {
+    if (current->IsAccessCheckNeeded() &&
+        !Top::MayNamedAccess(current, name, access_type)) {
+      // Access check callback denied the access, but some properties
+      // can have a special permissions which override callbacks descision
+      // (currently see v8::AccessControl).
+      break;
+    }
+
+    if (current == holder) {
+      return true;
+    }
+
+    current = JSObject::cast(current->GetPrototype());
+  }
+
+  // API callbacks can have per callback access exceptions.
+  switch (result->type()) {
+    case CALLBACKS: {
+      if (CheckAccessException(result, access_type)) {
+        return true;
+      }
+      break;
+    }
+    case INTERCEPTOR: {
+      // If the object has an interceptor, try real named properties.
+      // Overwrite the result to fetch the correct property later.
+      holder->LookupRealNamedProperty(name, result);
+      if (result->IsProperty()) {
+        if (CheckAccessException(result, access_type)) {
+          return true;
+        }
+      }
+      break;
+    }
+    default:
+      break;
+  }
+
+  Top::ReportFailedAccessCheck(current, access_type);
+  return false;
+}
+
+
 // Enumerator used as indices into the array returned from GetOwnProperty
 enum PropertyDescriptorIndices {
   IS_ACCESSOR_INDEX,
@@ -746,6 +817,10 @@ static MaybeObject* Runtime_GetOwnProperty(Arguments args) {
     return Heap::undefined_value();
   }
 
+  if (!CheckAccess(*obj, *name, &result, v8::ACCESS_HAS)) {
+    return Heap::undefined_value();
+  }
+
   elms->set(ENUMERABLE_INDEX, Heap::ToBoolean(!result.IsDontEnum()));
   elms->set(CONFIGURABLE_INDEX, Heap::ToBoolean(!result.IsDontDelete()));
 
@@ -754,16 +829,22 @@ static MaybeObject* Runtime_GetOwnProperty(Arguments args) {
 
   if (is_js_accessor) {
     // __defineGetter__/__defineSetter__ callback.
-    FixedArray* structure = FixedArray::cast(result.GetCallbackObject());
     elms->set(IS_ACCESSOR_INDEX, Heap::true_value());
-    elms->set(GETTER_INDEX, structure->get(0));
-    elms->set(SETTER_INDEX, structure->get(1));
+
+    FixedArray* structure = FixedArray::cast(result.GetCallbackObject());
+    if (CheckAccess(*obj, *name, &result, v8::ACCESS_GET)) {
+      elms->set(GETTER_INDEX, structure->get(0));
+    }
+    if (CheckAccess(*obj, *name, &result, v8::ACCESS_SET)) {
+      elms->set(SETTER_INDEX, structure->get(1));
+    }
   } else {
     elms->set(IS_ACCESSOR_INDEX, Heap::false_value());
     elms->set(WRITABLE_INDEX, Heap::ToBoolean(!result.IsReadOnly()));
 
     PropertyAttributes attrs;
     Object* value;
+    // GetProperty will check access and report any violations.
     { MaybeObject* maybe_value = obj->GetProperty(*obj, &result, *name, &attrs);
       if (!maybe_value->ToObject(&value)) return maybe_value;
     }

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index ee62067..b04420e 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -5309,11 +5309,13 @@ TEST(DetachAndReattachGlobal) {
 }
 
 
+static bool allowed_access_type[v8::ACCESS_KEYS] = { false };
 static bool NamedAccessBlocker(Local<v8::Object> global,
                                Local<Value> name,
                                v8::AccessType type,
                                Local<Value> data) {
-  return Context::GetCurrent()->Global()->Equals(global);
+  return Context::GetCurrent()->Global()->Equals(global) ||
+      allowed_access_type[type];
 }
 
 
@@ -5353,7 +5355,7 @@ static void UnreachableSetter(Local<String>, Local<Value>,
 }
 
 
-THREADED_TEST(AccessControl) {
+TEST(AccessControl) {
   v8::HandleScope handle_scope;
   v8::Handle<v8::ObjectTemplate> global_template = v8::ObjectTemplate::New();
 
@@ -5379,6 +5381,15 @@ THREADED_TEST(AccessControl) {
 
   v8::Handle<v8::Object> global0 = context0->Global();
 
+  // Define a property with JS getter and setter.
+  CompileRun(
+      "function getter() { return 'getter'; };\n"
+      "function setter() { return 'setter'; }\n"
+      "Object.defineProperty(this, 'js_accessor_p', {get:getter, set:setter})");
+
+  Local<Value> getter = global0->Get(v8_str("getter"));
+  Local<Value> setter = global0->Get(v8_str("setter"));
+
   v8::HandleScope scope1;
 
   v8::Persistent<Context> context1 = Context::New();
@@ -5387,19 +5398,89 @@ THREADED_TEST(AccessControl) {
   v8::Handle<v8::Object> global1 = context1->Global();
   global1->Set(v8_str("other"), global0);
 
-  v8::Handle<Value> value;
-
   // Access blocked property
-  value = CompileRun("other.blocked_prop = 1");
-  value = CompileRun("other.blocked_prop");
-  CHECK(value->IsUndefined());
-
-  value = CompileRun(
+  CompileRun("other.blocked_prop = 1");
+
+  ExpectUndefined("other.blocked_prop");
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'blocked_prop')");
+  ExpectFalse("propertyIsEnumerable.call(other, 'blocked_prop')");
+
+  // Enable ACCESS_HAS
+  allowed_access_type[v8::ACCESS_HAS] = true;
+  ExpectUndefined("other.blocked_prop");
+  // ... and now we can get the descriptor...
+  ExpectUndefined(
       "Object.getOwnPropertyDescriptor(other, 'blocked_prop').value");
-  CHECK(value->IsUndefined());
+  // ... and enumerate the property.
+  ExpectTrue("propertyIsEnumerable.call(other, 'blocked_prop')");
+  allowed_access_type[v8::ACCESS_HAS] = false;
+
+  CompileRun("other.js_accessor_p = 2");
+
+  ExpectUndefined("other.js_accessor_p");
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p')");
+
+  // Enable ACCESS_HAS.
+  allowed_access_type[v8::ACCESS_HAS] = true;
+  ExpectUndefined("other.js_accessor_p");
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').get");
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').set");
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').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.js_accessor_p", "getter");
+  ExpectObject(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').get", getter);
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').set");
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').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.js_accessor_p");
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').get");
+  ExpectObject(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').set", setter);
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').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.js_accessor_p", "getter");
+  ExpectObject(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').get", getter);
+  ExpectObject(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').set", setter);
+  ExpectUndefined(
+      "Object.getOwnPropertyDescriptor(other, 'js_accessor_p').value");
+
+  allowed_access_type[v8::ACCESS_SET] = false;
+  allowed_access_type[v8::ACCESS_GET] = false;
+  allowed_access_type[v8::ACCESS_HAS] = false;
 
-  value = CompileRun("propertyIsEnumerable.call(other, 'blocked_prop')");
-  CHECK(value->IsFalse());
+  v8::Handle<Value> value;
 
   // Access accessible property
   value = CompileRun("other.accessible_prop = 3");