From 8cf15cc9310ca5983f15e511c3ecdac8e75e225d Mon Sep 17 00:00:00 2001 From: Milan Burda Date: Thu, 28 Mar 2019 11:38:51 +0100 Subject: [PATCH] feat: only allow bundled preload scripts (#17308) --- docs/api/breaking-changes.md | 4 ++++ docs/api/browser-window.md | 2 ++ docs/api/sandbox-option.md | 2 +- docs/api/session.md | 2 ++ docs/api/webview-tag.md | 3 +++ docs/tutorial/security.md | 4 ++-- filenames.gni | 1 + lib/browser/rpc-server.js | 14 ++++++++++++++ lib/common/path-utils.ts | 6 ++++++ lib/renderer/init.ts | 13 +++++++++++++ spec/api-web-contents-spec.js | 31 +++++++++++++++++++++++++++++++ 11 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 lib/common/path-utils.ts diff --git a/docs/api/breaking-changes.md b/docs/api/breaking-changes.md index 8aded66183bf..e0e5298fb3cb 100644 --- a/docs/api/breaking-changes.md +++ b/docs/api/breaking-changes.md @@ -81,6 +81,10 @@ powerMonitor.querySystemIdleTime(callback) const idleTime = getSystemIdleTime() ``` +## Preload scripts outside of app path are not allowed + +For security reasons, preload scripts can only be loaded from a subpath of the [app path](app.md#appgetapppath). + # Planned Breaking API Changes (5.0) ## `new BrowserWindow({ webPreferences })` diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md index 9e8d1300b69f..6451845f05f7 100644 --- a/docs/api/browser-window.md +++ b/docs/api/browser-window.md @@ -266,6 +266,8 @@ It creates a new `BrowserWindow` with native properties as set by the `options`. When node integration is turned off, the preload script can reintroduce Node global symbols back to the global scope. See example [here](process.md#event-loaded). + **Note:** For security reasons, preload scripts can only be loaded from + a subpath of the [app path](app.md#appgetapppath). * `sandbox` Boolean (optional) - If set, this will sandbox the renderer associated with the window, making it compatible with the Chromium OS-level sandbox and disabling the Node.js engine. This is not the same as diff --git a/docs/api/sandbox-option.md b/docs/api/sandbox-option.md index 3c4cb1c7f26c..7d24bee28ac8 100644 --- a/docs/api/sandbox-option.md +++ b/docs/api/sandbox-option.md @@ -77,7 +77,7 @@ app.on('ready', () => { win = new BrowserWindow({ webPreferences: { sandbox: true, - preload: 'preload.js' + preload: path.join(app.getAppPath(), 'preload.js') } }) win.loadURL('http://google.com') diff --git a/docs/api/session.md b/docs/api/session.md index e01c8a2f0c8e..cf02b74f8538 100644 --- a/docs/api/session.md +++ b/docs/api/session.md @@ -561,6 +561,8 @@ Returns `Promise` - resolves when the session’s HTTP authentication cach Adds scripts that will be executed on ALL web contents that are associated with this session just before normal `preload` scripts run. +**Note:** For security reasons, preload scripts can only be loaded from a subpath of the [app path](app.md#appgetapppath). + #### `ses.getPreloads()` Returns `String[]` an array of paths to preload scripts that have been diff --git a/docs/api/webview-tag.md b/docs/api/webview-tag.md index 93ed322a8c3b..7c612b993ff4 100644 --- a/docs/api/webview-tag.md +++ b/docs/api/webview-tag.md @@ -162,6 +162,9 @@ When the guest page doesn't have node integration this script will still have access to all Node APIs, but global objects injected by Node will be deleted after this script has finished executing. +**Note:** For security reasons, preload scripts can only be loaded from +a subpath of the [app path](app.md#appgetapppath). + **Note:** This option will be appear as `preloadURL` (not `preload`) in the `webPreferences` specified to the `will-attach-webview` event. diff --git a/docs/tutorial/security.md b/docs/tutorial/security.md index fdd411bfcdc4..27ec922ab1fe 100644 --- a/docs/tutorial/security.md +++ b/docs/tutorial/security.md @@ -193,7 +193,7 @@ const mainWindow = new BrowserWindow({ webPreferences: { nodeIntegration: false, nodeIntegrationInWorker: false, - preload: './preload.js' + preload: path.join(app.getAppPath(), 'preload.js') } }) @@ -260,7 +260,7 @@ very small investment. const mainWindow = new BrowserWindow({ webPreferences: { contextIsolation: true, - preload: 'preload.js' + preload: path.join(app.getAppPath(), 'preload.js') } }) ``` diff --git a/filenames.gni b/filenames.gni index bff034ba1eca..81524e40bf56 100644 --- a/filenames.gni +++ b/filenames.gni @@ -61,6 +61,7 @@ filenames = { "lib/common/error-utils.js", "lib/common/init.ts", "lib/common/parse-features-string.js", + "lib/common/path-utils.ts", "lib/common/reset-search-paths.ts", "lib/common/web-view-methods.js", "lib/renderer/callbacks-registry.js", diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index b0c1a30168b4..ac8180f2b492 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -3,6 +3,7 @@ const electron = require('electron') const { EventEmitter } = require('events') const fs = require('fs') +const path = require('path') const util = require('util') const v8Util = process.electronBinding('v8_util') @@ -19,6 +20,7 @@ const guestViewManager = require('@electron/internal/browser/guest-view-manager' const bufferUtils = require('@electron/internal/common/buffer-utils') const errorUtils = require('@electron/internal/common/error-utils') const clipboardUtils = require('@electron/internal/common/clipboard-utils') +const { isParentDir } = require('@electron/internal/common/path-utils') const hasProp = {}.hasOwnProperty @@ -498,12 +500,24 @@ ipcMainUtils.handle('ELECTRON_BROWSER_CLIPBOARD', function (event, method, ...ar }) const readFile = util.promisify(fs.readFile) +const realpath = util.promisify(fs.realpath) + +let absoluteAppPath +const getAppPath = async function () { + if (absoluteAppPath === undefined) { + absoluteAppPath = await realpath(electron.app.getAppPath()) + } + return absoluteAppPath +} const getPreloadScript = async function (preloadPath) { let preloadSrc = null let preloadError = null if (preloadPath) { try { + if (!isParentDir(await getAppPath(), await realpath(preloadPath))) { + throw new Error('Preload scripts outside of app path are not allowed') + } preloadSrc = (await readFile(preloadPath)).toString() } catch (err) { preloadError = errorUtils.serialize(err) diff --git a/lib/common/path-utils.ts b/lib/common/path-utils.ts new file mode 100644 index 000000000000..a55e99941082 --- /dev/null +++ b/lib/common/path-utils.ts @@ -0,0 +1,6 @@ +import * as path from 'path' + +export const isParentDir = function (parent: string, dir: string) { + const relative = path.relative(parent, dir) + return !!relative && !relative.startsWith('..') && !path.isAbsolute(relative) +} diff --git a/lib/renderer/init.ts b/lib/renderer/init.ts index 8f0e1af51764..21b3b79254a2 100644 --- a/lib/renderer/init.ts +++ b/lib/renderer/init.ts @@ -1,4 +1,5 @@ import { EventEmitter } from 'events' +import * as fs from 'fs' import * as path from 'path' const Module = require('module') @@ -160,10 +161,22 @@ if (nodeIntegration) { } const errorUtils = require('@electron/internal/common/error-utils') +const { isParentDir } = require('@electron/internal/common/path-utils') + +let absoluteAppPath: string +const getAppPath = function () { + if (absoluteAppPath === undefined) { + absoluteAppPath = fs.realpathSync(appPath!) + } + return absoluteAppPath +} // Load the preload scripts. for (const preloadScript of preloadScripts) { try { + if (!isParentDir(getAppPath(), fs.realpathSync(preloadScript))) { + throw new Error('Preload scripts outside of app path are not allowed') + } require(preloadScript) } catch (error) { console.error(`Unable to load preload script: ${preloadScript}`) diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index e104157fe9a7..aa59acd15f93 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -3,6 +3,7 @@ const assert = require('assert') const ChildProcess = require('child_process') const fs = require('fs') +const os = require('os') const http = require('http') const path = require('path') const { closeWindow } = require('./window-helpers') @@ -1110,6 +1111,16 @@ describe('webContents module', () => { describe('preload-error event', () => { const generateSpecs = (description, sandbox) => { describe(description, () => { + const tmpPreload = path.join(os.tmpdir(), 'preload.js') + + before((done) => { + fs.writeFile(tmpPreload, '', done) + }) + + after((done) => { + fs.unlink(tmpPreload, () => done()) + }) + it('is triggered when unhandled exception is thrown', async () => { const preload = path.join(fixtures, 'module', 'preload-error-exception.js') @@ -1169,6 +1180,26 @@ describe('webContents module', () => { expect(preloadPath).to.equal(preload) expect(error.message).to.contain('preload-invalid.js') }) + + it('is triggered when preload script is outside of app path', async () => { + const preload = tmpPreload + + w.destroy() + w = new BrowserWindow({ + show: false, + webPreferences: { + sandbox, + preload + } + }) + + const promise = emittedOnce(w.webContents, 'preload-error') + w.loadURL('about:blank') + + const [, preloadPath, error] = await promise + expect(preloadPath).to.equal(preload) + expect(error.message).to.contain('Preload scripts outside of app path are not allowed') + }) }) }