Add access check for observed objects
authoradamk@chromium.org <adamk@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Wed, 11 Sep 2013 20:03:54 +0000 (20:03 +0000)
committeradamk@chromium.org <adamk@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Wed, 11 Sep 2013 20:03:54 +0000 (20:03 +0000)
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
R=rossberg@chromium.org

Review URL: https://codereview.chromium.org/22962009

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@16663 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/object-observe.js
src/runtime.cc
src/runtime.h
test/cctest/test-object-observe.cc

index a9c29cf..1035792 100644 (file)
@@ -367,13 +367,22 @@ function ArrayUnobserve(object, callback) {
   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 @@ function ObserverEnqueueIfActive(observer, objectInfo, changeRecord) {
   %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 @@ function ObjectNotifierNotify(changeRecord) {
   }
   ObjectFreeze(newRecord);
 
-  ObjectInfoEnqueueChangeRecord(objectInfo, newRecord);
+  ObjectInfoEnqueueChangeRecord(objectInfo, newRecord,
+                                true /* skip access check */);
 }
 
 function ObjectNotifierPerformChange(changeType, changeFn) {
index 9abcbc9..00efa51 100644 (file)
@@ -14505,6 +14505,14 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_HaveSameMap) {
 }
 
 
+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) {
   SealHandleScope shs(isolate);
   ASSERT(args.length() == 1);
@@ -14582,6 +14590,34 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_UnwrapGlobalProxy) {
 }
 
 
+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,
                                            Handle<JSFunction> constructor,
                                            Handle<Object> type_info,
index d056880..bce09a4 100644 (file)
@@ -358,6 +358,7 @@ namespace internal {
   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 @@ namespace internal {
   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
index e7cf1e5..b129ff3 100644 (file)
@@ -313,11 +313,13 @@ static void ExpectRecords(Handle<Value> records,
         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 @@ TEST(ObservationWeakMap) {
   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());
+}