From 2013421859f8f18a841bbea537c2f205ae7f6edf Mon Sep 17 00:00:00 2001 From: "ager@chromium.org" Date: Thu, 30 Oct 2008 12:51:06 +0000 Subject: [PATCH] Add support for API accessors that prohibit overwriting by accessors defined in JavaScript code by using __defineGetter__ and __defineSetter__. Also, disable access checks when configuring objects created from templates. Review URL: http://codereview.chromium.org/8914 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@656 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- include/v8.h | 13 +++++++-- src/api.cc | 3 +- src/apinatives.js | 18 ++++++++---- src/bootstrapper.cc | 2 +- src/factory.cc | 2 +- src/objects-inl.h | 24 +++++++++++++++ src/objects.cc | 32 ++++++++++++++++++-- src/objects.h | 18 ++++++------ src/runtime.cc | 17 +++++++++++ src/runtime.h | 2 ++ test/cctest/test-api.cc | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 184 insertions(+), 24 deletions(-) diff --git a/include/v8.h b/include/v8.h index 70b1f52..ec51ce6 100644 --- a/include/v8.h +++ b/include/v8.h @@ -1301,11 +1301,18 @@ typedef Handle (*IndexedPropertyEnumerator)(const AccessorInfo& info); * Some accessors should be accessible across contexts. These * accessors have an explicit access control parameter which specifies * the kind of cross-context access that should be allowed. + * + * Additionally, for security, accessors can prohibit overwriting by + * accessors defined in JavaScript. For objects that have such + * accessors either locally or in their prototype chain it is not + * possible to overwrite the accessor by using __defineGetter__ or + * __defineSetter__ from JavaScript code. */ enum AccessControl { - DEFAULT = 0, - ALL_CAN_READ = 1, - ALL_CAN_WRITE = 2 + DEFAULT = 0, + ALL_CAN_READ = 1, + ALL_CAN_WRITE = 1 << 1, + PROHIBITS_OVERWRITING = 1 << 2 }; diff --git a/src/api.cc b/src/api.cc index 3bac759..64326a7 100644 --- a/src/api.cc +++ b/src/api.cc @@ -701,6 +701,7 @@ void FunctionTemplate::AddInstancePropertyAccessor( obj->set_name(*Utils::OpenHandle(*name)); if (settings & ALL_CAN_READ) obj->set_all_can_read(true); if (settings & ALL_CAN_WRITE) obj->set_all_can_write(true); + if (settings & PROHIBITS_OVERWRITING) obj->set_prohibits_overwriting(true); obj->set_property_attributes(static_cast(attributes)); i::Handle list(Utils::OpenHandle(this)->property_accessors()); @@ -1915,7 +1916,7 @@ void v8::Object::TurnOnAccessCheck() { i::Handle new_map = i::Factory::CopyMapDropTransitions(i::Handle(obj->map())); - new_map->set_is_access_check_needed(); + new_map->set_is_access_check_needed(true); obj->set_map(*new_map); } diff --git a/src/apinatives.js b/src/apinatives.js index 8741f59..8ae80e7 100644 --- a/src/apinatives.js +++ b/src/apinatives.js @@ -82,12 +82,18 @@ function InstantiateFunction(data, name) { function ConfigureTemplateInstance(obj, data) { var properties = %GetTemplateField(data, kApiPropertyListOffset); if (properties) { - for (var i = 0; i < properties[0]; i += 3) { - var name = properties[i + 1]; - var prop_data = properties[i + 2]; - var attributes = properties[i + 3]; - var value = Instantiate(prop_data, name); - %SetProperty(obj, name, value, attributes); + // Disable access checks while instantiating the object. + var requires_access_checks = %DisableAccessChecks(obj); + try { + for (var i = 0; i < properties[0]; i += 3) { + var name = properties[i + 1]; + var prop_data = properties[i + 2]; + var attributes = properties[i + 3]; + var value = Instantiate(prop_data, name); + %SetProperty(obj, name, value, attributes); + } + } finally { + if (requires_access_checks) %EnableAccessChecks(obj); } } } diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 0562c23..f8eb658 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -597,7 +597,7 @@ void Genesis::CreateRoots(v8::Handle global_template, Handle global_name = Factory::LookupAsciiSymbol("global"); global_proxy_function->shared()->set_instance_class_name(*global_name); - global_proxy_function->initial_map()->set_is_access_check_needed(); + global_proxy_function->initial_map()->set_is_access_check_needed(true); // Set global_proxy.__proto__ to js_global after ConfigureGlobalObjects diff --git a/src/factory.cc b/src/factory.cc index 4b0faef..8b3947f 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -725,7 +725,7 @@ Handle Factory::CreateApiFunction( // Mark as needs_access_check if needed. if (obj->needs_access_check()) { - map->set_is_access_check_needed(); + map->set_is_access_check_needed(true); } // Set interceptor information in the map. diff --git a/src/objects-inl.h b/src/objects-inl.h index 03448fd..4d37a6d 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -1677,6 +1677,20 @@ bool Map::has_non_instance_prototype() { } +void Map::set_is_access_check_needed(bool access_check_needed) { + if (access_check_needed) { + set_bit_field(bit_field() | (1 << kIsAccessCheckNeeded)); + } else { + set_bit_field(bit_field() & ~(1 << kIsAccessCheckNeeded)); + } +} + + +bool Map::is_access_check_needed() { + return ((1 << kIsAccessCheckNeeded) & bit_field()) != 0; +} + + Code::Flags Code::flags() { return static_cast(READ_INT_FIELD(this, kFlagsOffset)); } @@ -2298,6 +2312,16 @@ void AccessorInfo::set_all_can_write(bool value) { } +bool AccessorInfo::prohibits_overwriting() { + return BooleanBit::get(flag(), kProhibitsOverwritingBit); +} + + +void AccessorInfo::set_prohibits_overwriting(bool value) { + set_flag(BooleanBit::set(flag(), kProhibitsOverwritingBit, value)); +} + + PropertyAttributes AccessorInfo::property_attributes() { return AttributesField::decode(static_cast(flag()->value())); } diff --git a/src/objects.cc b/src/objects.cc index 68dd330..dea10cb 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -2257,9 +2257,19 @@ void JSObject::Lookup(String* name, LookupResult* result) { current != Heap::null_value(); current = JSObject::cast(current)->GetPrototype()) { JSObject::cast(current)->LocalLookup(name, result); - if (result->IsValid() && !result->IsTransitionType()) { - return; - } + if (result->IsValid() && !result->IsTransitionType()) return; + } + result->NotFound(); +} + + +// Search object and it's prototype chain for callback properties. +void JSObject::LookupCallback(String* name, LookupResult* result) { + for (Object* current = this; + current != Heap::null_value(); + current = JSObject::cast(current)->GetPrototype()) { + JSObject::cast(current)->LocalLookupRealNamedProperty(name, result); + if (result->IsValid() && result->type() == CALLBACKS) return; } result->NotFound(); } @@ -2285,6 +2295,22 @@ Object* JSObject::DefineGetterSetter(String* name, uint32_t index; if (name->AsArrayIndex(&index)) return Heap::undefined_value(); + // Check if there is an API defined callback object which prohibits + // callback overwriting in this object or it's prototype chain. + // This mechanism is needed for instance in a browser setting, where + // certain accessors such as window.location should not be allowed + // to be overwriten because allowing overwriting could potentially + // cause security problems. + LookupResult callback_result; + LookupCallback(name, &callback_result); + if (callback_result.IsValid()) { + Object* obj = callback_result.GetCallbackObject(); + if (obj->IsAccessorInfo() && + AccessorInfo::cast(obj)->prohibits_overwriting()) { + return Heap::undefined_value(); + } + } + // Lookup the name. LookupResult result; LocalLookup(name, &result); diff --git a/src/objects.h b/src/objects.h index f2f9e18..f276ce3 100644 --- a/src/objects.h +++ b/src/objects.h @@ -1280,6 +1280,7 @@ class JSObject: public HeapObject { void LookupRealNamedProperty(String* name, LookupResult* result); void LookupRealNamedPropertyInPrototypes(String* name, LookupResult* result); void LookupCallbackSetterInPrototypes(String* name, LookupResult* result); + void LookupCallback(String* name, LookupResult* result); // Returns the number of properties on this object filtering out properties // with the specified attributes (ignoring interceptors). @@ -2364,13 +2365,8 @@ class Map: public HeapObject { // Tells whether the instance needs security checks when accessing its // properties. - inline void set_is_access_check_needed() { - set_bit_field(bit_field() | (1 << kIsAccessCheckNeeded)); - } - - inline bool is_access_check_needed() { - return ((1 << kIsAccessCheckNeeded) & bit_field()) != 0; - } + inline void set_is_access_check_needed(bool access_check_needed); + inline bool is_access_check_needed(); // [prototype]: implicit prototype object. DECL_ACCESSORS(prototype, Object) @@ -3717,6 +3713,9 @@ class AccessorInfo: public Struct { inline bool all_can_write(); inline void set_all_can_write(bool value); + inline bool prohibits_overwriting(); + inline void set_prohibits_overwriting(bool value); + inline PropertyAttributes property_attributes(); inline void set_property_attributes(PropertyAttributes attributes); @@ -3736,9 +3735,10 @@ class AccessorInfo: public Struct { private: // Bit positions in flag. - static const int kAllCanReadBit = 0; + static const int kAllCanReadBit = 0; static const int kAllCanWriteBit = 1; - class AttributesField: public BitField {}; + static const int kProhibitsOverwritingBit = 2; + class AttributesField: public BitField {}; DISALLOW_IMPLICIT_CONSTRUCTORS(AccessorInfo); }; diff --git a/src/runtime.cc b/src/runtime.cc index 8459af1..a38d164 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -336,6 +336,23 @@ static Object* Runtime_GetTemplateField(Arguments args) { } +static Object* Runtime_DisableAccessChecks(Arguments args) { + ASSERT(args.length() == 1); + CONVERT_CHECKED(HeapObject, object, args[0]); + bool needs_access_checks = object->map()->is_access_check_needed(); + object->map()->set_is_access_check_needed(false); + return needs_access_checks ? Heap::true_value() : Heap::false_value(); +} + + +static Object* Runtime_EnableAccessChecks(Arguments args) { + ASSERT(args.length() == 1); + CONVERT_CHECKED(HeapObject, object, args[0]); + object->map()->set_is_access_check_needed(true); + return Heap::undefined_value(); +} + + static Object* ThrowRedeclarationError(const char* type, Handle name) { HandleScope scope; Handle type_handle = Factory::NewStringFromAscii(CStrVector(type)); diff --git a/src/runtime.h b/src/runtime.h index 25931d9..6f21dba 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -179,6 +179,8 @@ namespace v8 { namespace internal { F(CreateApiFunction, 1) \ F(IsTemplate, 1) \ F(GetTemplateField, 2) \ + F(DisableAccessChecks, 1) \ + F(EnableAccessChecks, 1) \ \ /* Dates */ \ F(DateCurrentTime, 0) \ diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 0b4f6e5..8268960 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -5075,3 +5075,80 @@ THREADED_TEST(PropertyEnumeration) { const char* elmv3[] = {"w", "z", "x", "y"}; CheckProperties(elms->Get(v8::Integer::New(3)), elmc3, elmv3); } + + +static v8::Handle AccessorProhibitsOverwritingGetter( + Local name, + const AccessorInfo& info) { + ApiTestFuzzer::Fuzz(); + return v8::True(); +} + + +THREADED_TEST(AccessorProhibitsOverwriting) { + v8::HandleScope scope; + LocalContext context; + Local templ = ObjectTemplate::New(); + templ->SetAccessor(v8_str("x"), + AccessorProhibitsOverwritingGetter, + 0, + v8::Handle(), + v8::PROHIBITS_OVERWRITING, + v8::ReadOnly); + Local instance = templ->NewInstance(); + context->Global()->Set(v8_str("obj"), instance); + Local value = CompileRun( + "obj.__defineGetter__('x', function() { return false; });" + "obj.x"); + CHECK(value->BooleanValue()); + value = CompileRun( + "var setter_called = false;" + "obj.__defineSetter__('x', function() { setter_called = true; });" + "obj.x = 42;" + "setter_called"); + CHECK(!value->BooleanValue()); + value = CompileRun( + "obj2 = {};" + "obj2.__proto__ = obj;" + "obj2.__defineGetter__('x', function() { return false; });" + "obj2.x"); + CHECK(value->BooleanValue()); + value = CompileRun( + "var setter_called = false;" + "obj2 = {};" + "obj2.__proto__ = obj;" + "obj2.__defineSetter__('x', function() { setter_called = true; });" + "obj2.x = 42;" + "setter_called"); + CHECK(!value->BooleanValue()); +} + + +static bool NamedSetAccessBlocker(Local obj, + Local name, + v8::AccessType type, + Local data) { + return type != v8::ACCESS_SET; +} + + +static bool IndexedSetAccessBlocker(Local obj, + uint32_t key, + v8::AccessType type, + Local data) { + return type != v8::ACCESS_SET; +} + + +THREADED_TEST(DisableAccessChecksWhileConfiguring) { + v8::HandleScope scope; + LocalContext context; + Local templ = ObjectTemplate::New(); + templ->SetAccessCheckCallbacks(NamedSetAccessBlocker, + IndexedSetAccessBlocker); + templ->Set(v8_str("x"), v8::True()); + Local instance = templ->NewInstance(); + context->Global()->Set(v8_str("obj"), instance); + Local value = CompileRun("obj.x"); + CHECK(value->BooleanValue()); +} -- 2.7.4