feat: add worldSafe flag for executeJS results (#24712)

* feat: add worldSafe flag for executeJS results * chore: do not log warning for webContents.executeJS * Apply suggestions from code review Co-authored-by: Jeremy Rose <jeremya@chromium.org> * chore: apply PR feedback * chore: split logic a bit * chore: allow primitives through the world safe checl * chore: clean up per PR feedback * chore: flip boolean logic * chore: update per PR feedback * chore: fix typo * chore: fix spec * Update web-frame.ts Co-authored-by: Samuel Attard <samuel.r.attard@gmail.com> Co-authored-by: Samuel Attard <sattard@slack-corp.com> Co-authored-by: Jeremy Rose <jeremya@chromium.org>
electron · Aug 5, 2020 · a579582 · a579582
1 parent 0107cab
commit a579582
Show file tree

Hide file tree

Showing 10 changed files with 160 additions and 7 deletions.
diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md
@@ -348,6 +348,9 @@ It creates a new `BrowserWindow` with native properties as set by the `options`.
       You can access this context in the dev tools by selecting the
       'Electron Isolated Context' entry in the combo box at the top of the
       Console tab.
+    * `worldSafeExecuteJavaScript` Boolean (optional) - If true, values returned from `webFrame.executeJavaScript` will be sanitized to ensure JS values
+      can't unsafely cross between worlds when using `contextIsolation`.  The default
+      is `false`. In Electron 12, the default will be changed to `true`. _Deprecated_
     * `nativeWindowOpen` Boolean (optional) - Whether to use native
       `window.open()`. Defaults to `false`. Child windows will always have node
       integration disabled unless `nodeIntegrationInSubFrames` is true. **Note:** This option is currently

diff --git a/lib/renderer/api/web-frame.ts b/lib/renderer/api/web-frame.ts
@@ -1,4 +1,5 @@
 import { EventEmitter } from 'events';
+import deprecate from '@electron/internal/common/api/deprecate';
 
 const binding = process.electronBinding('web_frame');
 
@@ -47,14 +48,26 @@ class WebFrame extends EventEmitter {
   }
 }
 
+const { hasSwitch } = process.electronBinding('command_line');
+const worldSafeJS = hasSwitch('world-safe-execute-javascript') && hasSwitch('context-isolation');
+
 // Populate the methods.
 for (const name in binding) {
   if (!name.startsWith('_')) { // some methods are manually populated above
     // TODO(felixrieseberg): Once we can type web_frame natives, we could
     // use a neat `keyof` here
     (WebFrame as any).prototype[name] = function (...args: Array<any>) {
+      if (!worldSafeJS && name.startsWith('executeJavaScript')) {
+        deprecate.log(`Security Warning: webFrame.${name} was called without worldSafeExecuteJavaScript enabled. This is considered unsafe. worldSafeExecuteJavaScript will be enabled by default in Electron 12.`);
+      }
       return binding[name](this.context, ...args);
     };
+    // TODO(MarshallOfSound): Remove once the above deprecation is removed
+    if (name.startsWith('executeJavaScript')) {
+      (WebFrame as any).prototype[`_${name}`] = function (...args: Array<any>) {
+        return binding[name](this.context, ...args);
+      };
+    }
   }
 }
 

