Revision: 6592
Author: [email protected]
Date: Wed Feb 2 09:44:29 2011
Log: Better security checks when accessing named properties via
Object.getOwnPropertyDescriptor.
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
http://code.google.com/p/v8/source/detail?r=6592
Modified:
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/test/cctest/test-api.cc
=======================================
--- /branches/bleeding_edge/src/runtime.cc Wed Feb 2 05:31:52 2011
+++ /branches/bleeding_edge/src/runtime.cc Wed Feb 2 09:44:29 2011
@@ -642,6 +642,77 @@
name, result);
}
}
+
+
+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
@@ -745,6 +816,10 @@
if (!result.IsProperty()) {
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 @@
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;
}
=======================================
--- /branches/bleeding_edge/test/cctest/test-api.cc Wed Feb 2 05:31:52 2011
+++ /branches/bleeding_edge/test/cctest/test-api.cc Wed Feb 2 09:44:29 2011
@@ -5309,11 +5309,13 @@
}
+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 @@
}
-THREADED_TEST(AccessControl) {
+TEST(AccessControl) {
v8::HandleScope handle_scope;
v8::Handle<v8::ObjectTemplate> global_template =
v8::ObjectTemplate::New();
@@ -5379,6 +5381,15 @@
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 @@
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());
-
- value = CompileRun("propertyIsEnumerable.call(other, 'blocked_prop')");
- CHECK(value->IsFalse());
+ // ... 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;
+
+ v8::Handle<Value> value;
// Access accessible property
value = CompileRun("other.accessible_prop = 3");
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
