security: don't allow arbitrary methods to be invoked on webContents (#…

…16123)
electron · Jan 3, 2019 · 73e3667 · 73e3667
1 parent d50bd80
commit 73e3667
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 90 deletions.
diff --git 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/api/navigation-controller.js b/lib/browser/api/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/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
@@ -376,7 +377,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)
@@ -396,9 +400,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
@@ -288,14 +288,19 @@ 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) {
     event.returnValue = null
     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,25 +331,35 @@ 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) {
     event.returnValue = null
     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/common/web-view-methods.js 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
@@ -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 = (window) => {
     }
   }
 
-  // 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 = (window) => {
       }
     }
   }
-  for (const method of methods) {
+  for (const method of syncMethods) {
     proto[method] = createBlockHandler(method)
   }
 
@@ -332,7 +268,7 @@ const registerWebViewElement = (window) => {
       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
@@ -97,15 +97,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.
@@ -150,20 +141,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')
     }
   })