From 702cc84bd3dd9055a44fbe370c9b4bee0fcb1791 Mon Sep 17 00:00:00 2001 From: Milan Burda Date: Sat, 11 Aug 2018 00:19:49 +0200 Subject: [PATCH] Don't pass preloadPath via ELECTRON_BROWSER_SANDBOX_LOAD for security reasons (#13031) --- atom/browser/api/atom_api_web_contents.cc | 11 ++++++ atom/browser/api/atom_api_web_contents.h | 3 ++ atom/browser/web_contents_preferences.cc | 39 ++++++++++++------- atom/browser/web_contents_preferences.h | 3 ++ .../atom_sandboxed_renderer_client.cc | 12 ++---- lib/browser/rpc-server.js | 7 ++-- lib/sandboxed_renderer/init.js | 4 +- spec/api-browser-window-spec.js | 4 ++ 8 files changed, 57 insertions(+), 26 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 94e03b19b9..c19cbebbe6 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -1884,6 +1884,16 @@ void WebContents::OnGetZoomLevel(content::RenderFrameHost* rfh, rfh->Send(reply_msg); } +v8::Local WebContents::GetPreloadPath(v8::Isolate* isolate) const { + if (auto* web_preferences = WebContentsPreferences::From(web_contents())) { + base::FilePath::StringType preload; + if (web_preferences->GetPreloadPath(&preload)) { + return mate::ConvertToV8(isolate, preload); + } + } + return v8::Null(isolate); +} + v8::Local WebContents::GetWebPreferences( v8::Isolate* isolate) const { auto* web_preferences = WebContentsPreferences::From(web_contents()); @@ -2047,6 +2057,7 @@ void WebContents::BuildPrototype(v8::Isolate* isolate, .SetMethod("setZoomFactor", &WebContents::SetZoomFactor) .SetMethod("_getZoomFactor", &WebContents::GetZoomFactor) .SetMethod("getType", &WebContents::GetType) + .SetMethod("_getPreloadPath", &WebContents::GetPreloadPath) .SetMethod("getWebPreferences", &WebContents::GetWebPreferences) .SetMethod("getLastWebPreferences", &WebContents::GetLastWebPreferences) .SetMethod("getOwnerBrowserWindow", &WebContents::GetOwnerBrowserWindow) diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index bb90a9ee49..6b83439650 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -230,6 +230,9 @@ class WebContents : public mate::TrackableObject, const std::vector& features, const scoped_refptr& body); + // Returns the preload script path of current WebContents. + v8::Local GetPreloadPath(v8::Isolate* isolate) const; + // Returns the web preferences of current WebContents. v8::Local GetWebPreferences(v8::Isolate* isolate) const; v8::Local GetLastWebPreferences(v8::Isolate* isolate) const; diff --git a/atom/browser/web_contents_preferences.cc b/atom/browser/web_contents_preferences.cc index ea9c822c59..c21c933831 100644 --- a/atom/browser/web_contents_preferences.cc +++ b/atom/browser/web_contents_preferences.cc @@ -169,6 +169,30 @@ bool WebContentsPreferences::GetPreference(const base::StringPiece& name, return GetAsString(&preference_, name, value); } +bool WebContentsPreferences::GetPreloadPath( + base::FilePath::StringType* path) const { + DCHECK(path); + base::FilePath::StringType preload; + if (GetAsString(&preference_, options::kPreloadScript, &preload)) { + if (base::FilePath(preload).IsAbsolute()) { + *path = std::move(preload); + return true; + } else { + LOG(ERROR) << "preload script must have absolute path."; + } + } else if (GetAsString(&preference_, options::kPreloadURL, &preload)) { + // Translate to file path if there is "preload-url" option. + base::FilePath preload_path; + if (net::FileURLToFilePath(GURL(preload), &preload_path)) { + *path = std::move(preload_path.value()); + return true; + } else { + LOG(ERROR) << "preload url must be file:// protocol."; + } + } + return false; +} + // static content::WebContents* WebContentsPreferences::GetWebContentsFromProcessID( int process_id) { @@ -228,19 +252,8 @@ void WebContentsPreferences::AppendCommandLineSwitches( // The preload script. base::FilePath::StringType preload; - if (GetAsString(&preference_, options::kPreloadScript, &preload)) { - if (base::FilePath(preload).IsAbsolute()) - command_line->AppendSwitchNative(switches::kPreloadScript, preload); - else - LOG(ERROR) << "preload script must have absolute path."; - } else if (GetAsString(&preference_, options::kPreloadURL, &preload)) { - // Translate to file path if there is "preload-url" option. - base::FilePath preload_path; - if (net::FileURLToFilePath(GURL(preload), &preload_path)) - command_line->AppendSwitchPath(switches::kPreloadScript, preload_path); - else - LOG(ERROR) << "preload url must be file:// protocol."; - } + if (GetPreloadPath(&preload)) + command_line->AppendSwitchNative(switches::kPreloadScript, preload); // Custom args for renderer process auto* customArgs = diff --git a/atom/browser/web_contents_preferences.h b/atom/browser/web_contents_preferences.h index dee54a4854..d835347afc 100644 --- a/atom/browser/web_contents_preferences.h +++ b/atom/browser/web_contents_preferences.h @@ -55,6 +55,9 @@ class WebContentsPreferences // Return true if the particular preference value exists. bool GetPreference(const base::StringPiece& name, std::string* value) const; + // Returns the preload script path. + bool GetPreloadPath(base::FilePath::StringType* path) const; + // Returns the web preferences. base::Value* preference() { return &preference_; } base::Value* last_preference() { return &last_preference_; } diff --git a/atom/renderer/atom_sandboxed_renderer_client.cc b/atom/renderer/atom_sandboxed_renderer_client.cc index a6734738bf..56bedabd56 100644 --- a/atom/renderer/atom_sandboxed_renderer_client.cc +++ b/atom/renderer/atom_sandboxed_renderer_client.cc @@ -177,16 +177,12 @@ void AtomSandboxedRendererClient::DidCreateScriptContext( if (!render_frame->IsMainFrame() && !IsDevTools(render_frame)) return; - base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); - base::FilePath preload_script_path = - command_line->GetSwitchValuePath(switches::kPreloadScript); - auto* isolate = context->GetIsolate(); v8::HandleScope handle_scope(isolate); v8::Context::Scope context_scope(context); // Wrap the bundle into a function that receives the binding object and the // preload script path as arguments. - std::string left = "(function(binding, preloadPath, require) {\n"; + std::string left = "(function(binding, require) {\n"; std::string right = "\n})"; // Compile the wrapper and run it to get the function object auto script = v8::Script::Compile(v8::String::Concat( @@ -199,10 +195,10 @@ void AtomSandboxedRendererClient::DidCreateScriptContext( auto binding = v8::Object::New(isolate); InitializeBindings(binding, context); AddRenderBindings(isolate, binding); - v8::Local args[] = { - binding, mate::ConvertToV8(isolate, preload_script_path.value())}; + v8::Local args[] = {binding}; // Execute the function with proper arguments - ignore_result(func->Call(context, v8::Null(isolate), 2, args)); + ignore_result( + func->Call(context, v8::Null(isolate), node::arraysize(args), args)); } void AtomSandboxedRendererClient::WillReleaseScriptContext( diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index 436c26a108..f19e281911 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -440,7 +440,8 @@ ipcMain.on('ELECTRON_BROWSER_WINDOW_CLOSE', function (event) { event.returnValue = null }) -ipcMain.on('ELECTRON_BROWSER_SANDBOX_LOAD', function (event, preloadPath) { +ipcMain.on('ELECTRON_BROWSER_SANDBOX_LOAD', function (event) { + const preloadPath = event.sender._getPreloadPath() let preloadSrc = null let preloadError = null if (preloadPath) { @@ -451,8 +452,8 @@ ipcMain.on('ELECTRON_BROWSER_SANDBOX_LOAD', function (event, preloadPath) { } } event.returnValue = { - preloadSrc: preloadSrc, - preloadError: preloadError, + preloadSrc, + preloadError, platform: process.platform, env: process.env } diff --git a/lib/sandboxed_renderer/init.js b/lib/sandboxed_renderer/init.js index 8afb2302a8..37dcefff66 100644 --- a/lib/sandboxed_renderer/init.js +++ b/lib/sandboxed_renderer/init.js @@ -1,5 +1,5 @@ /* eslint no-eval: "off" */ -/* global binding, preloadPath, Buffer */ +/* global binding, Buffer */ const events = require('events') const electron = require('electron') @@ -37,7 +37,7 @@ const loadedModules = new Map([ const { preloadSrc, preloadError, platform, env -} = electron.ipcRenderer.sendSync('ELECTRON_BROWSER_SANDBOX_LOAD', preloadPath) +} = electron.ipcRenderer.sendSync('ELECTRON_BROWSER_SANDBOX_LOAD') require('../renderer/web-frame-init')() diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index acad20a16b..a276f3cc0d 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -1409,6 +1409,8 @@ describe('BrowserWindow module', () => { preload: preload } }) + ipcRenderer.send('set-web-preferences-on-next-new-window', w.webContents.id, 'preload', preload) + let htmlPath = path.join(fixtures, 'api', 'sandbox.html?verify-ipc-sender') const pageUrl = 'file://' + htmlPath let childWc @@ -1587,6 +1589,8 @@ describe('BrowserWindow module', () => { sandbox: true } }) + ipcRenderer.send('set-web-preferences-on-next-new-window', w.webContents.id, 'preload', preload) + w.loadURL('file://' + path.join(fixtures, 'api', 'sandbox.html?reload-remote-child')) ipcMain.on('get-remote-module-path', (event) => {