From 710fbd2cfbf7acb705c8b10de8b96897116f2574 Mon Sep 17 00:00:00 2001 From: "antonm@chromium.org" Date: Thu, 3 Feb 2011 18:09:51 +0000 Subject: [PATCH] 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 | 33 ++++++++++++---- test/cctest/test-api.cc | 86 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 10 deletions(-) diff --git a/src/runtime.cc b/src/runtime.cc index 8e1a3cb94e..239ba082b2 100644 --- a/src/runtime.cc +++ b/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 js_value = Handle::cast(obj); Handle str(String::cast(js_value->value())); - Handle substr = SubString(str, index, index+1, NOT_TENURED); + Handle 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 element = GetElement(Handle(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 holder = obj; if (obj->IsJSGlobalProxy()) { Object* proto = obj->GetPrototype(); if (proto->IsNull()) return Heap::undefined_value(); ASSERT(proto->IsJSGlobalObject()); - obj = Handle(JSObject::cast(proto)); + holder = Handle(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: diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index a201851181..afc973b0b4 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -5323,7 +5323,8 @@ static bool IndexedAccessBlocker(Local global, uint32_t key, v8::AccessType type, Local data) { - return Context::GetCurrent()->Global()->Equals(global); + return Context::GetCurrent()->Global()->Equals(global) || + allowed_access_type[type]; } @@ -5390,6 +5391,18 @@ TEST(AccessControl) { Local getter = global0->Get(v8_str("getter")); Local 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 el_getter = global0->Get(v8_str("el_getter")); + Local el_setter = global0->Get(v8_str("el_setter")); + v8::HandleScope scope1; v8::Persistent context1 = Context::New(); @@ -5398,7 +5411,7 @@ TEST(AccessControl) { v8::Handle 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; // Access accessible property