Reland^2 "Don't compile functions in a context the caller doesn't have access to"

Original issue's description:
> Don't compile functions in a context the caller doesn't have access to
>
> Instead just return undefined
>
> A side effect of this is that it's no longer possible to compile
> functions in a detached context.
>
> BUG=chromium:541703
> R=verwaest@chromium.org,bmeurer@chromium.org

BUG=chromium:541703
R=verwaest@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng

Review-Url: https://codereview.chromium.org/2155503004
Cr-Commit-Position: refs/heads/master@{#37842}

include/v8.h

@@ -5739,6 +5739,7 @@ class V8_EXPORT Isolate {
     kDecimalWithLeadingZeroInStrictMode = 32,
     kLegacyDateParser = 33,
     kDefineGetterOrSetterWouldThrow = 34,
+    kFunctionConstructorReturnedUndefined = 35,
 
     // If you add new values here, you'll also need to update Chromium's:
     // UseCounter.h, V8PerIsolateData.cpp, histograms.xml

src/api.cc

@@ -8942,6 +8942,10 @@ void HandleScopeImplementer::IterateThis(ObjectVisitor* v) {
     Object** start = reinterpret_cast<Object**>(&context_lists[i]->first());
     v->VisitPointers(start, start + context_lists[i]->length());
   }
+  if (microtask_context_) {
+    Object** start = reinterpret_cast<Object**>(&microtask_context_);
+    v->VisitPointers(start, start + 1);
+  }
 }
 
 

src/api.h

@@ -453,6 +453,7 @@ class HandleScopeImplementer {
         blocks_(0),
         entered_contexts_(0),
         saved_contexts_(0),
+        microtask_context_(nullptr),
         spare_(NULL),
         call_depth_(0),
         microtasks_depth_(0),
@@ -519,6 +520,10 @@ class HandleScopeImplementer {
   // contexts have been entered.
   inline Handle<Context> LastEnteredContext();
 
+  inline void EnterMicrotaskContext(Handle<Context> context);
+  inline void LeaveMicrotaskContext();
+  inline Handle<Context> MicrotaskContext();
+
   inline void SaveContext(Context* context);
   inline Context* RestoreContext();
   inline bool HasSavedContexts();
@@ -537,6 +542,7 @@ class HandleScopeImplementer {
     blocks_.Initialize(0);
     entered_contexts_.Initialize(0);
     saved_contexts_.Initialize(0);
+    microtask_context_ = nullptr;
     spare_ = NULL;
     last_handle_before_deferred_block_ = NULL;
     call_depth_ = 0;
@@ -546,6 +552,7 @@ class HandleScopeImplementer {
     DCHECK(blocks_.length() == 0);
     DCHECK(entered_contexts_.length() == 0);
     DCHECK(saved_contexts_.length() == 0);
+    DCHECK(!microtask_context_);
     blocks_.Free();
     entered_contexts_.Free();
     saved_contexts_.Free();
@@ -565,6 +572,7 @@ class HandleScopeImplementer {
   List<Context*> entered_contexts_;
   // Used as a stack to keep track of saved contexts.
   List<Context*> saved_contexts_;
+  Context* microtask_context_;
   Object** spare_;
   int call_depth_;
   int microtasks_depth_;
@@ -637,6 +645,20 @@ Handle<Context> HandleScopeImplementer::LastEnteredContext() {
   return Handle<Context>(entered_contexts_.last());
 }
 
+void HandleScopeImplementer::EnterMicrotaskContext(Handle<Context> context) {
+  DCHECK(!microtask_context_);
+  microtask_context_ = *context;
+}
+
+void HandleScopeImplementer::LeaveMicrotaskContext() {
+  DCHECK(microtask_context_);
+  microtask_context_ = nullptr;
+}
+
+Handle<Context> HandleScopeImplementer::MicrotaskContext() {
+  if (microtask_context_) return Handle<Context>(microtask_context_);
+  return Handle<Context>::null();
+}
 
 // If there's a spare block, use it for growing the current scope.
 internal::Object** HandleScopeImplementer::GetSpareOrNewBlock() {

src/builtins/builtins.cc

@@ -3485,14 +3485,38 @@ void Builtins::Generate_DatePrototypeGetUTCSeconds(MacroAssembler* masm) {
 
 namespace {
 
+bool AllowDynamicFunction(Isolate* isolate, Handle<JSFunction> target,
+                          Handle<JSObject> target_global_proxy) {
+  if (FLAG_allow_unsafe_function_constructor) return true;
+  HandleScopeImplementer* impl = isolate->handle_scope_implementer();
+  Handle<Context> responsible_context = impl->LastEnteredContext();
+  if (responsible_context.is_null()) {
+    responsible_context = impl->MicrotaskContext();
+    // TODO(jochen): Remove this.
+    if (responsible_context.is_null()) {
+      return true;
+    }
+  }
+  if (*responsible_context == target->context()) return true;
+  return isolate->MayAccess(responsible_context, target_global_proxy);
+}
+
 // ES6 section 19.2.1.1.1 CreateDynamicFunction
-MaybeHandle<JSFunction> CreateDynamicFunction(Isolate* isolate,
-                                              BuiltinArguments args,
-                                              const char* token) {
+MaybeHandle<Object> CreateDynamicFunction(Isolate* isolate,
+                                          BuiltinArguments args,
+                                          const char* token) {
   // Compute number of arguments, ignoring the receiver.
   DCHECK_LE(1, args.length());
   int const argc = args.length() - 1;
 
+  Handle<JSFunction> target = args.target<JSFunction>();
+  Handle<JSObject> target_global_proxy(target->global_proxy(), isolate);
+
+  if (!AllowDynamicFunction(isolate, target, target_global_proxy)) {
+    isolate->CountUsage(v8::Isolate::kFunctionConstructorReturnedUndefined);
+    return isolate->factory()->undefined_value();
+  }
+
   // Build the source string.
   Handle<String> source;
   {
@@ -3507,7 +3531,7 @@ MaybeHandle<JSFunction> CreateDynamicFunction(Isolate* isolate,
         Handle<String> param;
         ASSIGN_RETURN_ON_EXCEPTION(
             isolate, param, Object::ToString(isolate, args.at<Object>(i)),
-            JSFunction);
+            Object);
         param = String::Flatten(param);
         builder.AppendString(param);
         // If the formal parameters string include ) - an illegal
@@ -3532,37 +3556,35 @@ MaybeHandle<JSFunction> CreateDynamicFunction(Isolate* isolate,
       Handle<String> body;
       ASSIGN_RETURN_ON_EXCEPTION(
           isolate, body, Object::ToString(isolate, args.at<Object>(argc)),
-          JSFunction);
+          Object);
       builder.AppendString(body);
     }
     builder.AppendCString("\n})");
-    ASSIGN_RETURN_ON_EXCEPTION(isolate, source, builder.Finish(), JSFunction);
+    ASSIGN_RETURN_ON_EXCEPTION(isolate, source, builder.Finish(), Object);
 
     // The SyntaxError must be thrown after all the (observable) ToString
     // conversions are done.
     if (parenthesis_in_arg_string) {
       THROW_NEW_ERROR(isolate,
                       NewSyntaxError(MessageTemplate::kParenthesisInArgString),
-                      JSFunction);
+                      Object);
     }
   }
 
   // Compile the string in the constructor and not a helper so that errors to
   // come from here.
-  Handle<JSFunction> target = args.target<JSFunction>();
-  Handle<JSObject> target_global_proxy(target->global_proxy(), isolate);
   Handle<JSFunction> function;
   {
     ASSIGN_RETURN_ON_EXCEPTION(
         isolate, function,
         CompileString(handle(target->native_context(), isolate), source,
                       ONLY_SINGLE_FUNCTION_LITERAL),
-        JSFunction);
+        Object);
     Handle<Object> result;
     ASSIGN_RETURN_ON_EXCEPTION(
         isolate, result,
         Execution::Call(isolate, function, target_global_proxy, 0, nullptr),
-        JSFunction);
+        Object);
     function = Handle<JSFunction>::cast(result);
     function->shared()->set_name_should_print_as_anonymous(true);
   }
@@ -3581,7 +3603,7 @@ MaybeHandle<JSFunction> CreateDynamicFunction(Isolate* isolate,
     Handle<Map> initial_map;
     ASSIGN_RETURN_ON_EXCEPTION(
         isolate, initial_map,
-        JSFunction::GetDerivedMap(isolate, target, new_target), JSFunction);
+        JSFunction::GetDerivedMap(isolate, target, new_target), Object);
 
     Handle<SharedFunctionInfo> shared_info(function->shared(), isolate);
     Handle<Map> map = Map::AsLanguageMode(
@@ -3599,7 +3621,7 @@ MaybeHandle<JSFunction> CreateDynamicFunction(Isolate* isolate,
 // ES6 section 19.2.1.1 Function ( p1, p2, ... , pn, body )
 BUILTIN(FunctionConstructor) {
   HandleScope scope(isolate);
-  Handle<JSFunction> result;
+  Handle<Object> result;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
       isolate, result, CreateDynamicFunction(isolate, args, "function"));
   return *result;
@@ -3731,12 +3753,15 @@ BUILTIN(GeneratorFunctionConstructor) {
 
 BUILTIN(AsyncFunctionConstructor) {
   HandleScope scope(isolate);
-  Handle<JSFunction> func;
+  Handle<Object> maybe_func;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-      isolate, func, CreateDynamicFunction(isolate, args, "async function"));
+      isolate, maybe_func,
+      CreateDynamicFunction(isolate, args, "async function"));
+  if (!maybe_func->IsJSFunction()) return *maybe_func;
 
   // Do not lazily compute eval position for AsyncFunction, as they may not be
   // determined after the function is resumed.
+  Handle<JSFunction> func = Handle<JSFunction>::cast(maybe_func);
   Handle<Script> script = handle(Script::cast(func->shared()->script()));
   int position = script->GetEvalPosition();
   USE(position);

src/flag-definitions.h

@@ -591,6 +591,10 @@ DEFINE_BOOL(builtins_in_stack_traces, false,
             "show built-in functions in stack traces")
 DEFINE_BOOL(disable_native_files, false, "disable builtin natives files")
 
+// builtins.cc
+DEFINE_BOOL(allow_unsafe_function_constructor, false,
+            "allow invoking the function constructor without security checks")
+
 // builtins-ia32.cc
 DEFINE_BOOL(inline_new, true, "use fast inline allocation")
 

src/isolate.cc

@@ -2891,10 +2891,13 @@ void Isolate::RunMicrotasksInternal() {
             Handle<JSFunction>::cast(microtask);
         SaveContext save(this);
         set_context(microtask_function->context()->native_context());
+        handle_scope_implementer_->EnterMicrotaskContext(
+            handle(microtask_function->context(), this));
         MaybeHandle<Object> maybe_exception;
         MaybeHandle<Object> result = Execution::TryCall(
             this, microtask_function, factory()->undefined_value(), 0, NULL,
             &maybe_exception);
+        handle_scope_implementer_->LeaveMicrotaskContext();
         // If execution is terminating, just bail out.
         Handle<Object> exception;
         if (result.is_null() && maybe_exception.is_null()) {

test/cctest/test-api.cc

@@ -10131,6 +10131,12 @@ static bool AccessAlwaysBlocked(Local<v8::Context> accessing_context,
   return false;
 }
 
+static bool AccessAlwaysAllowed(Local<v8::Context> accessing_context,
+                                Local<v8::Object> global,
+                                Local<v8::Value> data) {
+  i::PrintF("Access allowed.\n");
+  return true;
+}
 
 THREADED_TEST(AccessControlGetOwnPropertyNames) {
   v8::Isolate* isolate = CcTest::isolate();
@@ -25343,3 +25349,73 @@ THREADED_TEST(ImmutableProto) {
             ->Equals(context.local(), original_proto)
             .FromJust());
 }
+
+Local<v8::Context> call_eval_context;
+Local<v8::Function> call_eval_bound_function;
+
+static void CallEval(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  v8::Context::Scope scope(call_eval_context);
+  args.GetReturnValue().Set(
+      call_eval_bound_function
+          ->Call(call_eval_context, call_eval_context->Global(), 0, NULL)
+          .ToLocalChecked());
+}
+
+TEST(CrossActivationEval) {
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::HandleScope scope(isolate);
+  {
+    call_eval_context = v8::Context::New(isolate);
+    v8::Context::Scope scope(call_eval_context);
+    call_eval_bound_function =
+        Local<Function>::Cast(CompileRun("eval.bind(this, '1')"));
+  }
+  env->Global()
+      ->Set(env.local(), v8_str("CallEval"),
+            v8::FunctionTemplate::New(isolate, CallEval)
+                ->GetFunction(env.local())
+                .ToLocalChecked())
+      .FromJust();
+  Local<Value> result = CompileRun("CallEval();");
+  CHECK(result->IsInt32());
+  CHECK_EQ(1, result->Int32Value(env.local()).FromJust());
+}
+
+TEST(EvalInAccessCheckedContext) {
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+
+  v8::Local<v8::ObjectTemplate> obj_template = v8::ObjectTemplate::New(isolate);
+
+  obj_template->SetAccessCheckCallback(AccessAlwaysAllowed);
+
+  v8::Local<Context> context0 = Context::New(isolate, NULL, obj_template);
+  v8::Local<Context> context1 = Context::New(isolate, NULL, obj_template);
+
+  Local<Value> foo = v8_str("foo");
+  Local<Value> bar = v8_str("bar");
+
+  // Set to different domains.
+  context0->SetSecurityToken(foo);
+  context1->SetSecurityToken(bar);
+
+  // Set up function in context0 that uses eval from context0.
+  context0->Enter();
+  v8::Local<v8::Value> fun = CompileRun(
+      "var x = 42;"
+      "(function() {"
+      "  var e = eval;"
+      "  return function(s) { return e(s); }"
+      "})()");
+  context0->Exit();
+
+  // Put the function into context1 and call it. Since the access check
+  // callback always returns true, the call succeeds even though the tokens
+  // are different.
+  context1->Enter();
+  context1->Global()->Set(context1, v8_str("fun"), fun).FromJust();
+  v8::Local<v8::Value> x_value = CompileRun("fun('x')");
+  CHECK_EQ(42, x_value->Int32Value(context1).FromJust());
+  context1->Exit();
+}

test/mjsunit/cross-realm-filtering.js

@@ -88,3 +88,77 @@ o = Realm.eval(realmIndex, "new f()");
 proto = Object.getPrototypeOf(o);
 assertFalse(proto === Object.prototype);
 assertTrue(proto === otherObject.prototype);
+
+// Check function constructor.
+var ctor_script = "Function";
+var ctor_a_script =
+    "(function() { return Function.apply(this, ['return 1;']); })";
+var ctor_b_script = "Function.bind(this, 'return 1;')";
+var ctor_c_script =
+    "(function() { return Function.call(this, 'return 1;'); })";
+Realm.shared = {
+  ctor_0 : Realm.eval(realms[0], ctor_script),
+  ctor_1 : Realm.eval(realms[1], ctor_script),
+  ctor_a_0 : Realm.eval(realms[0], ctor_a_script),
+  ctor_a_1 : Realm.eval(realms[1], ctor_a_script),
+  ctor_b_0 : Realm.eval(realms[0], ctor_b_script),
+  ctor_b_1 : Realm.eval(realms[1], ctor_b_script),
+  ctor_c_0 : Realm.eval(realms[0], ctor_c_script),
+  ctor_c_1 : Realm.eval(realms[1], ctor_c_script),
+}
+var script_0 = "                                                               \
+  var ctor_0 = Realm.shared.ctor_0;                                            \
+  Realm.shared.direct_0 = ctor_0('return 1');                                  \
+  Realm.shared.indirect_0 = (function() { return ctor_0('return 1;'); })();    \
+  Realm.shared.apply_0 = ctor_0.apply(this, ['return 1']);                     \
+  Realm.shared.bind_0 = ctor_0.bind(this, 'return 1')();                       \
+  Realm.shared.call_0 = ctor_0.call(this, 'return 1');                         \
+  Realm.shared.proxy_0 = new Proxy(ctor_0, {})('return 1');                    \
+  Realm.shared.reflect_0 = Reflect.apply(ctor_0, this, ['return 1']);          \
+  Realm.shared.a_0 = Realm.shared.ctor_a_0();                                  \
+  Realm.shared.b_0 = Realm.shared.ctor_b_0();                                  \
+  Realm.shared.c_0 = Realm.shared.ctor_c_0();                                  \
+";
+script = script_0 + script_0.replace(/_0/g, "_1");
+Realm.eval(realms[0], script);
+assertSame(1, Realm.shared.direct_0());
+assertSame(1, Realm.shared.indirect_0());
+assertSame(1, Realm.shared.apply_0());
+assertSame(1, Realm.shared.bind_0());
+assertSame(1, Realm.shared.call_0());
+assertSame(1, Realm.shared.proxy_0());
+assertSame(1, Realm.shared.reflect_0());
+assertSame(1, Realm.shared.a_0());
+assertSame(1, Realm.shared.b_0());
+assertSame(1, Realm.shared.c_0());
+assertSame(undefined, Realm.shared.direct_1);
+assertSame(undefined, Realm.shared.indirect_1);
+assertSame(undefined, Realm.shared.apply_1);
+assertSame(undefined, Realm.shared.bind_1);
+assertSame(undefined, Realm.shared.call_1);
+assertSame(undefined, Realm.shared.proxy_1);
+assertSame(undefined, Realm.shared.reflect_1);
+assertSame(undefined, Realm.shared.a_1);
+assertSame(undefined, Realm.shared.b_1);
+assertSame(undefined, Realm.shared.c_1);
+Realm.eval(realms[1], script);
+assertSame(undefined, Realm.shared.direct_0);
+assertSame(undefined, Realm.shared.indirect_0);
+assertSame(undefined, Realm.shared.apply_0);
+assertSame(undefined, Realm.shared.bind_0);
+assertSame(undefined, Realm.shared.call_0);
+assertSame(undefined, Realm.shared.proxy_0);
+assertSame(undefined, Realm.shared.reflect_0);
+assertSame(undefined, Realm.shared.a_0);
+assertSame(undefined, Realm.shared.b_0);
+assertSame(undefined, Realm.shared.c_0);
+assertSame(1, Realm.shared.direct_1());
+assertSame(1, Realm.shared.indirect_1());
+assertSame(1, Realm.shared.apply_1());
+assertSame(1, Realm.shared.bind_1());
+assertSame(1, Realm.shared.call_1());
+assertSame(1, Realm.shared.proxy_1());
+assertSame(1, Realm.shared.reflect_1());
+assertSame(1, Realm.shared.a_1());
+assertSame(1, Realm.shared.b_1());
+assertSame(1, Realm.shared.c_1());

test/mjsunit/es6/reflect-construct.js

 // Copyright 2014 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
+//
+// Flags: --allow-unsafe-function-constructor
 
 
 (function testReflectConstructArity() {