From aa2b2f7c8f3150a5c09b53788eef06a3e710cc4b Mon Sep 17 00:00:00 2001 From: Milan Burda Date: Tue, 4 Dec 2018 16:12:21 +0100 Subject: [PATCH] fix: security: don't allow arbitrary methods to be invoked on webContents via IPC (#15919) --- filenames.gni | 1 + lib/browser/guest-view-manager.js | 13 ++++-- lib/browser/guest-window-manager.js | 21 +++++++-- lib/browser/navigation-controller.js | 16 +++++-- lib/common/web-view-methods.js | 67 ++++++++++++++++++++++++++ lib/renderer/web-view/web-view.js | 70 ++-------------------------- lib/renderer/window-setup.js | 17 ++----- 7 files changed, 115 insertions(+), 90 deletions(-) create mode 100644 lib/common/web-view-methods.js diff --git a/filenames.gni b/filenames.gni index 6864819a53b98..b01a65746b61b 100644 --- a/filenames.gni +++ b/filenames.gni @@ -62,6 +62,7 @@ filenames = { "lib/common/init.js", "lib/common/parse-features-string.js", "lib/common/reset-search-paths.js", + "lib/common/web-view-methods.js", "lib/renderer/callbacks-registry.js", "lib/renderer/chrome-api.js", "lib/renderer/content-scripts-injector.js", diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js index 2823a7f5887de..e11b94e8685ea 100644 --- a/lib/browser/guest-view-manager.js +++ b/lib/browser/guest-view-manager.js @@ -4,6 +4,7 @@ const { webContents } = require('electron') const ipcMain = require('@electron/internal/browser/ipc-main-internal') const parseFeaturesString = require('@electron/internal/common/parse-features-string') const errorUtils = require('@electron/internal/common/error-utils') +const { syncMethods, asyncMethods } = require('@electron/internal/common/web-view-methods') // Doesn't exist in early initialization. let webViewManager = null @@ -368,7 +369,10 @@ handleMessage('ELECTRON_GUEST_VIEW_MANAGER_ASYNC_CALL', function (event, request new Promise(resolve => { const guest = getGuest(guestInstanceId) if (guest.hostWebContents !== event.sender) { - throw new Error('Access denied') + throw new Error(`Invalid guestInstanceId: ${guestInstanceId}`) + } + if (!asyncMethods.has(method)) { + throw new Error(`Invalid method: ${method}`) } if (hasCallback) { guest[method](...args, resolve) @@ -388,9 +392,12 @@ handleMessage('ELECTRON_GUEST_VIEW_MANAGER_SYNC_CALL', function (event, guestIns try { const guest = getGuest(guestInstanceId) if (guest.hostWebContents !== event.sender) { - throw new Error('Access denied') + throw new Error(`Invalid guestInstanceId: ${guestInstanceId}`) + } + if (!syncMethods.has(method)) { + throw new Error(`Invalid method: ${method}`) } - event.returnValue = [null, guest[method].apply(guest, args)] + event.returnValue = [null, guest[method](...args)] } catch (error) { event.returnValue = [errorUtils.serialize(error)] } diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 5bc5d26be6664..6a71e94ce4101 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -288,6 +288,11 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', function (event, guestI if (guestWindow != null) guestWindow.destroy() }) +const windowMethods = new Set([ + 'focus', + 'blur' +]) + ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', function (event, guestId, method, ...args) { const guestContents = webContents.fromId(guestId) if (guestContents == null) { @@ -295,7 +300,7 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', function (event, guest return } - if (!canAccessWindow(event.sender, guestContents)) { + if (!canAccessWindow(event.sender, guestContents) || !windowMethods.has(method)) { console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`) event.returnValue = null return @@ -326,17 +331,27 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function (event, } }) +const webContentsMethods = new Set([ + 'print', + 'executeJavaScript' +]) + ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', function (event, guestId, method, ...args) { const guestContents = webContents.fromId(guestId) if (guestContents == null) return - if (canAccessWindow(event.sender, guestContents)) { + if (canAccessWindow(event.sender, guestContents) && webContentsMethods.has(method)) { guestContents[method](...args) } else { console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`) } }) +const webContentsSyncMethods = new Set([ + 'getURL', + 'loadURL' +]) + ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD_SYNC', function (event, guestId, method, ...args) { const guestContents = webContents.fromId(guestId) if (guestContents == null) { @@ -344,7 +359,7 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD_SYNC', function (e return } - if (canAccessWindow(event.sender, guestContents)) { + if (canAccessWindow(event.sender, guestContents) && webContentsSyncMethods.has(method)) { event.returnValue = guestContents[method](...args) } else { console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`) diff --git a/lib/browser/navigation-controller.js b/lib/browser/navigation-controller.js index 1863961d06470..a9977141b5150 100644 --- a/lib/browser/navigation-controller.js +++ b/lib/browser/navigation-controller.js @@ -3,12 +3,20 @@ const ipcMain = require('@electron/internal/browser/ipc-main-internal') // The history operation in renderer is redirected to browser. -ipcMain.on('ELECTRON_NAVIGATION_CONTROLLER', function (event, method, ...args) { - event.sender[method](...args) +ipcMain.on('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK', function (event) { + event.sender.goBack() }) -ipcMain.on('ELECTRON_SYNC_NAVIGATION_CONTROLLER', function (event, method, ...args) { - event.returnValue = event.sender[method](...args) +ipcMain.on('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD', function (event) { + event.sender.goForward() +}) + +ipcMain.on('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', function (event, offset) { + event.sender.goToOffset(offset) +}) + +ipcMain.on('ELECTRON_NAVIGATION_CONTROLLER_LENGTH', function (event) { + event.returnValue = event.sender.length() }) // JavaScript implementation of Chromium's NavigationController. diff --git a/lib/common/web-view-methods.js b/lib/common/web-view-methods.js new file mode 100644 index 0000000000000..8e40c9ede995f --- /dev/null +++ b/lib/common/web-view-methods.js @@ -0,0 +1,67 @@ +'use strict' + +// Public-facing API methods. +exports.syncMethods = new Set([ + 'getURL', + 'loadURL', + 'getTitle', + 'isLoading', + 'isLoadingMainFrame', + 'isWaitingForResponse', + 'stop', + 'reload', + 'reloadIgnoringCache', + 'canGoBack', + 'canGoForward', + 'canGoToOffset', + 'clearHistory', + 'goBack', + 'goForward', + 'goToIndex', + 'goToOffset', + 'isCrashed', + 'setUserAgent', + 'getUserAgent', + 'openDevTools', + 'closeDevTools', + 'isDevToolsOpened', + 'isDevToolsFocused', + 'inspectElement', + 'setAudioMuted', + 'isAudioMuted', + 'isCurrentlyAudible', + 'undo', + 'redo', + 'cut', + 'copy', + 'paste', + 'pasteAndMatchStyle', + 'delete', + 'selectAll', + 'unselect', + 'replace', + 'replaceMisspelling', + 'findInPage', + 'stopFindInPage', + 'downloadURL', + 'inspectServiceWorker', + 'showDefinitionForSelection', + 'setZoomFactor', + 'setZoomLevel' +]) + +exports.asyncMethods = new Set([ + 'insertCSS', + 'insertText', + 'send', + 'sendInputEvent', + 'setLayoutZoomLevelLimits', + 'setVisualZoomLevelLimits', + // with callback + 'capturePage', + 'executeJavaScript', + 'getZoomFactor', + 'getZoomLevel', + 'print', + 'printToPDF' +]) diff --git a/lib/renderer/web-view/web-view.js b/lib/renderer/web-view/web-view.js index cf07522fcfdb7..2c20f82c71f56 100644 --- a/lib/renderer/web-view/web-view.js +++ b/lib/renderer/web-view/web-view.js @@ -7,6 +7,7 @@ const ipcRenderer = require('@electron/internal/renderer/ipc-renderer-internal') const guestViewInternal = require('@electron/internal/renderer/web-view/guest-view-internal') const webViewConstants = require('@electron/internal/renderer/web-view/web-view-constants') const errorUtils = require('@electron/internal/common/error-utils') +const { syncMethods, asyncMethods } = require('@electron/internal/common/web-view-methods') // ID generator. let nextId = 0 @@ -230,71 +231,6 @@ const registerWebViewElement = function () { } } - // Public-facing API methods. - const methods = [ - 'getURL', - 'loadURL', - 'getTitle', - 'isLoading', - 'isLoadingMainFrame', - 'isWaitingForResponse', - 'stop', - 'reload', - 'reloadIgnoringCache', - 'canGoBack', - 'canGoForward', - 'canGoToOffset', - 'clearHistory', - 'goBack', - 'goForward', - 'goToIndex', - 'goToOffset', - 'isCrashed', - 'setUserAgent', - 'getUserAgent', - 'openDevTools', - 'closeDevTools', - 'isDevToolsOpened', - 'isDevToolsFocused', - 'inspectElement', - 'setAudioMuted', - 'isAudioMuted', - 'isCurrentlyAudible', - 'undo', - 'redo', - 'cut', - 'copy', - 'paste', - 'pasteAndMatchStyle', - 'delete', - 'selectAll', - 'unselect', - 'replace', - 'replaceMisspelling', - 'findInPage', - 'stopFindInPage', - 'downloadURL', - 'inspectServiceWorker', - 'showDefinitionForSelection', - 'setZoomFactor', - 'setZoomLevel' - ] - const nonblockMethods = [ - 'insertCSS', - 'insertText', - 'send', - 'sendInputEvent', - 'setLayoutZoomLevelLimits', - 'setVisualZoomLevelLimits', - // with callback - 'capturePage', - 'executeJavaScript', - 'getZoomFactor', - 'getZoomLevel', - 'print', - 'printToPDF' - ] - const getGuestInstanceId = function (self) { const internal = v8Util.getHiddenValue(self, 'internal') if (!internal.guestInstanceId) { @@ -314,7 +250,7 @@ const registerWebViewElement = function () { } } } - for (const method of methods) { + for (const method of syncMethods) { proto[method] = createBlockHandler(method) } @@ -332,7 +268,7 @@ const registerWebViewElement = function () { ipcRenderer.send('ELECTRON_GUEST_VIEW_MANAGER_ASYNC_CALL', requestId, getGuestInstanceId(this), method, args, callback != null) } } - for (const method of nonblockMethods) { + for (const method of asyncMethods) { proto[method] = createNonBlockHandler(method) } diff --git a/lib/renderer/window-setup.js b/lib/renderer/window-setup.js index 3da16bad7d407..3be08d9177f27 100644 --- a/lib/renderer/window-setup.js +++ b/lib/renderer/window-setup.js @@ -146,15 +146,6 @@ function BrowserWindowProxy (ipcRenderer, guestId) { } } -// Forward history operations to browser. -const sendHistoryOperation = function (ipcRenderer, ...args) { - ipcRenderer.send('ELECTRON_NAVIGATION_CONTROLLER', ...args) -} - -const getHistoryOperation = function (ipcRenderer, ...args) { - return ipcRenderer.sendSync('ELECTRON_SYNC_NAVIGATION_CONTROLLER', ...args) -} - module.exports = (ipcRenderer, guestInstanceId, openerId, hiddenPage, usesNativeWindowOpen) => { if (guestInstanceId == null) { // Override default window.close. @@ -199,20 +190,20 @@ module.exports = (ipcRenderer, guestInstanceId, openerId, hiddenPage, usesNative }) window.history.back = function () { - sendHistoryOperation(ipcRenderer, 'goBack') + ipcRenderer.send('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK') } window.history.forward = function () { - sendHistoryOperation(ipcRenderer, 'goForward') + ipcRenderer.send('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD') } window.history.go = function (offset) { - sendHistoryOperation(ipcRenderer, 'goToOffset', +offset) + ipcRenderer.send('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', +offset) } defineProperty(window.history, 'length', { get: function () { - return getHistoryOperation(ipcRenderer, 'length') + return ipcRenderer.sendSync('ELECTRON_NAVIGATION_CONTROLLER_LENGTH') } })