Allow embedder to block or modify eval arguments.

This extends the existing Isolate::SetAllowCodeGenerationFromStringsCallback
mechanism, by adding SetModifyCodeGenerationFromStringCallback, which
can also modify the eval argument (it could e.g. add escaping).

Bug: chromium:940927
Change-Id: I2b72ec2e3b77a5a33f428a0db5cef3f9f8ed6ba2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1593336Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62185}

include/v8.h

@@ -6837,6 +6837,8 @@ typedef void (*FailedAccessCheckCallback)(Local<Object> target,
  */
 typedef bool (*AllowCodeGenerationFromStringsCallback)(Local<Context> context,
                                                        Local<String> source);
+typedef MaybeLocal<String> (*ModifyCodeGenerationFromStringsCallback)(
+    Local<Context> context, Local<Value> source);
 
 // --- WebAssembly compilation callbacks ---
 typedef bool (*ExtensionCallback)(const FunctionCallbackInfo<Value>&);
@@ -8437,6 +8439,8 @@ class V8_EXPORT Isolate {
    */
   void SetAllowCodeGenerationFromStringsCallback(
       AllowCodeGenerationFromStringsCallback callback);
+  void SetModifyCodeGenerationFromStringsCallback(
+      ModifyCodeGenerationFromStringsCallback callback);
 
   /**
    * Set the callback to invoke to check if wasm code generation should

src/api/api.cc

@@ -8424,6 +8424,9 @@ CALLBACK_SETTER(FatalErrorHandler, FatalErrorCallback, exception_behavior)
 CALLBACK_SETTER(OOMErrorHandler, OOMErrorCallback, oom_behavior)
 CALLBACK_SETTER(AllowCodeGenerationFromStringsCallback,
                 AllowCodeGenerationFromStringsCallback, allow_code_gen_callback)
+CALLBACK_SETTER(ModifyCodeGenerationFromStringsCallback,
+                ModifyCodeGenerationFromStringsCallback,
+                modify_code_gen_callback)
 CALLBACK_SETTER(AllowWasmCodeGenerationCallback,
                 AllowWasmCodeGenerationCallback, allow_wasm_code_gen_callback)
 

src/builtins/builtins-global.cc

@@ -86,17 +86,27 @@ BUILTIN(GlobalEval) {
   Handle<Object> x = args.atOrUndefined(isolate, 1);
   Handle<JSFunction> target = args.target();
   Handle<JSObject> target_global_proxy(target->global_proxy(), isolate);
-  if (!x->IsString()) return *x;
   if (!Builtins::AllowDynamicFunction(isolate, target, target_global_proxy)) {
     isolate->CountUsage(v8::Isolate::kFunctionConstructorReturnedUndefined);
     return ReadOnlyRoots(isolate).undefined_value();
   }
+
+  // Run embedder pre-checks before executing eval. If the argument is a
+  // non-String (or other object the embedder doesn't know to handle), then
+  // return it directly.
+  MaybeHandle<String> source;
+  bool unhandled_object;
+  std::tie(source, unhandled_object) =
+      Compiler::ValidateDynamicCompilationSource(
+          isolate, handle(target->native_context(), isolate), x);
+  if (unhandled_object) return *x;
+
   Handle<JSFunction> function;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
       isolate, function,
-      Compiler::GetFunctionFromString(handle(target->native_context(), isolate),
-                                      Handle<String>::cast(x),
-                                      NO_PARSE_RESTRICTION, kNoSourcePosition));
+      Compiler::GetFunctionFromValidatedString(
+          handle(target->native_context(), isolate), source,
+          NO_PARSE_RESTRICTION, kNoSourcePosition));
   RETURN_RESULT_OR_FAILURE(
       isolate,
       Execution::Call(isolate, function, target_global_proxy, 0, nullptr));

src/codegen/compiler.cc

@@ -1599,33 +1599,103 @@ MaybeHandle<JSFunction> Compiler::GetFunctionFromEval(
   return result;
 }
 
-bool Compiler::CodeGenerationFromStringsAllowed(Isolate* isolate,
-                                                Handle<Context> context,
-                                                Handle<String> source) {
+// Check whether embedder allows code generation in this context.
+// (via v8::Isolate::SetAllowCodeGenerationFromStringsCallback)
+bool CodeGenerationFromStringsAllowed(Isolate* isolate, Handle<Context> context,
+                                      Handle<String> source) {
   DCHECK(context->allow_code_gen_from_strings().IsFalse(isolate));
-  // Check with callback if set.
+  DCHECK(isolate->allow_code_gen_callback());
+
+  // Callback set. Let it decide if code generation is allowed.
+  VMState<EXTERNAL> state(isolate);
+  RuntimeCallTimerScope timer(
+      isolate, RuntimeCallCounterId::kCodeGenerationFromStringsCallbacks);
   AllowCodeGenerationFromStringsCallback callback =
       isolate->allow_code_gen_callback();
-  if (callback == nullptr) {
-    // No callback set and code generation disallowed.
-    return false;
-  } else {
-    // Callback set. Let it decide if code generation is allowed.
-    VMState<EXTERNAL> state(isolate);
-    return callback(v8::Utils::ToLocal(context), v8::Utils::ToLocal(source));
+  return callback(v8::Utils::ToLocal(context), v8::Utils::ToLocal(source));
+}
+
+// Check whether embedder allows code generation in this context.
+// (via v8::Isolate::SetModifyCodeGenerationFromStringsCallback)
+bool ModifyCodeGenerationFromStrings(Isolate* isolate, Handle<Context> context,
+                                     Handle<i::Object>* source) {
+  DCHECK(context->allow_code_gen_from_strings().IsFalse(isolate));
+  DCHECK(isolate->modify_code_gen_callback());
+  DCHECK(source);
+
+  // Callback set. Run it, and use the return value as source, or block
+  // execution if it's not set.
+  VMState<EXTERNAL> state(isolate);
+  ModifyCodeGenerationFromStringsCallback modify_callback =
+      isolate->modify_code_gen_callback();
+  RuntimeCallTimerScope timer(
+      isolate, RuntimeCallCounterId::kCodeGenerationFromStringsCallbacks);
+  MaybeLocal<v8::String> modified_source =
+      modify_callback(v8::Utils::ToLocal(context), v8::Utils::ToLocal(*source));
+  if (modified_source.IsEmpty()) return false;
+
+  // Use the new source (which might be the same as the old source) and return.
+  *source = Utils::OpenHandle(*modified_source.ToLocalChecked(), false);
+  return true;
+}
+
+// Run Embedder-mandated checks before generating code from a string.
+//
+// Returns a string to be used for compilation, or a flag that an object type
+// was encountered that is neither a string, nor something the embedder knows
+// how to handle.
+//
+// Returns: (assuming: std::tie(source, unknown_object))
+// - !source.is_null(): compilation allowed, source contains the source string.
+// - unknown_object is true: compilation allowed, but we don't know how to
+//                           deal with source_object.
+// - source.is_null() && !unknown_object: compilation should be blocked.
+//
+// - !source_is_null() and unknown_object can't be true at the same time.
+std::pair<MaybeHandle<String>, bool> Compiler::ValidateDynamicCompilationSource(
+    Isolate* isolate, Handle<Context> context,
+    Handle<i::Object> source_object) {
+  Handle<String> source;
+  if (source_object->IsString()) source = Handle<String>::cast(source_object);
+
+  // Check if the context unconditionally allows code gen from strings.
+  // allow_code_gen_from_strings can be many things, so we'll always check
+  // against the 'false' literal, so that e.g. undefined and 'true' are treated
+  // the same.
+  if (!context->allow_code_gen_from_strings().IsFalse(isolate)) {
+    return {source, !source_object->IsString()};
+  }
+
+  // Check if the context allows code generation for this string.
+  // allow_code_gen_callback only allows proper strings.
+  // (I.e., let allow_code_gen_callback decide, if it has been set.)
+  if (isolate->allow_code_gen_callback()) {
+    if (source_object->IsString() &&
+        CodeGenerationFromStringsAllowed(isolate, context, source)) {
+      return {source, !source_object->IsString()};
+    }
   }
+
+  // Check if the context wants to block or modify this source object.
+  // Double-check that we really have a string now.
+  // (Let modify_code_gen_callback decide, if it's been set.)
+  if (isolate->modify_code_gen_callback()) {
+    if (ModifyCodeGenerationFromStrings(isolate, context, &source_object) &&
+        source_object->IsString())
+      return {Handle<String>::cast(source_object), false};
+  }
+
+  return {MaybeHandle<String>(), !source_object->IsString()};
 }
 
-MaybeHandle<JSFunction> Compiler::GetFunctionFromString(
-    Handle<Context> context, Handle<String> source,
+MaybeHandle<JSFunction> Compiler::GetFunctionFromValidatedString(
+    Handle<Context> context, MaybeHandle<String> source,
     ParseRestriction restriction, int parameters_end_pos) {
   Isolate* const isolate = context->GetIsolate();
   Handle<Context> native_context(context->native_context(), isolate);
 
-  // Check if native context allows code generation from
-  // strings. Throw an exception if it doesn't.
-  if (native_context->allow_code_gen_from_strings().IsFalse(isolate) &&
-      !CodeGenerationFromStringsAllowed(isolate, native_context, source)) {
+  // Raise an EvalError if we did not receive a string.
+  if (source.is_null()) {
     Handle<Object> error_message =
         native_context->ErrorMessageForCodeGenerationFromStrings();
     THROW_NEW_ERROR(
@@ -1639,9 +1709,20 @@ MaybeHandle<JSFunction> Compiler::GetFunctionFromString(
   int eval_position = kNoSourcePosition;
   Handle<SharedFunctionInfo> outer_info(
       native_context->empty_function().shared(), isolate);
-  return Compiler::GetFunctionFromEval(
-      source, outer_info, native_context, LanguageMode::kSloppy, restriction,
-      parameters_end_pos, eval_scope_position, eval_position);
+  return Compiler::GetFunctionFromEval(source.ToHandleChecked(), outer_info,
+                                       native_context, LanguageMode::kSloppy,
+                                       restriction, parameters_end_pos,
+                                       eval_scope_position, eval_position);
+}
+
+MaybeHandle<JSFunction> Compiler::GetFunctionFromString(
+    Handle<Context> context, Handle<Object> source,
+    ParseRestriction restriction, int parameters_end_pos) {
+  Isolate* const isolate = context->GetIsolate();
+  Handle<Context> native_context(context->native_context(), isolate);
+  return GetFunctionFromValidatedString(
+      context, ValidateDynamicCompilationSource(isolate, context, source).first,
+      restriction, parameters_end_pos);
 }
 
 namespace {

src/codegen/compiler.h

@@ -132,17 +132,22 @@ class V8_EXPORT_PRIVATE Compiler : public AllStatic {
       v8::ScriptCompiler::CompileOptions compile_options,
       v8::ScriptCompiler::NoCacheReason no_cache_reason);
 
-  // Returns true if the embedder permits compiling the given source string in
-  // the given context.
-  static bool CodeGenerationFromStringsAllowed(Isolate* isolate,
-                                               Handle<Context> context,
-                                               Handle<String> source);
-
   // Create a (bound) function for a String source within a context for eval.
   V8_WARN_UNUSED_RESULT static MaybeHandle<JSFunction> GetFunctionFromString(
-      Handle<Context> context, Handle<String> source,
+      Handle<Context> context, Handle<i::Object> source,
       ParseRestriction restriction, int parameters_end_pos);
 
+  // Decompose GetFunctionFromString into two functions, to allow callers to
+  // deal seperately with a case of object not handled by the embedder.
+  V8_WARN_UNUSED_RESULT static std::pair<MaybeHandle<String>, bool>
+  ValidateDynamicCompilationSource(Isolate* isolate, Handle<Context> context,
+                                   Handle<i::Object> source_object);
+  V8_WARN_UNUSED_RESULT static MaybeHandle<JSFunction>
+  GetFunctionFromValidatedString(Handle<Context> context,
+                                 MaybeHandle<String> source,
+                                 ParseRestriction restriction,
+                                 int parameters_end_pos);
+
   // Create a shared function info object for a String source.
   static MaybeHandle<SharedFunctionInfo> GetSharedFunctionInfoForScript(
       Isolate* isolate, Handle<String> source,

src/execution/isolate.h

@@ -398,6 +398,8 @@ using DebugObjectCache = std::vector<Handle<HeapObject>>;
   V(OOMErrorCallback, oom_behavior, nullptr)                                   \
   V(LogEventCallback, event_logger, nullptr)                                   \
   V(AllowCodeGenerationFromStringsCallback, allow_code_gen_callback, nullptr)  \
+  V(ModifyCodeGenerationFromStringsCallback, modify_code_gen_callback,         \
+    nullptr)                                                                   \
   V(AllowWasmCodeGenerationCallback, allow_wasm_code_gen_callback, nullptr)    \
   V(ExtensionCallback, wasm_module_callback, &NoExtension)                     \
   V(ExtensionCallback, wasm_instance_callback, &NoExtension)                   \

src/logging/counters.h

@@ -893,6 +893,7 @@ class RuntimeCallTimer final {
   V(ArrayLengthSetter)                         \
   V(BoundFunctionLengthGetter)                 \
   V(BoundFunctionNameGetter)                   \
+  V(CodeGenerationFromStringsCallbacks)        \
   V(CompileAnalyse)                            \
   V(CompileBackgroundAnalyse)                  \
   V(CompileBackgroundCompileTask)              \

src/runtime/runtime-compiler.cc

@@ -294,7 +294,8 @@ RUNTIME_FUNCTION(Runtime_CompileForOnStackReplacement) {
   return Object();
 }
 
-static Object CompileGlobalEval(Isolate* isolate, Handle<String> source,
+static Object CompileGlobalEval(Isolate* isolate,
+                                Handle<i::Object> source_object,
                                 Handle<SharedFunctionInfo> outer_info,
                                 LanguageMode language_mode,
                                 int eval_scope_position, int eval_position) {
@@ -303,9 +304,15 @@ static Object CompileGlobalEval(Isolate* isolate, Handle<String> source,
 
   // Check if native context allows code generation from
   // strings. Throw an exception if it doesn't.
-  if (native_context->allow_code_gen_from_strings().IsFalse(isolate) &&
-      !Compiler::CodeGenerationFromStringsAllowed(isolate, native_context,
-                                                  source)) {
+  MaybeHandle<String> source;
+  bool unknown_object;
+  std::tie(source, unknown_object) = Compiler::ValidateDynamicCompilationSource(
+      isolate, native_context, source_object);
+  // If the argument is an unhandled string time, bounce to GlobalEval.
+  if (unknown_object) {
+    return native_context->global_eval_fun();
+  }
+  if (source.is_null()) {
     Handle<Object> error_message =
         native_context->ErrorMessageForCodeGenerationFromStrings();
     Handle<Object> error;
@@ -321,9 +328,9 @@ static Object CompileGlobalEval(Isolate* isolate, Handle<String> source,
   Handle<JSFunction> compiled;
   ASSIGN_RETURN_ON_EXCEPTION_VALUE(
       isolate, compiled,
-      Compiler::GetFunctionFromEval(source, outer_info, context, language_mode,
-                                    restriction, kNoSourcePosition,
-                                    eval_scope_position, eval_position),
+      Compiler::GetFunctionFromEval(
+          source.ToHandleChecked(), outer_info, context, language_mode,
+          restriction, kNoSourcePosition, eval_scope_position, eval_position),
       ReadOnlyRoots(isolate).exception());
   return *compiled;
 }
@@ -336,11 +343,7 @@ RUNTIME_FUNCTION(Runtime_ResolvePossiblyDirectEval) {
 
   // If "eval" didn't refer to the original GlobalEval, it's not a
   // direct call to eval.
-  // (And even if it is, but the first argument isn't a string, just let
-  // execution default to an indirect call to eval, which will also return
-  // the first argument without doing anything).
-  if (*callee != isolate->native_context()->global_eval_fun() ||
-      !args[1].IsString()) {
+  if (*callee != isolate->native_context()->global_eval_fun()) {
     return *callee;
   }
 
@@ -350,7 +353,7 @@ RUNTIME_FUNCTION(Runtime_ResolvePossiblyDirectEval) {
   DCHECK(args[4].IsSmi());
   Handle<SharedFunctionInfo> outer_info(args.at<JSFunction>(2)->shared(),
                                         isolate);
-  return CompileGlobalEval(isolate, args.at<String>(1), outer_info,
+  return CompileGlobalEval(isolate, args.at<Object>(1), outer_info,
                            language_mode, args.smi_at(4), args.smi_at(5));
 }
 }  // namespace internal

test/cctest/test-api.cc

@@ -20265,6 +20265,21 @@ bool CodeGenerationDisallowed(Local<Context> context, Local<String> source) {
   return false;
 }
 
+v8::MaybeLocal<String> ModifyCodeGeneration(Local<Context> context,
+                                            Local<Value> source) {
+  // For testing purposes, deny all odd-length strings and replace '2' with '3'
+  String::Utf8Value utf8(context->GetIsolate(), source);
+  DCHECK(utf8.length());
+  if (utf8.length() == 0 || utf8.length() % 2 != 0)
+    return v8::MaybeLocal<String>();
+
+  for (char* i = *utf8; *i != '\0'; i++) {
+    if (*i == '2') *i = '3';
+  }
+  return String::NewFromUtf8(context->GetIsolate(), *utf8,
+                             v8::NewStringType::kNormal)
+      .ToLocalChecked();
+}
 
 THREADED_TEST(AllowCodeGenFromStrings) {
   LocalContext context;
@@ -20297,6 +20312,36 @@ THREADED_TEST(AllowCodeGenFromStrings) {
   CheckCodeGenerationDisallowed();
 }
 
+TEST(ModifyCodeGenFromStrings) {
+  LocalContext context;
+  v8::HandleScope scope(context->GetIsolate());
+  context->AllowCodeGenerationFromStrings(false);
+  context->GetIsolate()->SetModifyCodeGenerationFromStringsCallback(
+      &ModifyCodeGeneration);
+
+  // Test 'allowed' case in different modes (direct eval, indirect eval,
+  // Function constructor, Function contructor with arguments).
+  Local<Value> result = CompileRun("eval('42')");
+  CHECK_EQ(43, result->Int32Value(context.local()).FromJust());
+
+  result = CompileRun("(function(e) { return e('42'); })(eval)");
+  CHECK_EQ(43, result->Int32Value(context.local()).FromJust());
+
+  result = CompileRun("var f = new Function('return 42;'); f()");
+  CHECK_EQ(43, result->Int32Value(context.local()).FromJust());
+
+  // Test 'disallowed' cases.
+  TryCatch try_catch(CcTest::isolate());
+  result = CompileRun("eval('123')");
+  CHECK(result.IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+
+  result = CompileRun("new Function('a', 'return 42;')(123)");
+  CHECK(result.IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+}
 
 TEST(SetErrorMessageForCodeGenFromStrings) {
   LocalContext context;