From d586ef2f39d650f117054f32a523fd9786087e9f Mon Sep 17 00:00:00 2001 From: Felix Rieseberg Date: Sat, 3 Feb 2018 06:50:12 -0800 Subject: [PATCH] feature: Hot security tips (#11810) * :wrench: Add security issue detection (and logs) * :wrench: Check for it on load * :construction_worker: Add some tests * :construction_worker: Make the linter happy * :wrench: Allow them to be enabled by force * :memo: Make message slightly prettier * :wrench: Fix a typo in the code comment * :wrench: Classic mistake * :rocket: Optimize things a bit more * :construction_worker: Add tests, fix tests * :memo: Document things * :wrench: Make linter happy * :wrench: One more piece of cleanup --- docs/tutorial/security.md | 15 +- filenames.gypi | 1 + lib/renderer/init.js | 42 ++- lib/renderer/security-warnings.js | 277 +++++++++++++++++++ script/test.py | 3 +- spec/fixtures/pages/base-page-security.html | 9 + spec/fixtures/pages/insecure-resources.html | 10 + spec/fixtures/pages/webview-allowpopups.html | 10 + spec/security-warnings-spec.js | 185 +++++++++++++ spec/static/main.js | 3 + 10 files changed, 541 insertions(+), 14 deletions(-) create mode 100644 lib/renderer/security-warnings.js create mode 100644 spec/fixtures/pages/base-page-security.html create mode 100644 spec/fixtures/pages/insecure-resources.html create mode 100644 spec/fixtures/pages/webview-allowpopups.html create mode 100644 spec/security-warnings-spec.js diff --git a/docs/tutorial/security.md b/docs/tutorial/security.md index 0135af77ee34..8ce06fc7e07f 100644 --- a/docs/tutorial/security.md +++ b/docs/tutorial/security.md @@ -54,9 +54,20 @@ Node.js integration enabled. Instead, use only local files (packaged together with your application) to execute Node.js code. To display remote content, use the [`webview`][web-view] tag and make sure to disable the `nodeIntegration`. -#### Checklist: Security Recommendations +## Electron Security Warnings -This is not bulletproof, but at the least, you should attempt the following: +From Electron 2.0 on, developers will see warnings and recommendations printed +to the developer console. They only show op when the binary's name is Electron, +indicating that a developer is currently looking at the console. + +You can force-enable or force-disable these warnings by setting +`ELECTRON_ENABLE_SECURITY_WARNINGS` or `ELECTRON_DISABLE_SECURITY_WARNINGS` on +either `process.env` or the `window` object. + +## Checklist: Security Recommendations + +This is not bulletproof, but at the least, you should follow these steps to +improve the security of your application. 1) [Only load secure content](#only-load-secure-content) 2) [Disable the Node.js integration in all renderers that display remote content](#disable-node.js-integration-for-remote-content) diff --git a/filenames.gypi b/filenames.gypi index 8031a305f490..e901ad04e834 100644 --- a/filenames.gypi +++ b/filenames.gypi @@ -63,6 +63,7 @@ 'lib/renderer/init.js', 'lib/renderer/inspector.js', 'lib/renderer/override.js', + 'lib/renderer/security-warnings.js', 'lib/renderer/window-setup.js', 'lib/renderer/web-view/guest-view-internal.js', 'lib/renderer/web-view/web-view.js', diff --git a/lib/renderer/init.js b/lib/renderer/init.js index 1b0766d3007d..1cb69436dd47 100644 --- a/lib/renderer/init.js +++ b/lib/renderer/init.js @@ -28,6 +28,18 @@ v8Util.setHiddenValue(global, 'ipc', new events.EventEmitter()) // Use electron module after everything is ready. const electron = require('electron') +const { + warnAboutNodeWithRemoteContent, + warnAboutDisabledWebSecurity, + warnAboutInsecureContentAllowed, + warnAboutExperimentalFeatures, + warnAboutBlinkFeatures, + warnAboutInsecureResources, + warnAboutInsecureCSP, + warnAboutAllowedPopups, + shouldLogSecurityWarnings +} = require('./security-warnings') + // Call webFrame method. electron.ipcRenderer.on('ELECTRON_INTERNAL_RENDERER_WEB_FRAME_METHOD', (event, method, args) => { electron.webFrame[method](...args) @@ -148,17 +160,6 @@ if (nodeIntegration === 'true') { } } - if (window.location.protocol === 'https:' || - window.location.protocol === 'http:' || - window.location.protocol === 'ftp:') { - let warning = 'This renderer process has Node.js integration enabled ' - warning += 'and attempted to load remote content. This exposes users of this app to severe ' - warning += 'security risks.\n' - warning += 'For more information and help, consult https://electronjs.org/docs/tutorial/security' - - console.warn('%cElectron Security Warning', 'font-weight: bold;', warning) - } - // Redirect window.onerror to uncaughtException. window.onerror = function (message, filename, lineno, colno, error) { if (global.process.listeners('uncaughtException').length > 0) { @@ -188,3 +189,22 @@ for (const preloadScript of preloadScripts) { console.error(error.stack || error.message) } } + +// Warn about security issues +window.addEventListener('load', function loadHandler () { + if (shouldLogSecurityWarnings()) { + if (nodeIntegration === 'true') { + warnAboutNodeWithRemoteContent() + } + + warnAboutDisabledWebSecurity() + warnAboutInsecureResources() + warnAboutInsecureContentAllowed() + warnAboutExperimentalFeatures() + warnAboutBlinkFeatures() + warnAboutInsecureCSP() + warnAboutAllowedPopups() + } + + window.removeEventListener('load', loadHandler) +}) diff --git a/lib/renderer/security-warnings.js b/lib/renderer/security-warnings.js new file mode 100644 index 000000000000..d2da884f1d9c --- /dev/null +++ b/lib/renderer/security-warnings.js @@ -0,0 +1,277 @@ +let shouldLog = null + +/** + * This method checks if a security message should be logged. + * It does so by determining whether we're running as Electron, + * which indicates that a developer is currently looking at the + * app. + * + * @returns {boolean} - Should we log? + */ +const shouldLogSecurityWarnings = function () { + if (shouldLog !== null) { + return shouldLog + } + + const { platform, execPath, env } = process + + switch (platform) { + case 'darwin': + shouldLog = execPath.endsWith('MacOS/Electron') || + execPath.includes('Electron.app/Contents/Frameworks/') + break + case 'freebsd': + case 'linux': + shouldLog = execPath.endsWith('/electron') + break + case 'win32': + shouldLog = execPath.endsWith('\\electron.exe') + break + default: + shouldLog = false + } + + if ((env && env.ELECTRON_DISABLE_SECURITY_WARNINGS) || + (window && window.ELECTRON_DISABLE_SECURITY_WARNINGS)) { + shouldLog = false + } + + if ((env && env.ELECTRON_ENABLE_SECURITY_WARNINGS) || + (window && window.ELECTRON_ENABLE_SECURITY_WARNINGS)) { + shouldLog = true + } + + return shouldLog +} + +/** + * Check's if the current window is remote. + * + * @returns {boolean} - Is this a remote protocol? + */ +const getIsRemoteProtocol = function () { + if (window && window.location && window.location.protocol) { + return /^(http|ftp)s?/gi.test(window.location.protocol) + } +} + +/** + * Tries to determine whether a CSP without `unsafe-eval` is set. + * + * @returns {boolean} Is a CSP with `unsafe-eval` set? + */ +const isUnsafeEvalEnabled = function () { + try { + //eslint-disable-next-line + new Function(''); + return true + } catch (error) { + return false + } +} + +/** + * Attempts to get the current webContents. Returns null + * if that fails + * + * @returns {Object|null} webPreferences + */ +let webPreferences +const getWebPreferences = function () { + try { + if (webPreferences) { + return webPreferences + } + + const { remote } = require('electron') + webPreferences = remote.getCurrentWindow().webContents.getWebPreferences() + return webPreferences + } catch (error) { + return null + } +} + +const moreInformation = '\nFor more information and help, consult ' + + 'https://electronjs.org/docs/tutorial/security.\n' + + 'This warning will not show up once the app is packaged.' + +module.exports = { + shouldLogSecurityWarnings, + + /** + * #1 Only load secure content + * + * Checks the loaded resources on the current page and logs a + * message about all resources loaded over HTTP or FTP. + */ + warnAboutInsecureResources: () => { + if (!window || !window.performance || !window.performance.getEntriesByType) { + return + } + + const resources = window.performance + .getEntriesByType('resource') + .filter(({ name }) => /^(http|ftp):?/gi.test(name || '')) + .map(({ name }) => `- ${name}`) + .join('\n') + + if (!resources || resources.length === 0) { + return + } + + let warning = 'This renderer process loads resources using insecure protocols. ' + + 'This exposes users of this app to unnecessary security risks. ' + + 'Consider loading the following resources over HTTPS or FTPS. \n' + + resources + '\n' + + moreInformation + + console.warn('%cElectron Security Warning (Insecure Resources)', + 'font-weight: bold;', warning) + }, + + /** + * #2 on the checklist: Disable the Node.js integration in all renderers that + * display remote content + * + * Logs a warning message about Node integration. + */ + warnAboutNodeWithRemoteContent: () => { + if (getIsRemoteProtocol()) { + let warning = 'This renderer process has Node.js integration enabled ' + + 'and attempted to load remote content. This exposes users of this app to severe ' + + 'security risks.\n' + + moreInformation + + console.warn('%cElectron Security Warning (Node.js Integration with Remote Content)', + 'font-weight: bold;', warning) + } + }, + + // Currently missing since it has ramifications and is still experimental: + // #3 Enable context isolation in all renderers that display remote content + // + // Currently missing since we can't easily programmatically check for those cases: + // #4 Use ses.setPermissionRequestHandler() in all sessions that load remote content + + /** + * #5 on the checklist: Do not disable websecurity + * + * Logs a warning message about disabled webSecurity. + */ + warnAboutDisabledWebSecurity: () => { + const webPreferences = getWebPreferences() + if (!webPreferences || webPreferences.webSecurity !== false) return + + let warning = 'This renderer process has "webSecurity" disabled. ' + + 'This exposes users of this app to severe security risks.\n' + + moreInformation + + console.warn('%cElectron Security Warning (Disabled webSecurity)', + 'font-weight: bold;', warning) + }, + + /** + * #6 on the checklist: Define a Content-Security-Policy and use restrictive + * rules (i.e. script-src 'self') + * + * #7 on the checklist: Disable eval + * + * Logs a warning message about unset or insecure CSP + */ + warnAboutInsecureCSP: () => { + if (isUnsafeEvalEnabled()) { + let warning = 'This renderer process has either no Content Security Policy set ' + + 'or a policy with "unsafe-eval" enabled. This exposes users of this ' + + 'app to unnecessary security risks.\n' + + moreInformation + + console.warn('%cElectron Security Warning (Insecure Content-Security-Policy)', + 'font-weight: bold;', warning) + } + }, + + /** + * #8 on the checklist: Do not set allowRunningInsecureContent to true + * + * Logs a warning message about disabled webSecurity. + */ + warnAboutInsecureContentAllowed: () => { + const webPreferences = getWebPreferences() + if (!webPreferences || !webPreferences.allowRunningInsecureContent) return + + let warning = 'This renderer process has "allowRunningInsecureContent" ' + + 'enabled. This exposes users of this app to severe security risks.\n' + + moreInformation + + console.warn('%cElectron Security Warning (allowRunningInsecureContent)', + 'font-weight: bold;', warning) + }, + + /** + * #9 on the checklist: Do not enable experimental features + * + * Logs a warning message about experimental features. + */ + warnAboutExperimentalFeatures: () => { + const webPreferences = getWebPreferences() + if (!webPreferences || (!webPreferences.experimentalFeatures && + !webPreferences.experimentalCanvasFeatures)) { + return + } + + let warning = 'This renderer process has "experimentalFeatures" ' + + 'enabled. This exposes users of this app to some security risk. ' + + 'If you do not need this feature, you should disable it.\n' + + moreInformation + + console.warn('%cElectron Security Warning (experimentalFeatures)', + 'font-weight: bold;', warning) + }, + + /** + * #10 on the checklist: Do not use blinkFeatures + * + * Logs a warning message about blinkFeatures + */ + warnAboutBlinkFeatures: () => { + const webPreferences = getWebPreferences() + if (!webPreferences || !webPreferences.blinkFeatures || + (webPreferences.blinkFeatures.length && webPreferences.blinkFeatures.length === 0)) { + return + } + + let warning = 'This renderer process has additional "blinkFeatures" ' + + 'enabled. This exposes users of this app to some security risk. ' + + 'If you do not need this feature, you should disable it.\n' + + moreInformation + + console.warn('%cElectron Security Warning (blinkFeatures)', + 'font-weight: bold;', warning) + }, + + /** + * #11 on the checklist: Do Not Use allowpopups + * + * Logs a warning message about allowed popups + */ + warnAboutAllowedPopups: () => { + if (document && document.querySelectorAll) { + const domElements = document.querySelectorAll('[allowpopups]') + + if (!domElements || domElements.length === 0) { + return + } + + let warning = 'A has "allowpopups" set to true. ' + + 'This exposes users of this app to some security risk, since popups are just ' + + 'BrowserWindows. If you do not need this feature, you should disable it.\n' + + moreInformation + + console.warn('%cElectron Security Warning (allowpopups)', + 'font-weight: bold;', warning) + } + } + + // Currently missing since we can't easily programmatically check for it: + // #12WebViews: Verify the options and params of all `` tags +} diff --git a/script/test.py b/script/test.py index 67292997348b..1f90ed795fcc 100755 --- a/script/test.py +++ b/script/test.py @@ -15,7 +15,7 @@ if sys.platform == 'linux2': # powerMonitor interaction with org.freedesktop.login1 service. The # dbus_mock module takes care of setting up the fake server with mock, # while also setting DBUS_SYSTEM_BUS_ADDRESS environment variable, which - # will be picked up by electron. + # will be picked up by electron. try: import lib.dbus_mock except ImportError: @@ -62,6 +62,7 @@ def main(): try: if args.use_instrumented_asar: install_instrumented_asar_file(resources_path) + os.environ["ELECTRON_DISABLE_SECURITY_WARNINGS"] = "1" subprocess.check_call([electron, 'spec'] + sys.argv[1:]) except subprocess.CalledProcessError as e: returncode = e.returncode diff --git a/spec/fixtures/pages/base-page-security.html b/spec/fixtures/pages/base-page-security.html new file mode 100644 index 000000000000..9b58913dbb50 --- /dev/null +++ b/spec/fixtures/pages/base-page-security.html @@ -0,0 +1,9 @@ + + + + + + + diff --git a/spec/fixtures/pages/insecure-resources.html b/spec/fixtures/pages/insecure-resources.html new file mode 100644 index 000000000000..97f91b769eae --- /dev/null +++ b/spec/fixtures/pages/insecure-resources.html @@ -0,0 +1,10 @@ + + + + + + + + diff --git a/spec/fixtures/pages/webview-allowpopups.html b/spec/fixtures/pages/webview-allowpopups.html new file mode 100644 index 000000000000..f392c0e22f0f --- /dev/null +++ b/spec/fixtures/pages/webview-allowpopups.html @@ -0,0 +1,10 @@ + + + + + + + + diff --git a/spec/security-warnings-spec.js b/spec/security-warnings-spec.js new file mode 100644 index 000000000000..feac23db7e06 --- /dev/null +++ b/spec/security-warnings-spec.js @@ -0,0 +1,185 @@ +const assert = require('assert') +const http = require('http') +const fs = require('fs') +const path = require('path') +const url = require('url') + +const {remote} = require('electron') +const {BrowserWindow} = remote + +const {closeWindow} = require('./window-helpers') + +describe('security warnings', () => { + let server + let w = null + let useCsp = true + + before(() => { + // Create HTTP Server + server = http.createServer((request, response) => { + const uri = url.parse(request.url).pathname + let filename = path.join(__dirname, './fixtures/pages', uri) + + fs.stat(filename, (error, stats) => { + if (error) { + response.writeHead(404, { 'Content-Type': 'text/plain' }) + response.end() + return + } + + if (stats.isDirectory()) { + filename += '/index.html' + } + + fs.readFile(filename, 'binary', (err, file) => { + if (err) { + response.writeHead(404, { 'Content-Type': 'text/plain' }) + response.end() + return + } + + const cspHeaders = { 'Content-Security-Policy': `script-src 'self' 'unsafe-inline'` } + response.writeHead(200, useCsp ? cspHeaders : undefined) + response.write(file, 'binary') + response.end() + }) + }) + }).listen(8881) + }) + + after(() => { + // Close server + server.close() + server = null + }) + + afterEach(() => { + closeWindow(w).then(() => { w = null }) + + useCsp = true + }) + + it('should warn about Node.js integration with remote content', (done) => { + w = new BrowserWindow({ show: false }) + w.webContents.on('console-message', (e, level, message) => { + assert(message.includes('Node.js Integration with Remote Content')) + done() + }) + + w.loadURL(`http://127.0.0.1:8881/base-page-security.html`) + }) + + it('should warn about disabled webSecurity', (done) => { + w = new BrowserWindow({ + show: false, + webPreferences: { + webSecurity: false, + nodeIntegration: false + } + }) + w.webContents.on('console-message', (e, level, message) => { + assert(message.includes('Disabled webSecurity')) + done() + }) + + w.loadURL(`http://127.0.0.1:8881/base-page-security.html`) + }) + + it('should warn about insecure Content-Security-Policy', (done) => { + w = new BrowserWindow({ + show: false, + webPreferences: { + nodeIntegration: false + } + }) + + w.webContents.on('console-message', (e, level, message) => { + assert(message.includes('Insecure Content-Security-Policy')) + done() + }) + + useCsp = false + w.loadURL(`http://127.0.0.1:8881/base-page-security.html`) + }) + + it('should warn about allowRunningInsecureContent', (done) => { + w = new BrowserWindow({ + show: false, + webPreferences: { + allowRunningInsecureContent: true, + nodeIntegration: false + } + }) + w.webContents.on('console-message', (e, level, message) => { + assert(message.includes('allowRunningInsecureContent')) + done() + }) + + w.loadURL(`http://127.0.0.1:8881/base-page-security.html`) + }) + + it('should warn about experimentalFeatures', (done) => { + w = new BrowserWindow({ + show: false, + webPreferences: { + experimentalFeatures: true, + nodeIntegration: false + } + }) + w.webContents.on('console-message', (e, level, message) => { + assert(message.includes('experimentalFeatures')) + done() + }) + + w.loadURL(`http://127.0.0.1:8881/base-page-security.html`) + }) + + it('should warn about blinkFeatures', (done) => { + w = new BrowserWindow({ + show: false, + webPreferences: { + blinkFeatures: ['my-cool-feature'], + nodeIntegration: false + } + }) + w.webContents.on('console-message', (e, level, message) => { + assert(message.includes('blinkFeatures')) + done() + }) + + w.loadURL(`http://127.0.0.1:8881/base-page-security.html`) + }) + + it('should warn about allowpopups', (done) => { + w = new BrowserWindow({ + show: false, + webPreferences: { + nodeIntegration: false + } + }) + w.webContents.on('console-message', (e, level, message) => { + console.log(message) + assert(message.includes('allowpopups')) + done() + }) + + w.loadURL(`http://127.0.0.1:8881/webview-allowpopups.html`) + }) + + it('should warn about insecure resources', (done) => { + w = new BrowserWindow({ + show: true, + webPreferences: { + nodeIntegration: false + } + }) + w.webContents.on('console-message', (e, level, message) => { + console.log(message) + assert(message.includes('Insecure Resources')) + done() + }) + + w.loadURL(`http://127.0.0.1:8881/insecure-resources.html`) + w.webContents.openDevTools() + }) +}) diff --git a/spec/static/main.js b/spec/static/main.js index 80c8e716c678..7050f3a7e430 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -29,6 +29,9 @@ app.commandLine.appendSwitch('js-flags', '--expose_gc') app.commandLine.appendSwitch('ignore-certificate-errors') app.commandLine.appendSwitch('disable-renderer-backgrounding') +// Disable security warnings (the security warnings test will enable them) +process.env.ELECTRON_DISABLE_SECURITY_WARNINGS = true + // Accessing stdout in the main process will result in the process.stdout // throwing UnknownSystemError in renderer process sometimes. This line makes // sure we can reproduce it in renderer process.