fix: illegal access errors with nodeIntegrationInSubFrames (#29170)

electron · May 14, 2021 · 692c510 · 692c510
1 parent e6f15c4
commit 692c510
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 4 deletions.
diff --git a/shell/renderer/electron_render_frame_observer.cc b/shell/renderer/electron_render_frame_observer.cc
@@ -78,6 +78,7 @@ void ElectronRenderFrameObserver::DidInstallConditionalFeatures(
   bool is_not_opened = !render_frame_->GetWebFrame()->Opener() ||
                        prefs.node_leakage_in_renderers;
   bool allow_node_in_sub_frames = prefs.node_integration_in_sub_frames;
+
   bool should_create_isolated_context =
       use_context_isolation && is_main_world &&
       (is_main_frame || allow_node_in_sub_frames) &&
@@ -155,12 +156,24 @@ bool ElectronRenderFrameObserver::IsIsolatedWorld(int world_id) {
 
 bool ElectronRenderFrameObserver::ShouldNotifyClient(int world_id) {
   auto prefs = render_frame_->GetBlinkPreferences();
+
+  // This is necessary because if an iframe is created and a source is not
+  // set, the iframe loads about:blank and creates a script context for the
+  // same. We don't want to create a Node.js environment here because if the src
+  // is later set, the JS necessary to do that triggers illegal access errors
+  // when the initial about:blank Node.js environment is cleaned up. See:
+  // https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/render_frame_impl.h;l=870-892;drc=4b6001440a18740b76a1c63fa2a002cc941db394
+  GURL url = render_frame_->GetWebFrame()->GetDocument().Url();
   bool allow_node_in_sub_frames = prefs.node_integration_in_sub_frames;
+  if (allow_node_in_sub_frames && url.IsAboutBlank() &&
+      !render_frame_->IsMainFrame())
+    return false;
+
   if (prefs.context_isolation &&
       (render_frame_->IsMainFrame() || allow_node_in_sub_frames))
     return IsIsolatedWorld(world_id);
-  else
-    return IsMainWorld(world_id);
+
+  return IsMainWorld(world_id);
 }
 
 }  // namespace electron
diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc
@@ -80,8 +80,8 @@ void ElectronRendererClient::DidCreateScriptContext(
   // TODO(zcbenz): Do not create Node environment if node integration is not
   // enabled.
 
-  // Only load node if we are a main frame or a devtools extension
-  // unless node support has been explicitly enabled for sub frames
+  // Only load Node.js if we are a main frame or a devtools extension
+  // unless Node.js support has been explicitly enabled for subframes.
   auto prefs = render_frame->GetBlinkPreferences();
   bool reuse_renderer_processes_enabled =
       prefs.disable_electron_site_instance_overrides;
@@ -97,6 +97,7 @@ void ElectronRendererClient::DidCreateScriptContext(
                        (is_not_opened || reuse_renderer_processes_enabled);
   bool is_devtools = IsDevToolsExtension(render_frame);
   bool allow_node_in_subframes = prefs.node_integration_in_sub_frames;
+
   bool should_load_node =
       (is_main_frame || is_devtools || allow_node_in_subframes) &&
       !IsWebViewFrame(renderer_context, render_frame);

diff --git a/shell/renderer/electron_sandboxed_renderer_client.cc b/shell/renderer/electron_sandboxed_renderer_client.cc
@@ -204,8 +204,10 @@ void ElectronSandboxedRendererClient::DidCreateScriptContext(
   bool is_main_frame = render_frame->IsMainFrame();
   bool is_devtools =
       IsDevTools(render_frame) || IsDevToolsExtension(render_frame);
+
   bool allow_node_in_sub_frames =
       render_frame->GetBlinkPreferences().node_integration_in_sub_frames;
+
   bool should_load_preload =
       (is_main_frame || is_devtools || allow_node_in_sub_frames) &&
       !IsWebViewFrame(context, render_frame);

diff --git a/spec-main/fixtures/crash-cases/js-execute-iframe/index.html b/spec-main/fixtures/crash-cases/js-execute-iframe/index.html
@@ -0,0 +1,29 @@
+<html>
+  <body>
+    <iframe id="mainframe"></iframe>
+    <script>
+      const net = require('net');
+      const path = require('path');
+
+      document.getElementById("mainframe").src="./page2.html";
+
+      const p = process.platform === 'win32'
+              ? path.join('\\\\?\\pipe', process.cwd(), 'myctl')
+              : '/tmp/echo.sock';
+
+      const client = net.createConnection({ path: p }, () => {
+        console.log('connected to server');
+        client.write('world!\r\n');
+      });
+
+      client.on('data', (data) => {
+        console.log(data.toString());
+        client.end();
+      });
+
+      client.on('end', () => {
+        console.log('disconnected from server');
+      });
+    </script>
+  </body>
+</html>
diff --git a/spec-main/fixtures/crash-cases/js-execute-iframe/index.js b/spec-main/fixtures/crash-cases/js-execute-iframe/index.js
@@ -0,0 +1,51 @@
+const { app, BrowserWindow } = require('electron');
+const net = require('net');
+const path = require('path');
+
+function createWindow () {
+  const mainWindow = new BrowserWindow({
+    webPreferences: {
+      nodeIntegration: true,
+      contextIsolation: false,
+      nodeIntegrationInSubFrames: true
+    }
+  });
+
+  mainWindow.loadFile('index.html');
+}
+
+app.whenReady().then(() => {
+  createWindow();
+
+  app.on('activate', () => {
+    if (BrowserWindow.getAllWindows().length === 0) createWindow();
+  });
+});
+
+app.on('window-all-closed', () => {
+  if (process.platform !== 'darwin') app.quit();
+});
+
+const server = net.createServer((c) => {
+  console.log('client connected');
+
+  c.on('end', () => {
+    console.log('client disconnected');
+    app.quit();
+  });
+
+  c.write('hello\r\n');
+  c.pipe(c);
+});
+
+server.on('error', (err) => {
+  throw err;
+});
+
+const p = process.platform === 'win32'
+  ? path.join('\\\\?\\pipe', process.cwd(), 'myctl')
+  : '/tmp/echo.sock';
+
+server.listen(p, () => {
+  console.log('server bound');
+});
diff --git a/spec-main/fixtures/crash-cases/js-execute-iframe/page2.html b/spec-main/fixtures/crash-cases/js-execute-iframe/page2.html
@@ -0,0 +1,4 @@
+<!DOCTYPE html>
+<html>
+	<body>HELLO</body>
+</html>