From b7a23450b7ec345929d282dcf0e2c8a75c179d51 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Fri, 14 May 2021 13:36:15 +0200 Subject: [PATCH] fix: illegal access errors with nodeIntegrationInSubFrames (#29093) --- .../electron_render_frame_observer.cc | 17 ++++++- shell/renderer/electron_renderer_client.cc | 5 +- .../electron_sandboxed_renderer_client.cc | 2 + .../crash-cases/js-execute-iframe/index.html | 29 +++++++++++ .../crash-cases/js-execute-iframe/index.js | 51 +++++++++++++++++++ .../crash-cases/js-execute-iframe/page2.html | 4 ++ 6 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 spec-main/fixtures/crash-cases/js-execute-iframe/index.html create mode 100644 spec-main/fixtures/crash-cases/js-execute-iframe/index.js create mode 100644 spec-main/fixtures/crash-cases/js-execute-iframe/page2.html diff --git a/shell/renderer/electron_render_frame_observer.cc b/shell/renderer/electron_render_frame_observer.cc index 408c86e4a2d0..ecd6c7f0dc44 100644 --- a/shell/renderer/electron_render_frame_observer.cc +++ b/shell/renderer/electron_render_frame_observer.cc @@ -79,6 +79,7 @@ void ElectronRenderFrameObserver::DidInstallConditionalFeatures( bool is_main_world = IsMainWorld(world_id); bool is_main_frame = render_frame_->IsMainFrame(); 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); @@ -157,12 +158,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 index f35d60e2855c..59d511ba2ba3 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/shell/renderer/electron_renderer_client.cc @@ -80,12 +80,13 @@ 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 is_main_frame = render_frame->IsMainFrame(); 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 index 19898de29283..afa958fcde6e 100644 --- a/shell/renderer/electron_sandboxed_renderer_client.cc +++ b/shell/renderer/electron_sandboxed_renderer_client.cc @@ -209,8 +209,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 new file mode 100644 index 000000000000..96a82f6749f2 --- /dev/null +++ b/spec-main/fixtures/crash-cases/js-execute-iframe/index.html @@ -0,0 +1,29 @@ + + + + + + \ No newline at end of file diff --git a/spec-main/fixtures/crash-cases/js-execute-iframe/index.js b/spec-main/fixtures/crash-cases/js-execute-iframe/index.js new file mode 100644 index 000000000000..d024d4eea4b1 --- /dev/null +++ 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 new file mode 100644 index 000000000000..755d755c42e3 --- /dev/null +++ b/spec-main/fixtures/crash-cases/js-execute-iframe/page2.html @@ -0,0 +1,4 @@ + + + HELLO + \ No newline at end of file