From f9be929eb9a2ca55bb51695dc9477032fc03fde5 Mon Sep 17 00:00:00 2001 From: ansuz Date: Fri, 21 Jan 2022 15:50:40 +0530 Subject: [PATCH 1/6] check for unnecessarily permissive CSP --- www/checkup/main.js | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/www/checkup/main.js b/www/checkup/main.js index d710747d7..f490014c1 100644 --- a/www/checkup/main.js +++ b/www/checkup/main.js @@ -859,6 +859,43 @@ define([ }); }); + assert(function (cb, msg) { + var directives = [ + 'img-src', + 'media-src', + 'child-src', + 'frame-src' + ]; + + msg.appendChild(h('span', [ + "This instance's ", + code("Content-Security-Policy"), + " headers are unnecessarily permissive.", + h('br'), + h('br'), + " Review the recommended settings for ", + code('img-src'), ', ', + code('media-src'), ', ', + code('child-src'), ', and ', + code('frame-src'), + " in the provided NGINX configuration file for an example of how to set these headers correctly.", + ])); + $.ajax(cacheBuster('/'), { + dataType: 'text', + complete: function (xhr) { + var CSP = parseCSP(xhr.getResponseHeader('content-security-policy')); + // check that the relevant CSP directives are defined + // and that none of them permit general remote content via '*' + if (directives.every(function (k) { + return typeof(CSP[k]) === 'string' && !/ \* /.test(CSP[k]); + })) { + return void cb(true); + } + cb(CSP); + }, + }); + }); + /* assert(function (cb, msg) { setWarningClass(msg); From 7e07f6e0d16011902ab7017f4adc4be9f0f3b788 Mon Sep 17 00:00:00 2001 From: ansuz Date: Fri, 21 Jan 2022 16:09:54 +0530 Subject: [PATCH 2/6] restructure bounce app --- www/bounce/main.js | 59 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 11 deletions(-) diff --git a/www/bounce/main.js b/www/bounce/main.js index ddb23d983..7764fb4c1 100644 --- a/www/bounce/main.js +++ b/www/bounce/main.js @@ -1,20 +1,57 @@ define(['/api/config'], function (ApiConfig) { + var reject = function () { + window.close(); + }; if (ApiConfig.httpSafeOrigin !== window.location.origin) { window.alert('The bounce application must only be used from the sandbox domain, ' + 'please report this issue on https://github.com/xwiki-labs/cryptpad'); return; } - var bounceTo = decodeURIComponent(window.location.hash.slice(1)); - if (!bounceTo) { - window.alert('The bounce application must only be used with a valid href to visit'); - return; - } - if (bounceTo.indexOf('javascript:') === 0 || // jshint ignore:line - bounceTo.indexOf('vbscript:') === 0 || // jshint ignore:line - bounceTo.indexOf('data:') === 0) { - window.alert('Illegal bounce URL'); - return; + if (typeof(URL) !== 'function') { + window.alert("Your browser does not support functionality this page requires"); + return void reject(); } + window.opener = null; - window.location.href = bounceTo; + + var host; + try { + host = new URL('', ApiConfig.httpUnsafeOrigin); + } catch (err) { + window.alert("This server is configured incorrectly. Its administrator should check its diagnostics page"); + return void reject(); + } + + var target; + try { + var bounceTo = decodeURIComponent(window.location.hash.slice(1)); + target = new URL(bounceTo, ApiConfig.httpUnsafeOrigin); + } catch (err) { + console.error(err); + window.alert('The bounce application must only be used with a valid href to visit'); + return void reject(); + } + + var go = function () { + window.location.href = target.href; + }; + + if (target.host === host.host) { return void go(); } + + require([ + '/customize/messages.js', + ], function (Messages) { + Messages.bounce_confirm = 'You are about to leave {0}\n\nAre you sure you want to visit "{1}"?'; // XXX + Messages.bounce_danger = 'It looks like someone is trying to trick you into visiting a dangerous link.\n\n("{0}")\n\nBe careful!'; // XXX + + if (['javascript:', 'vbscript:', 'data:', 'blob:'].includes(target.protocol)) { + window.alert(Messages._getKey('bounce_danger', [target.href])); + return void reject(); + } + + var question = Messages._getKey('bounce_confirm', [host.hostname, target.href]); + var answer = window.confirm(question); + if (answer) { return void go(); } + reject(); + }); }); From c438f0ba1004ac2d21ebebe93684c23fe4c7a40e Mon Sep 17 00:00:00 2001 From: ansuz Date: Fri, 21 Jan 2022 17:02:05 +0530 Subject: [PATCH 3/6] remove hardcoded translations --- www/bounce/main.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/www/bounce/main.js b/www/bounce/main.js index 7764fb4c1..d9669d075 100644 --- a/www/bounce/main.js +++ b/www/bounce/main.js @@ -41,9 +41,6 @@ define(['/api/config'], function (ApiConfig) { require([ '/customize/messages.js', ], function (Messages) { - Messages.bounce_confirm = 'You are about to leave {0}\n\nAre you sure you want to visit "{1}"?'; // XXX - Messages.bounce_danger = 'It looks like someone is trying to trick you into visiting a dangerous link.\n\n("{0}")\n\nBe careful!'; // XXX - if (['javascript:', 'vbscript:', 'data:', 'blob:'].includes(target.protocol)) { window.alert(Messages._getKey('bounce_danger', [target.href])); return void reject(); From 23e47032bf44584737b80f27144d10daf2590dcb Mon Sep 17 00:00:00 2001 From: ansuz Date: Fri, 21 Jan 2022 17:44:09 +0530 Subject: [PATCH 4/6] it's nice when important code is commented --- www/bounce/main.js | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/www/bounce/main.js b/www/bounce/main.js index d9669d075..3b955efc0 100644 --- a/www/bounce/main.js +++ b/www/bounce/main.js @@ -1,27 +1,46 @@ define(['/api/config'], function (ApiConfig) { +/* The 'bounce app' provides a unified way to do the following things in CryptPad + + 1. remove the 'opener' attribute from the tab/window every time you navigate + 2. detect and block malicious URLs after warning the user + 3. inform users when they are navigating away from their cryptpad instance + +*/ + + // when a URL is rejected we close the window var reject = function () { window.close(); }; + // this app is intended to be loaded and used exclusively from the sandbox domain + // where stricter CSP blocks various attacks. Reject any other usage. if (ApiConfig.httpSafeOrigin !== window.location.origin) { window.alert('The bounce application must only be used from the sandbox domain, ' + 'please report this issue on https://github.com/xwiki-labs/cryptpad'); - return; + return void reject(); } + // Old/bad browsers lack the URL API, making it more difficult to validate and compare URLs. + // Warn and reject. if (typeof(URL) !== 'function') { window.alert("Your browser does not support functionality this page requires"); return void reject(); } + // remove the 'opener' to prevent 'reverse tabnabbing'. window.opener = null; + // Parse the outer domain's root URL to facilitate comparisons. + // Reject everything if this fails to parse. var host; try { host = new URL('', ApiConfig.httpUnsafeOrigin); } catch (err) { - window.alert("This server is configured incorrectly. Its administrator should check its diagnostics page"); + window.alert("This server is configured incorrectly. Details for its administrator can be found on its diagnostics page."); return void reject(); } + // Decode the target URL that should have been provided through the document's hash. + // Reject if no URL was provided. + // Absolute URLs are easy to handle, other consider URLs relative to the outer domain. var target; try { var bounceTo = decodeURIComponent(window.location.hash.slice(1)); @@ -32,21 +51,29 @@ define(['/api/config'], function (ApiConfig) { return void reject(); } + // Valid links should navigate to the normalized href var go = function () { window.location.href = target.href; }; + // Local URLs don't require any warning and can navigate directly without user input. if (target.host === host.host) { return void go(); } + // Everything else requires user input, so we load the platform's translations. + // FIXME: this seems to infer language preferences from the browser instead of the user's account preferences require([ '/customize/messages.js', ], function (Messages) { + // The provided URL seems to be a malicious or invalid payload. + // Inform the user that we won't navigate and that the 'bounce tab' will be closed. if (['javascript:', 'vbscript:', 'data:', 'blob:'].includes(target.protocol)) { window.alert(Messages._getKey('bounce_danger', [target.href])); return void reject(); } + // The provided URL will navigate the user away from the outer domain. var question = Messages._getKey('bounce_confirm', [host.hostname, target.href]); + // Confirm that they want to leave, then navigate or reject based on their choice. var answer = window.confirm(question); if (answer) { return void go(); } reject(); From ae84d99af07e24caf672cd5f586b379dafaff195 Mon Sep 17 00:00:00 2001 From: ansuz Date: Fri, 21 Jan 2022 17:48:53 +0530 Subject: [PATCH 5/6] update the recommended settings for img-src and media-src --- docs/example.nginx.conf | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/example.nginx.conf b/docs/example.nginx.conf index ea21e3ba7..6a9d268e8 100644 --- a/docs/example.nginx.conf +++ b/docs/example.nginx.conf @@ -96,14 +96,14 @@ server { set $fontSrc "'self' data: ${main_domain}"; # images can be loaded from anywhere, though we'd like to deprecate this as it allows the use of images for tracking - set $imgSrc "'self' data: * blob: ${main_domain}"; + set $imgSrc "'self' data: blob: ${main_domain} ${sandbox_domain}"; # frame-src specifies valid sources for nested browsing contexts. # this prevents loading any iframes from anywhere other than the sandbox domain set $frameSrc "'self' ${sandbox_domain} blob:"; # specifies valid sources for loading media using video or audio - set $mediaSrc "'self' data: * blob: ${main_domain}"; + set $mediaSrc "'self' data: blob: ${main_domain} ${sandbox_domain}"; # defines valid sources for webworkers and nested browser contexts # deprecated in favour of worker-src and frame-src From 29fe4b7223d9de90b2397e986f63a5926426d1bc Mon Sep 17 00:00:00 2001 From: ansuz Date: Fri, 21 Jan 2022 18:01:55 +0530 Subject: [PATCH 6/6] restrict unnecessarily permissive CSP --- lib/defaults.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/defaults.js b/lib/defaults.js index c4cb507cb..f43253ccb 100644 --- a/lib/defaults.js +++ b/lib/defaults.js @@ -2,6 +2,7 @@ var Default = module.exports; Default.commonCSP = function (domain, sandbox) { domain = ' ' + domain; + sandbox = (sandbox && sandbox !== domain? ' ' + sandbox: ''); // Content-Security-Policy return [ @@ -15,19 +16,19 @@ Default.commonCSP = function (domain, sandbox) { * it is recommended that you configure these fields to match the * domain which will serve your CryptPad instance. */ - "child-src blob: *", + "child-src 'self' blob: " + domain + sandbox, // IE/Edge - "frame-src blob: *", + "frame-src 'self' blob: " + domain + sandbox, /* this allows connections over secure or insecure websockets if you are deploying to production, you'll probably want to remove the ws://* directive, and change '*' to your domain */ - "connect-src 'self' ws: wss: blob: " + domain + (sandbox && sandbox !== domain? ' ' + sandbox: ''), + "connect-src 'self' ws: wss: blob: " + domain + sandbox, // data: is used by codemirror "img-src 'self' data: blob:" + domain, - "media-src * blob:", + "media-src blob:", // for accounts.cryptpad.fr authentication and cross-domain iframe sandbox "frame-ancestors *",