From fed562267100a0fef92b5f95b839f608142e9531 Mon Sep 17 00:00:00 2001 From: "antonm@chromium.org" Date: Wed, 2 Feb 2011 17:44:29 +0000 Subject: [PATCH] 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 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6592 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/runtime.cc | 87 +++++++++++++++++++++++++++++++-- test/cctest/test-api.cc | 103 +++++++++++++++++++++++++++++++++++----- 2 files changed, 176 insertions(+), 14 deletions(-) diff --git a/src/runtime.cc b/src/runtime.cc index d55a201e59..9911e1e3a3 100644 --- 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 ee620678a7..b04420ed9b 100644 --- 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 global, Local name, v8::AccessType type, Local 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, Local, } -THREADED_TEST(AccessControl) { +TEST(AccessControl) { v8::HandleScope handle_scope; v8::Handle global_template = v8::ObjectTemplate::New(); @@ -5379,6 +5381,15 @@ THREADED_TEST(AccessControl) { v8::Handle 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 getter = global0->Get(v8_str("getter")); + Local setter = global0->Get(v8_str("setter")); + v8::HandleScope scope1; v8::Persistent context1 = Context::New(); @@ -5387,19 +5398,89 @@ THREADED_TEST(AccessControl) { v8::Handle global1 = context1->Global(); global1->Set(v8_str("other"), global0); - v8::Handle value; - // Access blocked property - value = CompileRun("other.blocked_prop = 1"); - value = CompileRun("other.blocked_prop"); - CHECK(value->IsUndefined()); + CompileRun("other.blocked_prop = 1"); - value = CompileRun( + 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; - value = CompileRun("propertyIsEnumerable.call(other, 'blocked_prop')"); - CHECK(value->IsFalse()); + 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; // Access accessible property value = CompileRun("other.accessible_prop = 3");