From a0cdcc5f8dc31dbcdd0f48a36b2fb3670a5d5105 Mon Sep 17 00:00:00 2001
From: Milan Burda <milan.burda@gmail.com>
Date: Thu, 3 Jan 2019 19:31:10 +0100
Subject: [PATCH] security: improve IPC validation in guest-view-manager

---
 lib/browser/guest-view-manager.js | 52 +++++++++++++++++++++----------
 lib/browser/rpc-server.js         |  5 +--
 2 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js
index e11b94e8685e..e05229afc077 100644
--- a/lib/browser/guest-view-manager.js
+++ b/lib/browser/guest-view-manager.js
@@ -184,9 +184,12 @@ const attachGuest = function (event, embedderFrameId, elementInstanceId, guestIn
   const guestInstance = guestInstances[guestInstanceId]
   // If this isn't a valid guest instance then do nothing.
   if (!guestInstance) {
-    return
+    throw new Error(`Invalid guestInstanceId: ${guestInstanceId}`)
   }
   const { guest } = guestInstance
+  if (guest.hostWebContents !== event.sender) {
+    throw new Error(`Access denied to guestInstanceId: ${guestInstanceId}`)
+  }
 
   // If this guest is already attached to an element then remove it
   if (guestInstance.elementInstanceId) {
@@ -351,26 +354,35 @@ handleMessage('ELECTRON_GUEST_VIEW_MANAGER_CREATE_GUEST_SYNC', function (event,
 })
 
 handleMessage('ELECTRON_GUEST_VIEW_MANAGER_DESTROY_GUEST', function (event, guestInstanceId) {
-  const guest = getGuest(guestInstanceId)
-  if (guest) {
+  try {
+    const guest = getGuestForWebContents(guestInstanceId, event.sender)
     guest.destroy()
+  } catch (error) {
+    console.error(`Guest destroy failed: ${error}`)
   }
 })
 
 handleMessage('ELECTRON_GUEST_VIEW_MANAGER_ATTACH_GUEST', function (event, embedderFrameId, elementInstanceId, guestInstanceId, params) {
-  attachGuest(event, embedderFrameId, elementInstanceId, guestInstanceId, params)
+  try {
+    attachGuest(event, embedderFrameId, elementInstanceId, guestInstanceId, params)
+  } catch (error) {
+    console.error(`Guest attach failed: ${error}`)
+  }
 })
 
-handleMessage('ELECTRON_GUEST_VIEW_MANAGER_FOCUS_CHANGE', function (event, focus, guestInstanceId) {
-  event.sender.emit('focus-change', {}, focus, guestInstanceId)
+// this message is sent by the actual <webview>
+ipcMain.on('ELECTRON_GUEST_VIEW_MANAGER_FOCUS_CHANGE', function (event, focus, guestInstanceId) {
+  const guest = getGuest(guestInstanceId)
+  if (guest === event.sender) {
+    event.sender.emit('focus-change', {}, focus, guestInstanceId)
+  } else {
+    console.error(`focus-change for guestInstanceId: ${guestInstanceId}`)
+  }
 })
 
 handleMessage('ELECTRON_GUEST_VIEW_MANAGER_ASYNC_CALL', function (event, requestId, guestInstanceId, method, args, hasCallback) {
   new Promise(resolve => {
-    const guest = getGuest(guestInstanceId)
-    if (guest.hostWebContents !== event.sender) {
-      throw new Error(`Invalid guestInstanceId: ${guestInstanceId}`)
-    }
+    const guest = getGuestForWebContents(guestInstanceId, event.sender)
     if (!asyncMethods.has(method)) {
       throw new Error(`Invalid method: ${method}`)
     }
@@ -390,10 +402,7 @@ handleMessage('ELECTRON_GUEST_VIEW_MANAGER_ASYNC_CALL', function (event, request
 
 handleMessage('ELECTRON_GUEST_VIEW_MANAGER_SYNC_CALL', function (event, guestInstanceId, method, args) {
   try {
-    const guest = getGuest(guestInstanceId)
-    if (guest.hostWebContents !== event.sender) {
-      throw new Error(`Invalid guestInstanceId: ${guestInstanceId}`)
-    }
+    const guest = getGuestForWebContents(guestInstanceId, event.sender)
     if (!syncMethods.has(method)) {
       throw new Error(`Invalid method: ${method}`)
     }
@@ -403,6 +412,18 @@ handleMessage('ELECTRON_GUEST_VIEW_MANAGER_SYNC_CALL', function (event, guestIns
   }
 })
 
+// Returns WebContents from its guest id hosted in given webContents.
+const getGuestForWebContents = function (guestInstanceId, contents) {
+  const guest = getGuest(guestInstanceId)
+  if (!guest) {
+    throw new Error(`Invalid guestInstanceId: ${guestInstanceId}`)
+  }
+  if (guest.hostWebContents !== contents) {
+    throw new Error(`Access denied to guestInstanceId: ${guestInstanceId}`)
+  }
+  return guest
+}
+
 // Returns WebContents from its guest id.
 const getGuest = function (guestInstanceId) {
   const guestInstance = guestInstances[guestInstanceId]
@@ -415,5 +436,4 @@ const getEmbedder = function (guestInstanceId) {
   if (guestInstance != null) return guestInstance.embedder
 }
 
-exports.getGuest = getGuest
-exports.getEmbedder = getEmbedder
+exports.getGuestForWebContents = getGuestForWebContents
diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js
index e0249cff766f..d318ec7cf6d9 100644
--- a/lib/browser/rpc-server.js
+++ b/lib/browser/rpc-server.js
@@ -13,6 +13,7 @@ const { isPromise } = electron
 
 const ipcMain = require('@electron/internal/browser/ipc-main-internal')
 const objectsRegistry = require('@electron/internal/browser/objects-registry')
+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')
 
@@ -411,8 +412,8 @@ handleRemoteCommand('ELECTRON_BROWSER_CONTEXT_RELEASE', (event, contextId) => {
 })
 
 handleRemoteCommand('ELECTRON_BROWSER_GUEST_WEB_CONTENTS', function (event, contextId, guestInstanceId) {
-  const guestViewManager = require('@electron/internal/browser/guest-view-manager')
-  return valueToMeta(event.sender, contextId, guestViewManager.getGuest(guestInstanceId))
+  const guest = guestViewManager.getGuestForWebContents(guestInstanceId, event.sender)
+  return valueToMeta(event.sender, contextId, guest)
 })
 
 // Implements window.close()