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..0c2a433e64ca 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.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,37 @@ 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) return + + if (canAccessWindow(event.sender, guestWindow.webContents)) { + guestWindow.destroy() + } else { + console.error(`Blocked ${event.sender.getURL()} from closing its opener.`) + } }) 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) { + 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} on its opener.`) + 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 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) @@ -191,5 +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) guestContents[method].apply(guestContents, args) + if (guestContents == null) return + + if (canAccessWindow(event.sender, guestContents)) { + guestContents[method].apply(guestContents, args) + } else { + console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`) + } }) diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index d37fb3c0cd25..a63bba1b4837 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) + + const scheme = 'other' + let url = `${scheme}://${fixtures}/pages/window-opener-location.html` + let w = null + + before(function (done) { + protocol.registerFileProtocol(scheme, function (request, callback) { + callback(`${fixtures}/pages/window-opener-location.html`) + }, function (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 @@ + + + + +