Revision: 16663
Author: [email protected]
Date: Wed Sep 11 20:03:54 2013 UTC
Log: Add access check for observed objects
This change is mostly straightforward: for 'normal' sorts of change records,
simply don't deliver a changeRecord to a given observer callback if an
access
the callback's Context is not allowed to "GET" or "HAS" changeRecord.name on
changeRecord.object, or if ACCESS_KEYS is disallowed.
For 'splice' records, the question of whether to hand it to an observer is
trickier, since
there are multiple properties involved, and multiple types of possible
information leakage.
Given that access-checked objects are very rare (only two in Blink, Window
and Location),
and that they are not normally used as Arrays, it seems better to simply
not emit any splice
records for such objects rather than spending lots of logic to attempt to
avoid information
leakage for something that may never happen.
BUG=v8:2778
[email protected]
Review URL: https://codereview.chromium.org/22962009
http://code.google.com/p/v8/source/detail?r=16663
Modified:
/branches/bleeding_edge/src/object-observe.js
/branches/bleeding_edge/src/runtime.cc
/branches/bleeding_edge/src/runtime.h
/branches/bleeding_edge/test/cctest/test-object-observe.cc
=======================================
--- /branches/bleeding_edge/src/object-observe.js Wed Sep 11 10:52:20 2013
UTC
+++ /branches/bleeding_edge/src/object-observe.js Wed Sep 11 20:03:54 2013
UTC
@@ -367,13 +367,22 @@
return ObjectUnobserve(object, callback);
}
-function ObserverEnqueueIfActive(observer, objectInfo, changeRecord) {
+function ObserverEnqueueIfActive(observer, objectInfo, changeRecord,
+ needsAccessCheck) {
if (!ObserverIsActive(observer, objectInfo) ||
!TypeMapHasType(ObserverGetAcceptTypes(observer),
changeRecord.type)) {
return;
}
var callback = ObserverGetCallback(observer);
+ if (needsAccessCheck &&
+ // Drop all splice records on the floor for access-checked objects
+ (changeRecord.type == 'splice' ||
+ !%IsAccessAllowedForObserver(
+ callback, changeRecord.object, changeRecord.name))) {
+ return;
+ }
+
var callbackInfo = CallbackInfoNormalize(callback);
if (!observationState.pendingObservers)
observationState.pendingObservers = { __proto__: null };
@@ -382,19 +391,25 @@
%SetObserverDeliveryPending();
}
-function ObjectInfoEnqueueChangeRecord(objectInfo, changeRecord) {
+function ObjectInfoEnqueueChangeRecord(objectInfo, changeRecord,
+ skipAccessCheck) {
// TODO(rossberg): adjust once there is a story for symbols vs proxies.
if (IS_SYMBOL(changeRecord.name)) return;
+ var needsAccessCheck = !skipAccessCheck &&
+ %IsAccessCheckNeeded(changeRecord.object);
+
if (ChangeObserversIsOptimized(objectInfo.changeObservers)) {
var observer = objectInfo.changeObservers;
- ObserverEnqueueIfActive(observer, objectInfo, changeRecord);
+ ObserverEnqueueIfActive(observer, objectInfo, changeRecord,
+ needsAccessCheck);
return;
}
for (var priority in objectInfo.changeObservers) {
var observer = objectInfo.changeObservers[priority];
- ObserverEnqueueIfActive(observer, objectInfo, changeRecord);
+ ObserverEnqueueIfActive(observer, objectInfo, changeRecord,
+ needsAccessCheck);
}
}
@@ -463,7 +478,8 @@
}
ObjectFreeze(newRecord);
- ObjectInfoEnqueueChangeRecord(objectInfo, newRecord);
+ ObjectInfoEnqueueChangeRecord(objectInfo, newRecord,
+ true /* skip access check */);
}
function ObjectNotifierPerformChange(changeType, changeFn) {
=======================================
--- /branches/bleeding_edge/src/runtime.cc Wed Sep 11 15:16:56 2013 UTC
+++ /branches/bleeding_edge/src/runtime.cc Wed Sep 11 20:03:54 2013 UTC
@@ -14503,6 +14503,14 @@
CONVERT_ARG_CHECKED(JSObject, obj2, 1);
return isolate->heap()->ToBoolean(obj1->map() == obj2->map());
}
+
+
+RUNTIME_FUNCTION(MaybeObject*, Runtime_IsAccessCheckNeeded) {
+ SealHandleScope shs(isolate);
+ ASSERT(args.length() == 1);
+ CONVERT_ARG_CHECKED(HeapObject, obj, 0);
+ return isolate->heap()->ToBoolean(obj->IsAccessCheckNeeded());
+}
RUNTIME_FUNCTION(MaybeObject*, Runtime_IsObserved) {
@@ -14580,6 +14588,34 @@
}
return object;
}
+
+
+RUNTIME_FUNCTION(MaybeObject*, Runtime_IsAccessAllowedForObserver) {
+ HandleScope scope(isolate);
+ ASSERT(args.length() == 3);
+ CONVERT_ARG_HANDLE_CHECKED(JSFunction, observer, 0);
+ CONVERT_ARG_HANDLE_CHECKED(JSObject, object, 1);
+ ASSERT(object->IsAccessCheckNeeded());
+ Handle<Object> key = args.at<Object>(2);
+ SaveContext save(isolate);
+ isolate->set_context(observer->context());
+ if (!isolate->MayNamedAccess(*object, isolate->heap()->undefined_value(),
+ v8::ACCESS_KEYS)) {
+ return isolate->heap()->false_value();
+ }
+ bool access_allowed = false;
+ uint32_t index = 0;
+ if (key->ToArrayIndex(&index) ||
+ (key->IsString() && String::cast(*key)->AsArrayIndex(&index))) {
+ access_allowed =
+ isolate->MayIndexedAccess(*object, index, v8::ACCESS_GET) &&
+ isolate->MayIndexedAccess(*object, index, v8::ACCESS_HAS);
+ } else {
+ access_allowed = isolate->MayNamedAccess(*object, *key,
v8::ACCESS_GET) &&
+ isolate->MayNamedAccess(*object, *key, v8::ACCESS_HAS);
+ }
+ return isolate->heap()->ToBoolean(access_allowed);
+}
static MaybeObject* ArrayConstructorCommon(Isolate* isolate,
=======================================
--- /branches/bleeding_edge/src/runtime.h Wed Sep 11 12:39:00 2013 UTC
+++ /branches/bleeding_edge/src/runtime.h Wed Sep 11 20:03:54 2013 UTC
@@ -358,6 +358,7 @@
F(GetObservationState, 0, 1) \
F(ObservationWeakMapCreate, 0, 1) \
F(UnwrapGlobalProxy, 1, 1) \
+ F(IsAccessAllowedForObserver, 3, 1) \
\
/* Harmony typed arrays */ \
F(ArrayBufferInitialize, 2, 1)\
@@ -469,7 +470,8 @@
F(HasExternalDoubleElements, 1, 1) \
F(HasFastProperties, 1, 1) \
F(TransitionElementsKind, 2, 1) \
- F(HaveSameMap, 2, 1)
+ F(HaveSameMap, 2, 1) \
+ F(IsAccessCheckNeeded, 1, 1)
#ifdef ENABLE_DEBUGGER_SUPPORT
=======================================
--- /branches/bleeding_edge/test/cctest/test-object-observe.cc Tue Sep 10
18:13:54 2013 UTC
+++ /branches/bleeding_edge/test/cctest/test-object-observe.cc Wed Sep 11
20:03:54 2013 UTC
@@ -313,11 +313,13 @@
recordObj->Get(String::New("object"))));
CHECK(String::New(expectations[i].type)->Equals(
recordObj->Get(String::New("type"))));
- CHECK(String::New(expectations[i].name)->Equals(
- recordObj->Get(String::New("name"))));
- if (!expectations[i].old_value.IsEmpty()) {
- CHECK(expectations[i].old_value->Equals(
- recordObj->Get(String::New("oldValue"))));
+ if (strcmp("splice", expectations[i].type) != 0) {
+ CHECK(String::New(expectations[i].name)->Equals(
+ recordObj->Get(String::New("name"))));
+ if (!expectations[i].old_value.IsEmpty()) {
+ CHECK(expectations[i].old_value->Equals(
+ recordObj->Get(String::New("oldValue"))));
+ }
}
}
}
@@ -446,3 +448,270 @@
CHECK_EQ(0, NumberOfElements(objectInfoMap));
CHECK_EQ(0, NumberOfElements(notifierObjectInfoMap));
}
+
+
+static bool NamedAccessAlwaysAllowed(Local<Object>, Local<Value>,
AccessType,
+ Local<Value>) {
+ return true;
+}
+
+
+static bool IndexedAccessAlwaysAllowed(Local<Object>, uint32_t, AccessType,
+ Local<Value>) {
+ return true;
+}
+
+
+static AccessType g_access_block_type = ACCESS_GET;
+
+
+static bool NamedAccessAllowUnlessBlocked(Local<Object> host,
+ Local<Value> key,
+ AccessType type,
+ Local<Value>) {
+ if (type != g_access_block_type) return true;
+ Handle<Object> global = Context::GetCurrent()->Global();
+ Handle<Value> blacklist = global->Get(String::New("blacklist"));
+ if (!blacklist->IsObject()) return true;
+ if (key->IsString()) return !blacklist.As<Object>()->Has(key);
+ return true;
+}
+
+
+static bool IndexedAccessAllowUnlessBlocked(Local<Object> host,
+ uint32_t index,
+ AccessType type,
+ Local<Value>) {
+ if (type != ACCESS_GET) return true;
+ Handle<Object> global = Context::GetCurrent()->Global();
+ Handle<Value> blacklist = global->Get(String::New("blacklist"));
+ if (!blacklist->IsObject()) return true;
+ return !blacklist.As<Object>()->Has(index);
+}
+
+
+static bool BlockAccessKeys(Local<Object> host, Local<Value> key,
+ AccessType type, Local<Value>) {
+ Handle<Object> global = Context::GetCurrent()->Global();
+ Handle<Value> blacklist = global->Get(String::New("blacklist"));
+ if (!blacklist->IsObject()) return true;
+ return type != ACCESS_KEYS ||
+ !blacklist.As<Object>()->Has(String::New("__block_access_keys"));
+}
+
+
+static Handle<Object> CreateAccessCheckedObject(
+ NamedSecurityCallback namedCallback,
+ IndexedSecurityCallback indexedCallback) {
+ Handle<ObjectTemplate> tmpl = ObjectTemplate::New();
+ tmpl->SetAccessCheckCallbacks(namedCallback, indexedCallback);
+ Handle<Object> instance = tmpl->NewInstance();
+ instance->CreationContext()->Global()->Set(String::New("obj"), instance);
+ return instance;
+}
+
+
+TEST(NamedAccessCheck) {
+ HarmonyIsolate isolate;
+ const AccessType types[] = { ACCESS_GET, ACCESS_HAS };
+ for (size_t i = 0; i < ARRAY_SIZE(types); ++i) {
+ HandleScope scope(isolate.GetIsolate());
+ LocalContext context;
+ g_access_block_type = types[i];
+ Handle<Object> instance = CreateAccessCheckedObject(
+ NamedAccessAllowUnlessBlocked, IndexedAccessAlwaysAllowed);
+ CompileRun("var records = null;"
+ "var objNoCheck = {};"
+ "var blacklist = {foo: true};"
+ "var observer = function(r) { records = r };"
+ "Object.observe(obj, observer);"
+ "Object.observe(objNoCheck, observer);");
+ Handle<Value> obj_no_check = CompileRun("objNoCheck");
+ {
+ LocalContext context2;
+ context2->Global()->Set(String::New("obj"), instance);
+ context2->Global()->Set(String::New("objNoCheck"), obj_no_check);
+ CompileRun("var records2 = null;"
+ "var observer2 = function(r) { records2 = r };"
+ "Object.observe(obj, observer2);"
+ "Object.observe(objNoCheck, observer2);"
+ "obj.foo = 'bar';"
+ "Object.defineProperty(obj, 'foo', {value: 5});"
+ "Object.defineProperty(obj, 'foo', {get: function(){}});"
+ "obj.bar = 'baz';"
+ "objNoCheck.baz = 'quux'");
+ const RecordExpectation expected_records2[] = {
+ { instance, "new", "foo", Handle<Value>() },
+ { instance, "updated", "foo", String::New("bar") },
+ { instance, "reconfigured", "foo", Number::New(5) },
+ { instance, "new", "bar", Handle<Value>() },
+ { obj_no_check, "new", "baz", Handle<Value>() },
+ };
+ EXPECT_RECORDS(CompileRun("records2"), expected_records2);
+ }
+ const RecordExpectation expected_records[] = {
+ { instance, "new", "bar", Handle<Value>() },
+ { obj_no_check, "new", "baz", Handle<Value>() }
+ };
+ EXPECT_RECORDS(CompileRun("records"), expected_records);
+ }
+}
+
+
+TEST(IndexedAccessCheck) {
+ HarmonyIsolate isolate;
+ const AccessType types[] = { ACCESS_GET, ACCESS_HAS };
+ for (size_t i = 0; i < ARRAY_SIZE(types); ++i) {
+ HandleScope scope(isolate.GetIsolate());
+ LocalContext context;
+ g_access_block_type = types[i];
+ Handle<Object> instance = CreateAccessCheckedObject(
+ NamedAccessAlwaysAllowed, IndexedAccessAllowUnlessBlocked);
+ CompileRun("var records = null;"
+ "var objNoCheck = {};"
+ "var blacklist = {7: true};"
+ "var observer = function(r) { records = r };"
+ "Object.observe(obj, observer);"
+ "Object.observe(objNoCheck, observer);");
+ Handle<Value> obj_no_check = CompileRun("objNoCheck");
+ {
+ LocalContext context2;
+ context2->Global()->Set(String::New("obj"), instance);
+ context2->Global()->Set(String::New("objNoCheck"), obj_no_check);
+ CompileRun("var records2 = null;"
+ "var observer2 = function(r) { records2 = r };"
+ "Object.observe(obj, observer2);"
+ "Object.observe(objNoCheck, observer2);"
+ "obj[7] = 'foo';"
+ "Object.defineProperty(obj, '7', {value: 5});"
+ "Object.defineProperty(obj, '7', {get: function(){}});"
+ "obj[8] = 'bar';"
+ "objNoCheck[42] = 'quux'");
+ const RecordExpectation expected_records2[] = {
+ { instance, "new", "7", Handle<Value>() },
+ { instance, "updated", "7", String::New("foo") },
+ { instance, "reconfigured", "7", Number::New(5) },
+ { instance, "new", "8", Handle<Value>() },
+ { obj_no_check, "new", "42", Handle<Value>() }
+ };
+ EXPECT_RECORDS(CompileRun("records2"), expected_records2);
+ }
+ const RecordExpectation expected_records[] = {
+ { instance, "new", "8", Handle<Value>() },
+ { obj_no_check, "new", "42", Handle<Value>() }
+ };
+ EXPECT_RECORDS(CompileRun("records"), expected_records);
+ }
+}
+
+
+TEST(SpliceAccessCheck) {
+ HarmonyIsolate isolate;
+ HandleScope scope(isolate.GetIsolate());
+ LocalContext context;
+ g_access_block_type = ACCESS_GET;
+ Handle<Object> instance = CreateAccessCheckedObject(
+ NamedAccessAlwaysAllowed, IndexedAccessAllowUnlessBlocked);
+ CompileRun("var records = null;"
+ "obj[1] = 'foo';"
+ "obj.length = 2;"
+ "var objNoCheck = {1: 'bar', length: 2};"
+ "var blacklist = {1: true};"
+ "observer = function(r) { records = r };"
+ "Array.observe(obj, observer);"
+ "Array.observe(objNoCheck, observer);");
+ Handle<Value> obj_no_check = CompileRun("objNoCheck");
+ {
+ LocalContext context2;
+ context2->Global()->Set(String::New("obj"), instance);
+ context2->Global()->Set(String::New("objNoCheck"), obj_no_check);
+ CompileRun("var records2 = null;"
+ "var observer2 = function(r) { records2 = r };"
+ "Array.observe(obj, observer2);"
+ "Array.observe(objNoCheck, observer2);"
+ // No one should hear about this: no splice records are
emitted
+ // for access-checked objects
+ "[].push.call(obj, 5);"
+ "[].splice.call(obj, 1, 1);"
+ "[].pop.call(obj);"
+ "[].pop.call(objNoCheck);");
+ // TODO(adamk): Extend EXPECT_RECORDS to be able to assert more things
+ // about splice records. For this test it's not so important since
+ // we just want to guarantee the machinery is in operation at all.
+ const RecordExpectation expected_records2[] = {
+ { obj_no_check, "splice", "", Handle<Value>() }
+ };
+ EXPECT_RECORDS(CompileRun("records2"), expected_records2);
+ }
+ const RecordExpectation expected_records[] = {
+ { obj_no_check, "splice", "", Handle<Value>() }
+ };
+ EXPECT_RECORDS(CompileRun("records"), expected_records);
+}
+
+
+TEST(DisallowAllForAccessKeys) {
+ HarmonyIsolate isolate;
+ HandleScope scope(isolate.GetIsolate());
+ LocalContext context;
+ Handle<Object> instance = CreateAccessCheckedObject(
+ BlockAccessKeys, IndexedAccessAlwaysAllowed);
+ CompileRun("var records = null;"
+ "var objNoCheck = {};"
+ "var observer = function(r) { records = r };"
+ "var blacklist = {__block_access_keys: true};"
+ "Object.observe(obj, observer);"
+ "Object.observe(objNoCheck, observer);");
+ Handle<Value> obj_no_check = CompileRun("objNoCheck");
+ {
+ LocalContext context2;
+ context2->Global()->Set(String::New("obj"), instance);
+ context2->Global()->Set(String::New("objNoCheck"), obj_no_check);
+ CompileRun("var records2 = null;"
+ "var observer2 = function(r) { records2 = r };"
+ "Object.observe(obj, observer2);"
+ "Object.observe(objNoCheck, observer2);"
+ "obj.foo = 'bar';"
+ "obj[5] = 'baz';"
+ "objNoCheck.baz = 'quux'");
+ const RecordExpectation expected_records2[] = {
+ { instance, "new", "foo", Handle<Value>() },
+ { instance, "new", "5", Handle<Value>() },
+ { obj_no_check, "new", "baz", Handle<Value>() },
+ };
+ EXPECT_RECORDS(CompileRun("records2"), expected_records2);
+ }
+ const RecordExpectation expected_records[] = {
+ { obj_no_check, "new", "baz", Handle<Value>() }
+ };
+ EXPECT_RECORDS(CompileRun("records"), expected_records);
+}
+
+
+TEST(AccessCheckDisallowApiModifications) {
+ HarmonyIsolate isolate;
+ HandleScope scope(isolate.GetIsolate());
+ LocalContext context;
+ Handle<Object> instance = CreateAccessCheckedObject(
+ BlockAccessKeys, IndexedAccessAlwaysAllowed);
+ CompileRun("var records = null;"
+ "var observer = function(r) { records = r };"
+ "var blacklist = {__block_access_keys: true};"
+ "Object.observe(obj, observer);");
+ {
+ LocalContext context2;
+ context2->Global()->Set(String::New("obj"), instance);
+ CompileRun("var records2 = null;"
+ "var observer2 = function(r) { records2 = r };"
+ "Object.observe(obj, observer2);");
+ instance->Set(5, String::New("bar"));
+ instance->Set(String::New("foo"), String::New("bar"));
+ CompileRun(""); // trigger delivery
+ const RecordExpectation expected_records2[] = {
+ { instance, "new", "5", Handle<Value>() },
+ { instance, "new", "foo", Handle<Value>() }
+ };
+ EXPECT_RECORDS(CompileRun("records2"), expected_records2);
+ }
+ CHECK(CompileRun("records")->IsNull());
+}
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
