From a1dfdbdde426f2f32f45582b2408a676a701e306 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 15 Nov 2016 16:47:28 +0900 Subject: [PATCH 1/8] spec: Should check origin before accessing window.opener --- spec/chromium-spec.js | 50 +++++++++++++++++-- .../pages/window-opener-location.html | 7 +++ 2 files changed, 53 insertions(+), 4 deletions(-) create mode 100644 spec/fixtures/pages/window-opener-location.html diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index d37fb3c0cd25..d940017736e5 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -283,11 +283,11 @@ describe('chromium feature', function () { describe('window.opener', function () { this.timeout(10000) - var url = 'file://' + fixtures + '/pages/window-opener.html' - var w = null + let url = 'file://' + fixtures + '/pages/window-opener.html' + let w = null afterEach(function () { - w != null ? w.destroy() : void 0 + if (w) w.destroy() }) it('is null for main window', function (done) { @@ -302,7 +302,7 @@ describe('chromium feature', function () { }) it('is not null for window opened by window.open', function (done) { - var b + let b listener = function (event) { assert.equal(event.data, 'object') b.close() @@ -313,6 +313,48 @@ describe('chromium feature', function () { }) }) + describe('window.opener security', function () { + this.timeout(10000) + + let scheme = 'other' + let url = `${scheme}://${fixtures}/pages/window-opener-location.html` + let w = null + + before(function () { + protocol.registerFileProtocol(scheme, function (request, callback) { + callback(`${fixtures}/pages/window-opener-location.html`) + }, function (error) { + if (error) done(error) + }) + }) + + after(function() { + protocol.unregisterProtocol(scheme) + }) + + afterEach(function () { + w.close() + }) + + it('does nothing when origin of current window does not match opener', function (done) { + listener = function (event) { + assert.equal(event.data, undefined) + done() + } + window.addEventListener('message', listener) + w = window.open(url, '', 'show=no') + }) + + it('works when origin does not match opener but has node integration', function (done) { + listener = function (event) { + assert.equal(event.data, location.href) + done() + } + window.addEventListener('message', listener) + w = window.open(url, '', 'show=no,nodeIntegration=yes') + }) + }) + describe('window.postMessage', function () { it('sets the source and origin correctly', function (done) { var b, sourceId diff --git a/spec/fixtures/pages/window-opener-location.html b/spec/fixtures/pages/window-opener-location.html new file mode 100644 index 000000000000..c1594e667de3 --- /dev/null +++ b/spec/fixtures/pages/window-opener-location.html @@ -0,0 +1,7 @@ + + + + + From a1066617a8fa321664601f0407c7ccbed96eb0c4 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 15 Nov 2016 17:45:34 +0900 Subject: [PATCH 2/8] Do permission check when calling guest window methods --- atom/common/api/atom_api_v8_util.cc | 7 ++++++ lib/browser/guest-window-manager.js | 34 ++++++++++++++++++++++++++--- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/atom/common/api/atom_api_v8_util.cc b/atom/common/api/atom_api_v8_util.cc index a587cd772a0c..a01ca8f84d91 100644 --- a/atom/common/api/atom_api_v8_util.cc +++ b/atom/common/api/atom_api_v8_util.cc @@ -9,9 +9,11 @@ #include "atom/common/api/remote_callback_freer.h" #include "atom/common/api/remote_object_freer.h" #include "atom/common/native_mate_converters/content_converter.h" +#include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/node_includes.h" #include "base/hash.h" #include "native_mate/dictionary.h" +#include "url/origin.h" #include "v8/include/v8-profiler.h" namespace std { @@ -97,6 +99,10 @@ void RequestGarbageCollectionForTesting(v8::Isolate* isolate) { v8::Isolate::GarbageCollectionType::kFullGarbageCollection); } +bool IsSameOrigin(const GURL& l, const GURL& r) { + return url::Origin(l).IsSameOriginWith(url::Origin(r)); +} + void Initialize(v8::Local exports, v8::Local unused, v8::Local context, void* priv) { mate::Dictionary dict(context->GetIsolate(), exports); @@ -112,6 +118,7 @@ void Initialize(v8::Local exports, v8::Local unused, &atom::api::KeyWeakMap>::Create); dict.SetMethod("requestGarbageCollectionForTesting", &RequestGarbageCollectionForTesting); + dict.SetMethod("isSameOrigin", &IsSameOrigin); } } // namespace diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index d7d169f40118..34286815af66 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -1,6 +1,7 @@ 'use strict' const {BrowserWindow, ipcMain, webContents} = require('electron') +const {isSameOrigin} = process.atomBinding('v8_util') const hasProp = {}.hasOwnProperty const frameToGuest = {} @@ -151,6 +152,22 @@ const getGuestWindow = function (guestId) { return guestWindow } +// Checks whether |sender| can access the |target|: +// 1. Check whether |sender| is the parent of |target|. +// 2. Check whether |sender| has node integration, if so it is allowed to +// do anything it wants. +// 3. Check whether the origins match. +// +// However it allows a child window without node integration but with same +// origin to do anything it wants, when its opener window has node integration. +// The W3C does not have anything on this, but from my understanding of the +// security model of |window.opener|, this should be fine. +const canAccessWindow = function (sender, target) { + return (target.getWebPreferences().openerId === sender.webContents.id) || + (sender.getWebPreferences().nodeIntegration === true) || + isSameOrigin(sender.getURL(), target.getURL()) +} + // Routed window.open messages. ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', function (event, url, frameName, disposition, options, @@ -171,18 +188,27 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', function (event, url, fr ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', function (event, guestId) { const guestWindow = getGuestWindow(guestId) - if (guestWindow != null) guestWindow.destroy() + if (guestWindow != null && canAccessWindow(event.sender, guestWindow.webContents)) { + guestWindow.destroy() + } }) ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', function (event, guestId, method, ...args) { const guestWindow = getGuestWindow(guestId) - event.returnValue = guestWindow != null ? guestWindow[method].apply(guestWindow, args) : null + if (guestWindow != null && canAccessWindow(event.sender, guestWindow.webContents)) { + event.returnValue = guestWindow[method].apply(guestWindow, args) + } else { + event.returnValue = null + } }) ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function (event, guestId, message, targetOrigin, sourceOrigin) { const guestContents = webContents.fromId(guestId) if (guestContents == null) return + // The W3C does not seem to have word on how postMessage should work when the + // origins do not match, so we do not do |canAccessWindow| check here since + // postMessage across origins is usefull and not harmful. if (guestContents.getURL().indexOf(targetOrigin) === 0 || targetOrigin === '*') { const sourceId = event.sender.id guestContents.send('ELECTRON_GUEST_WINDOW_POSTMESSAGE', sourceId, message, sourceOrigin) @@ -191,5 +217,7 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function (event, ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', function (event, guestId, method, ...args) { const guestContents = webContents.fromId(guestId) - if (guestContents != null) guestContents[method].apply(guestContents, args) + if (guestContents != null && canAccessWindow(event.sender, guestContents)) { + guestContents[method].apply(guestContents, args) + } }) From 18fca785c411f823a5f60d13d9da4a8db0eb3ddf Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 16 Nov 2016 09:51:43 +0900 Subject: [PATCH 3/8] Print error messages --- lib/browser/guest-window-manager.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 34286815af66..14936aa23e13 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -190,6 +190,8 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', function (event, guestI const guestWindow = getGuestWindow(guestId) if (guestWindow != null && canAccessWindow(event.sender, guestWindow.webContents)) { guestWindow.destroy() + } else { + console.error(`Blocked ${event.sender.getURL()} from closing its opener.`) } }) @@ -198,6 +200,7 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', function (event, guest if (guestWindow != null && canAccessWindow(event.sender, guestWindow.webContents)) { event.returnValue = guestWindow[method].apply(guestWindow, args) } else { + console.error(`Blocked ${event.sender.getURL()} from calling ${method} of its opener.`) event.returnValue = null } }) @@ -219,5 +222,7 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', function (event, const guestContents = webContents.fromId(guestId) if (guestContents != null && canAccessWindow(event.sender, guestContents)) { guestContents[method].apply(guestContents, args) + } else { + console.error(`Blocked ${event.sender.getURL()} from calling ${method} of its opener.`) } }) From 81f2e76e36ab70b7ccbf617beaca3276cc6bc6b7 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 16 Nov 2016 09:57:12 +0900 Subject: [PATCH 4/8] Fix standard linting errors --- spec/chromium-spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index d940017736e5..29846e25815d 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -320,7 +320,7 @@ describe('chromium feature', function () { let url = `${scheme}://${fixtures}/pages/window-opener-location.html` let w = null - before(function () { + before(function (done) { protocol.registerFileProtocol(scheme, function (request, callback) { callback(`${fixtures}/pages/window-opener-location.html`) }, function (error) { @@ -328,7 +328,7 @@ describe('chromium feature', function () { }) }) - after(function() { + after(function () { protocol.unregisterProtocol(scheme) }) From 04c68745db219d089b1eef6048fdb7eebad80220 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 15 Nov 2016 17:35:57 -0800 Subject: [PATCH 5/8] Always call done callback in before block --- spec/chromium-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index 29846e25815d..a616a5c45201 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -324,7 +324,7 @@ describe('chromium feature', function () { protocol.registerFileProtocol(scheme, function (request, callback) { callback(`${fixtures}/pages/window-opener-location.html`) }, function (error) { - if (error) done(error) + done(error) }) }) From 92577c37c8037a2de562faa621b880607105d503 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 15 Nov 2016 17:41:15 -0800 Subject: [PATCH 6/8] Don't log blocked messages when guestWindow is null --- lib/browser/guest-window-manager.js | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 14936aa23e13..6c7518b7c062 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -188,7 +188,9 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', function (event, url, fr ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', function (event, guestId) { const guestWindow = getGuestWindow(guestId) - if (guestWindow != null && canAccessWindow(event.sender, guestWindow.webContents)) { + if (guestWindow == null) return + + if (canAccessWindow(event.sender, guestWindow.webContents)) { guestWindow.destroy() } else { console.error(`Blocked ${event.sender.getURL()} from closing its opener.`) @@ -197,10 +199,15 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', function (event, guestI ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', function (event, guestId, method, ...args) { const guestWindow = getGuestWindow(guestId) - if (guestWindow != null && canAccessWindow(event.sender, guestWindow.webContents)) { + if (guestWindow == null) { + event.returnValue = null + return + } + + if (canAccessWindow(event.sender, guestWindow.webContents)) { event.returnValue = guestWindow[method].apply(guestWindow, args) } else { - console.error(`Blocked ${event.sender.getURL()} from calling ${method} of its opener.`) + console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`) event.returnValue = null } }) @@ -211,7 +218,7 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function (event, // The W3C does not seem to have word on how postMessage should work when the // origins do not match, so we do not do |canAccessWindow| check here since - // postMessage across origins is usefull and not harmful. + // postMessage across origins is useful and not harmful. if (guestContents.getURL().indexOf(targetOrigin) === 0 || targetOrigin === '*') { const sourceId = event.sender.id guestContents.send('ELECTRON_GUEST_WINDOW_POSTMESSAGE', sourceId, message, sourceOrigin) @@ -220,9 +227,11 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function (event, ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', function (event, guestId, method, ...args) { const guestContents = webContents.fromId(guestId) - if (guestContents != null && canAccessWindow(event.sender, guestContents)) { + if (guestContents == null) return + + if (canAccessWindow(event.sender, guestContents)) { guestContents[method].apply(guestContents, args) } else { - console.error(`Blocked ${event.sender.getURL()} from calling ${method} of its opener.`) + console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`) } }) From 651eaec64f6635ea8b16ad77b535d8033ceb9e46 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 16 Nov 2016 07:50:24 -0800 Subject: [PATCH 7/8] Use sender.id instead of sender.webContents.id --- lib/browser/guest-window-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 6c7518b7c062..0c2a433e64ca 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -163,7 +163,7 @@ const getGuestWindow = function (guestId) { // The W3C does not have anything on this, but from my understanding of the // security model of |window.opener|, this should be fine. const canAccessWindow = function (sender, target) { - return (target.getWebPreferences().openerId === sender.webContents.id) || + return (target.getWebPreferences().openerId === sender.id) || (sender.getWebPreferences().nodeIntegration === true) || isSameOrigin(sender.getURL(), target.getURL()) } From aa2824621a1e1c3ee30a1329f3bfe9309fb16872 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 16 Nov 2016 08:03:07 -0800 Subject: [PATCH 8/8] Make scheme const --- spec/chromium-spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index a616a5c45201..a63bba1b4837 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -316,7 +316,7 @@ describe('chromium feature', function () { describe('window.opener security', function () { this.timeout(10000) - let scheme = 'other' + const scheme = 'other' let url = `${scheme}://${fixtures}/pages/window-opener-location.html` let w = null