add access checks to receivers on function callbacks

R=verwaest@chromium.org
BUG=468451
LOG=N

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

Cr-Commit-Position: refs/heads/master@{#27452}

diff --git a/src/builtins.cc b/src/builtins.cc
index 71867e1..f4d9aa2 100644 (file)
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -1044,7 +1044,9 @@ MUST_USE_RESULT static MaybeHandle<Object> HandleApiCallHelper(
   DCHECK(!args[0]->IsNull());
   if (args[0]->IsUndefined()) args[0] = function->global_proxy();
 
-  Object* raw_holder = fun_data->GetCompatibleReceiver(isolate, args[0]);
+  Handle<Object> receiver(&args[0]);
+  Handle<Object> raw_holder =
+      fun_data->GetCompatibleReceiver(isolate, receiver, is_construct);
 
   if (raw_holder->IsNull()) {
     // This function cannot be called with the given receiver.  Abort!
@@ -1066,12 +1068,8 @@ MUST_USE_RESULT static MaybeHandle<Object> HandleApiCallHelper(
     LOG(isolate, ApiObjectAccess("call", JSObject::cast(*args.receiver())));
     DCHECK(raw_holder->IsJSObject());
 
-    FunctionCallbackArguments custom(isolate,
-                                     data_obj,
-                                     *function,
-                                     raw_holder,
-                                     &args[0] - 1,
-                                     args.length() - 1,
+    FunctionCallbackArguments custom(isolate, data_obj, *function, *raw_holder,
+                                     &args[0] - 1, args.length() - 1,
                                      is_construct);
 
     v8::Handle<v8::Value> value = custom.Call(callback);

diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index 230939f..c4b446e 100644 (file)
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -8646,6 +8646,14 @@ bool HOptimizedGraphBuilder::TryInlineApiCall(Handle<JSFunction> function,
   CallOptimization optimization(function);
   if (!optimization.is_simple_api_call()) return false;
   Handle<Map> holder_map;
+  for (int i = 0; i < receiver_maps->length(); ++i) {
+    auto map = receiver_maps->at(i);
+    // Don't inline calls to receivers requiring accesschecks.
+    if (map->is_access_check_needed() &&
+        map->instance_type() != JS_GLOBAL_PROXY_TYPE) {
+      return false;
+    }
+  }
   if (call_type == kCallApiFunction) {
     // Cannot embed a direct reference to the global proxy map
     // as it maybe dropped on deserialization.

diff --git a/src/objects.cc b/src/objects.cc
index f876cda..1cfdf63 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -241,21 +241,28 @@ bool FunctionTemplateInfo::IsTemplateFor(Map* map) {
 
 
 // TODO(dcarney): CallOptimization duplicates this logic, merge.
-Object* FunctionTemplateInfo::GetCompatibleReceiver(Isolate* isolate,
-                                                    Object* receiver) {
+Handle<Object> FunctionTemplateInfo::GetCompatibleReceiver(
+    Isolate* isolate, Handle<Object> receiver, bool is_construct) {
   // API calls are only supported with JSObject receivers.
-  if (!receiver->IsJSObject()) return isolate->heap()->null_value();
+  if (!receiver->IsJSObject()) return isolate->factory()->null_value();
+  auto js_receiver = Handle<JSObject>::cast(receiver);
+  if (!is_construct && js_receiver->IsAccessCheckNeeded() &&
+      !isolate->MayAccess(js_receiver)) {
+    return isolate->factory()->null_value();
+  }
   Object* recv_type = this->signature();
   // No signature, return holder.
   if (recv_type->IsUndefined()) return receiver;
   FunctionTemplateInfo* signature = FunctionTemplateInfo::cast(recv_type);
   // Check the receiver.
-  for (PrototypeIterator iter(isolate, receiver,
+  for (PrototypeIterator iter(isolate, *receiver,
                               PrototypeIterator::START_AT_RECEIVER);
        !iter.IsAtEnd(PrototypeIterator::END_AT_NON_HIDDEN); iter.Advance()) {
-    if (signature->IsTemplateFor(iter.GetCurrent())) return iter.GetCurrent();
+    if (signature->IsTemplateFor(iter.GetCurrent())) {
+      return Handle<Object>(iter.GetCurrent(), isolate);
+    }
   }
-  return isolate->heap()->null_value();
+  return isolate->factory()->null_value();
 }
 
 

diff --git a/src/objects.h b/src/objects.h
index cf9184f..0fc1d0d 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -10783,7 +10783,9 @@ class FunctionTemplateInfo: public TemplateInfo {
 
   // Returns the holder JSObject if the function can legally be called with this
   // receiver.  Returns Heap::null_value() if the call is illegal.
-  Object* GetCompatibleReceiver(Isolate* isolate, Object* receiver);
+  Handle<Object> GetCompatibleReceiver(Isolate* isolate,
+                                       Handle<Object> receiver,
+                                       bool is_construct);
 
  private:
   // Bit position in the flag, from least significant bit position.

diff --git a/test/cctest/test-accessors.cc b/test/cctest/test-accessors.cc
index bbb74c0..5454699 100644 (file)
--- a/test/cctest/test-accessors.cc
+++ b/test/cctest/test-accessors.cc
@@ -605,3 +605,86 @@ THREADED_TEST(Regress433458) {
       "Object.defineProperty(obj, 'prop', { writable: false });"
       "Object.defineProperty(obj, 'prop', { writable: true });");
 }
+
+
+static bool security_check_value = false;
+
+
+static bool SecurityTestCallback(Local<v8::Object> global, Local<Value> name,
+                                 v8::AccessType type, Local<Value> data) {
+  return security_check_value;
+}
+
+
+TEST(PrototypeGetterAccessCheck) {
+  i::FLAG_allow_natives_syntax = true;
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::HandleScope scope(isolate);
+  auto fun_templ = v8::FunctionTemplate::New(isolate);
+  auto getter_templ = v8::FunctionTemplate::New(isolate, handle_property);
+  getter_templ->SetLength(0);
+  fun_templ->InstanceTemplate()->SetAccessorProperty(v8_str("foo"),
+                                                     getter_templ);
+  auto obj_templ = v8::ObjectTemplate::New(isolate);
+  obj_templ->SetAccessCheckCallbacks(SecurityTestCallback, nullptr);
+  env->Global()->Set(v8_str("Fun"), fun_templ->GetFunction());
+  env->Global()->Set(v8_str("obj"), obj_templ->NewInstance());
+  env->Global()->Set(v8_str("obj2"), obj_templ->NewInstance());
+
+  security_check_value = true;
+  CompileRun("var proto = new Fun();");
+  CompileRun("obj.__proto__ = proto;");
+  ExpectInt32("proto.foo", 907);
+
+  // Test direct.
+  security_check_value = true;
+  ExpectInt32("obj.foo", 907);
+  security_check_value = false;
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("obj.foo");
+    CHECK(try_catch.HasCaught());
+  }
+
+  // Test through call.
+  security_check_value = true;
+  ExpectInt32("proto.__lookupGetter__('foo').call(obj)", 907);
+  security_check_value = false;
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("proto.__lookupGetter__('foo').call(obj)");
+    CHECK(try_catch.HasCaught());
+  }
+
+  // Test ics.
+  CompileRun(
+      "function f() {"
+      "   var x;"
+      "  for (var i = 0; i < 4; i++) {"
+      "    x = obj.foo;"
+      "  }"
+      "  return x;"
+      "}");
+
+  security_check_value = true;
+  ExpectInt32("f()", 907);
+  security_check_value = false;
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("f();");
+    CHECK(try_catch.HasCaught());
+  }
+
+  // Test crankshaft.
+  CompileRun("%OptimizeFunctionOnNextCall(f);");
+
+  security_check_value = true;
+  ExpectInt32("f()", 907);
+  security_check_value = false;
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("f();");
+    CHECK(try_catch.HasCaught());
+  }
+}

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 5edbda7..3ad0e6c 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -8279,8 +8279,11 @@ TEST(DetachedAccesses) {
   CHECK(result.IsEmpty());
   result = CompileRun("get_x_w()");
   CHECK(result.IsEmpty());
-  result = CompileRun("this_x()");
-  CHECK(v8_str("env2_x")->Equals(result));
+  {
+    v8::TryCatch try_catch(env1->GetIsolate());
+    CompileRun("this_x()");
+    CHECK(try_catch.HasCaught());
+  }
 
   // Reattach env2's proxy
   env2 = Context::New(env1->GetIsolate(),