diff --git a/lib/renderer/web-frame-init.ts b/lib/renderer/web-frame-init.ts
@@ -12,6 +12,11 @@ export const webFrameInit = () => {
   ipcRendererUtils.handle('ELECTRON_INTERNAL_RENDERER_WEB_FRAME_METHOD', (
     event, method: keyof WebFrameMethod, ...args: any[]
   ) => {
+    // TODO(MarshallOfSound): Remove once the world-safe-execute-javascript deprecation warning is removed
+    if (method.startsWith('executeJavaScript')) {
+      return (webFrame as any)[`_${method}`](...args);
+    }
+
     // The TypeScript compiler cannot handle the sheer number of
     // call signatures here and simply gives up. Incorrect invocations
     // will be caught by "keyof WebFrameMethod" though.

diff --git a/shell/browser/web_contents_preferences.cc b/shell/browser/web_contents_preferences.cc
@@ -127,6 +127,7 @@ WebContentsPreferences::WebContentsPreferences(
   SetDefaultBoolIfUndefined(options::kSandbox, false);
   SetDefaultBoolIfUndefined(options::kNativeWindowOpen, false);
   SetDefaultBoolIfUndefined(options::kContextIsolation, false);
+  SetDefaultBoolIfUndefined(options::kWorldSafeExecuteJavaScript, false);
   SetDefaultBoolIfUndefined(options::kJavaScript, true);
   SetDefaultBoolIfUndefined(options::kImages, true);
   SetDefaultBoolIfUndefined(options::kTextAreasAreResizable, true);
@@ -337,6 +338,9 @@ void WebContentsPreferences::AppendCommandLineSwitches(
   if (IsEnabled(options::kContextIsolation))
     command_line->AppendSwitch(switches::kContextIsolation);
 
+  if (IsEnabled(options::kWorldSafeExecuteJavaScript))
+    command_line->AppendSwitch(switches::kWorldSafeExecuteJavaScript);
+
   // --background-color.
   std::string s;
   if (GetAsString(&preference_, options::kBackgroundColor, &s)) {

diff --git a/shell/common/options_switches.cc b/shell/common/options_switches.cc
@@ -114,6 +114,9 @@ const char kNodeIntegration[] = "nodeIntegration";
 // Enable context isolation of Electron APIs and preload script
 const char kContextIsolation[] = "contextIsolation";
 
+// Enable world safe passing of values when using "executeJavaScript"
+const char kWorldSafeExecuteJavaScript[] = "worldSafeExecuteJavaScript";
+
 // Instance ID of guest WebContents.
 const char kGuestInstanceID[] = "guestInstanceId";
 
@@ -235,6 +238,7 @@ const char kPreloadScript[] = "preload";
 const char kPreloadScripts[] = "preload-scripts";
 const char kNodeIntegration[] = "node-integration";
 const char kContextIsolation[] = "context-isolation";
+const char kWorldSafeExecuteJavaScript[] = "world-safe-execute-javascript";
 const char kGuestInstanceID[] = "guest-instance-id";
 const char kOpenerID[] = "opener-id";
 const char kScrollBounce[] = "scroll-bounce";

diff --git a/shell/common/options_switches.h b/shell/common/options_switches.h
@@ -62,6 +62,7 @@ extern const char kPreloadScript[];
 extern const char kPreloadURL[];
 extern const char kNodeIntegration[];
 extern const char kContextIsolation[];
+extern const char kWorldSafeExecuteJavaScript[];
 extern const char kGuestInstanceID[];
 extern const char kExperimentalFeatures[];
 extern const char kOpenerID[];
@@ -120,6 +121,7 @@ extern const char kPreloadScript[];
 extern const char kPreloadScripts[];
 extern const char kNodeIntegration[];
 extern const char kContextIsolation[];
+extern const char kWorldSafeExecuteJavaScript[];
 extern const char kGuestInstanceID[];
 extern const char kOpenerID[];
 extern const char kScrollBounce[];

diff --git a/shell/renderer/api/electron_api_web_frame.cc b/shell/renderer/api/electron_api_web_frame.cc
@@ -21,6 +21,7 @@
 #include "shell/common/gin_helper/error_thrower.h"
 #include "shell/common/gin_helper/promise.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/options_switches.h"
 #include "shell/renderer/api/electron_api_spell_check_client.h"
 #include "third_party/blink/public/common/page/page_zoom.h"
 #include "third_party/blink/public/common/web_cache/web_cache_resource_type_stats.h"
@@ -120,25 +121,83 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback {
 
   explicit ScriptExecutionCallback(
       gin_helper::Promise<v8::Local<v8::Value>> promise,
+      bool world_safe_result,
       CompletionCallback callback)
-      : promise_(std::move(promise)), callback_(std::move(callback)) {}
+      : promise_(std::move(promise)),
+        world_safe_result_(world_safe_result),
+        callback_(std::move(callback)) {}
 
   ~ScriptExecutionCallback() override = default;
 
+  void CopyResultToCallingContextAndFinalize(
+      v8::Isolate* isolate,
+      const v8::Local<v8::Value>& result) {
+    blink::CloneableMessage ret;
+    bool success;
+    std::string error_message;
+    {
+      v8::TryCatch try_catch(isolate);
+      success = gin::ConvertFromV8(isolate, result, &ret);
+      if (try_catch.HasCaught()) {
+        auto message = try_catch.Message();
+
+        if (message.IsEmpty() ||
+            !gin::ConvertFromV8(isolate, message->Get(), &error_message)) {
+          error_message =
+              "An unknown exception occurred while getting the result of "
+              "the script";
+        }
+      }
+    }
+    if (!success) {
+      // Failed convert so we send undefined everywhere
+      if (callback_)
+        std::move(callback_).Run(
+            v8::Undefined(isolate),
+            v8::Exception::Error(
+                v8::String::NewFromUtf8(isolate, error_message.c_str())
+                    .ToLocalChecked()));
+      promise_.RejectWithErrorMessage(error_message);
+    } else {
+      v8::Local<v8::Context> context = promise_.GetContext();
+      v8::Context::Scope context_scope(context);
+      v8::Local<v8::Value> cloned_value = gin::ConvertToV8(isolate, ret);
+      if (callback_)
+        std::move(callback_).Run(cloned_value, v8::Undefined(isolate));
+      promise_.Resolve(cloned_value);
+    }
+  }
+
   void Completed(
       const blink::WebVector<v8::Local<v8::Value>>& result) override {
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
     if (!result.empty()) {
       if (!result[0].IsEmpty()) {
-        // Right now only single results per frame is supported.
-        if (!callback_.is_null())
-          std::move(callback_).Run(result[0], v8::Undefined(isolate));
-        promise_.Resolve(result[0]);
+        v8::Local<v8::Value> value = result[0];
+        // Either world safe results are disabled or the result was created in
+        // the same world as the caller or the result is not an object and
+        // therefore does not have a prototype chain to protect
+        bool should_clone_value =
+            world_safe_result_ &&
+            !(value->IsObject() &&
+              promise_.GetContext() ==
+                  value.As<v8::Object>()->CreationContext()) &&
+            value->IsObject();
+        if (should_clone_value) {
+          CopyResultToCallingContextAndFinalize(isolate, value);
+        } else {
+          // Right now only single results per frame is supported.
+          if (callback_)
+            std::move(callback_).Run(value, v8::Undefined(isolate));
+          promise_.Resolve(value);
+        }
       } else {
         const char* error_message =
             "Script failed to execute, this normally means an error "
             "was thrown. Check the renderer console for the error.";
         if (!callback_.is_null()) {
+          v8::Local<v8::Context> context = promise_.GetContext();
+          v8::Context::Scope context_scope(context);
           std::move(callback_).Run(
               v8::Undefined(isolate),
               v8::Exception::Error(
@@ -152,6 +211,8 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback {
           "WebFrame was removed before script could run. This normally means "
           "the underlying frame was destroyed";
       if (!callback_.is_null()) {
+        v8::Local<v8::Context> context = promise_.GetContext();
+        v8::Context::Scope context_scope(context);
         std::move(callback_).Run(
             v8::Undefined(isolate),
             v8::Exception::Error(v8::String::NewFromUtf8(isolate, error_message)
@@ -164,6 +225,7 @@ class ScriptExecutionCallback : public blink::WebScriptExecutionCallback {
 
  private:
   gin_helper::Promise<v8::Local<v8::Value>> promise_;
+  bool world_safe_result_;
   CompletionCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ScriptExecutionCallback);
@@ -492,10 +554,13 @@ v8::Local<v8::Promise> ExecuteJavaScript(gin_helper::Arguments* args,
   ScriptExecutionCallback::CompletionCallback completion_callback;
   args->GetNext(&completion_callback);
 
+  bool world_safe_exec_js = base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kWorldSafeExecuteJavaScript);
+
   render_frame->GetWebFrame()->RequestExecuteScriptAndReturnValue(
       blink::WebScriptSource(blink::WebString::FromUTF16(code)),
       has_user_gesture,
-      new ScriptExecutionCallback(std::move(promise),
+      new ScriptExecutionCallback(std::move(promise), world_safe_exec_js,
                                   std::move(completion_callback)));
 
   return handle;
@@ -555,13 +620,16 @@ v8::Local<v8::Promise> ExecuteJavaScriptInIsolatedWorld(
                                blink::WebURL(GURL(url)), start_line));
   }
 
+  bool world_safe_exec_js = base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kWorldSafeExecuteJavaScript);
+
   // Debugging tip: if you see a crash stack trace beginning from this call,
   // then it is very likely that some exception happened when executing the
   // "content_script/init.js" script.
   render_frame->GetWebFrame()->RequestExecuteScriptInIsolatedWorld(
       world_id, &sources.front(), sources.size(), has_user_gesture,
       scriptExecutionType,
-      new ScriptExecutionCallback(std::move(promise),
+      new ScriptExecutionCallback(std::move(promise), world_safe_exec_js,
                                   std::move(completion_callback)));
 
   return handle;

diff --git a/spec-main/api-web-frame-spec.ts b/spec-main/api-web-frame-spec.ts
@@ -2,12 +2,48 @@ import { expect } from 'chai';
 import * as path from 'path';
 import { BrowserWindow, ipcMain } from 'electron';
 import { closeAllWindows } from './window-helpers';
+import { emittedOnce } from './events-helpers';
 
 describe('webFrame module', () => {
   const fixtures = path.resolve(__dirname, '..', 'spec', 'fixtures');
 
   afterEach(closeAllWindows);
 
+  for (const worldSafe of [true, false]) {
+    it(`can use executeJavaScript with world safe mode ${worldSafe ? 'enabled' : 'disabled'}`, async () => {
+      const w = new BrowserWindow({
+        show: true,
+        webPreferences: {
+          nodeIntegration: true,
+          contextIsolation: true,
+          worldSafeExecuteJavaScript: worldSafe,
+          preload: path.join(fixtures, 'pages', 'world-safe-preload.js')
+        }
+      });
+      const isSafe = emittedOnce(ipcMain, 'executejs-safe');
+      w.loadURL('about:blank');
+      const [, wasSafe] = await isSafe;
+      expect(wasSafe).to.equal(worldSafe);
+    });
+  }
+
+  it('can use executeJavaScript with world safe mode enabled and catch conversion errors', async () => {
+    const w = new BrowserWindow({
+      show: true,
+      webPreferences: {
+        nodeIntegration: true,
+        contextIsolation: true,
+        worldSafeExecuteJavaScript: true,
+        preload: path.join(fixtures, 'pages', 'world-safe-preload-error.js')
+      }
+    });
+    const execError = emittedOnce(ipcMain, 'executejs-safe');
+    w.loadURL('about:blank');
+    const [, error] = await execError;
+    expect(error).to.not.equal(null, 'Error should not be null');
+    expect(error).to.have.property('message', 'Uncaught Error: An object could not be cloned.');
+  });
+
   it('calls a spellcheck provider', async () => {
     const w = new BrowserWindow({
       show: false,

diff --git a/spec/fixtures/pages/world-safe-preload-error.js b/spec/fixtures/pages/world-safe-preload-error.js
@@ -0,0 +1,10 @@
+const { ipcRenderer, webFrame } = require('electron');
+
+webFrame.executeJavaScript(`(() => {
+  return Object(Symbol('a'));
+})()`).catch((err) => {
+  // Considered safe if the object is constructed in this world
+  ipcRenderer.send('executejs-safe', err);
+}).then(() => {
+  ipcRenderer.send('executejs-safe', null);
+});
diff --git a/spec/fixtures/pages/world-safe-preload.js b/spec/fixtures/pages/world-safe-preload.js
@@ -0,0 +1,8 @@
+const { ipcRenderer, webFrame } = require('electron');
+
+webFrame.executeJavaScript(`(() => {
+  return {};
+})()`).then((obj) => {
+  // Considered safe if the object is constructed in this world
+  ipcRenderer.send('executejs-safe', obj.constructor === Object);
+});