From 01414e43ca22a8b0e7a2689f72552baf688dd310 Mon Sep 17 00:00:00 2001 From: rugk Date: Mon, 13 Jan 2020 19:17:30 +0100 Subject: [PATCH 01/17] Do not double-encode HTML in i18n This issue got introduced in 4bf7f86 due to double Fixes https://github.com/PrivateBin/PrivateBin/issues/557 Fixes https://github.com/PrivateBin/PrivateBin/issues/558 Also _inverted_ the logic/variable name for containsNoLinks to the more logical one "containsLinks" to avoid too many negations. Also verified that the attachment name is stil properly displayed when you clone a paste. --- js/privatebin.js | 12 +++--------- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index dc02e8fd..c83d8ffb 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -618,21 +618,15 @@ jQuery.PrivateBin = (function($, RawDeflate) { args[0] = translations[messageId]; } - // messageID may contain links, but should be from a trusted source (code or translation JSON files) - let containsNoLinks = args[0].indexOf(' 0) may never contain HTML as they may come from untrusted parties - if (i > 0 || containsNoLinks) { - args[i] = Helper.htmlEntities(args[i]); - } - } + // messageID may contain links, but only the first parameter, as that is from a trusted source (code or translation JSON files) + let containsLinks = args[0].indexOf(' - + diff --git a/tpl/page.php b/tpl/page.php index 865b0295..0f8df184 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From ebc2d649c412afc0d82b1c3ad6a7990189d11289 Mon Sep 17 00:00:00 2001 From: rugk Date: Mon, 13 Jan 2020 19:56:15 +0100 Subject: [PATCH 02/17] [TEST] Try to disallow vulnerable cases --- js/privatebin.js | 3 +++ test.diff | 40 ++++++++++++++++++++++++++++++++++++++++ tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 4 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 test.diff diff --git a/js/privatebin.js b/js/privatebin.js index c83d8ffb..5c0aeccb 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -620,6 +620,9 @@ jQuery.PrivateBin = (function($, RawDeflate) { // messageID may contain links, but only the first parameter, as that is from a trusted source (code or translation JSON files) let containsLinks = args[0].indexOf(' 0) { + throw new Error('security violation detected: do not concatenate links and untrusted data!'); + } // format string let output = Helper.sprintf.apply(this, args); diff --git a/test.diff b/test.diff new file mode 100644 index 00000000..97e384f2 --- /dev/null +++ b/test.diff @@ -0,0 +1,40 @@ +diff --git a/js/privatebin.js b/js/privatebin.js +index c83d8ff..5c0aecc 100644 +--- a/js/privatebin.js ++++ b/js/privatebin.js +@@ -620,6 +620,9 @@ jQuery.PrivateBin = (function($, RawDeflate) { + + // messageID may contain links, but only the first parameter, as that is from a trusted source (code or translation JSON files) + let containsLinks = args[0].indexOf(' 0) { ++ throw new Error('security violation detected: do not concatenate links and untrusted data!'); ++ } + + // format string + let output = Helper.sprintf.apply(this, args); +diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php +index 59c730e..0e949da 100644 +--- a/tpl/bootstrap.php ++++ b/tpl/bootstrap.php +@@ -72,7 +72,7 @@ endif; + ?> + + +- ++ + + + +diff --git a/tpl/page.php b/tpl/page.php +index 0f8df18..ff2d5f5 100644 +--- a/tpl/page.php ++++ b/tpl/page.php +@@ -50,7 +50,7 @@ endif; + ?> + + +- ++ + + + diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 59c730e2..0e949da6 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 0f8df184..ff2d5f50 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From eb549d70d1477411f28f00720a035a81a1ca9f48 Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 15 Jan 2020 17:52:51 +0100 Subject: [PATCH 03/17] Invert conatainsLink logic --- js/privatebin.js | 14 +++++--------- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index 5c0aeccb..7fab6076 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -453,11 +453,7 @@ jQuery.PrivateBin = (function($, RawDeflate) { * @return string escaped HTML */ me.htmlEntities = function(str) { - // using textarea, since other tags may allow and execute scripts, even when detached from DOM - let holder = document.createElement('textarea'); - holder.textContent = str; - // as per OWASP recommendation, also encoding quotes and slash - return holder.innerHTML.replace( + return str.replace( /["'\/]/g, function(s) { return { @@ -629,10 +625,7 @@ jQuery.PrivateBin = (function($, RawDeflate) { // if $element is given, apply text to element if ($element !== null) { - if (!containsLinks) { - // avoid HTML entity encoding if translation contains links - $element.text(output); - } else { + if (containsLinks) { // only allow tags/attributes we actually use in our translations $element.html( DOMPurify.sanitize(output, { @@ -640,6 +633,9 @@ jQuery.PrivateBin = (function($, RawDeflate) { ALLOWED_ATTR: ['href', 'id'] }) ); + } else { + // avoid HTML entity encoding if translation contains no links + $element.text(output); } } diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 0e949da6..03636d98 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index ff2d5f50..760d991c 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From fd4492f2299878d231f5bfeb5414153b62fb25ba Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 07:09:56 +0100 Subject: [PATCH 04/17] ensuring that both critical branches get tested --- js/test/I18n.js | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/js/test/I18n.js b/js/test/I18n.js index 9bfc76ac..ba10e63f 100644 --- a/js/test/I18n.js +++ b/js/test/I18n.js @@ -39,13 +39,13 @@ describe('I18n', function () { } ); jsc.property( - 'replaces %s in strings with first given parameter', + 'replaces %s in strings with first given parameter, encoding all, when no link is in the messageID', 'string', '(small nearray) string', 'string', function (prefix, params, postfix) { prefix = prefix.replace(/%(s|d)/g, '%%'); - params[0] = params[0].replace(/%(s|d)/g, '%%'); + params[0] = params[0].replace(/%(s|d)/g, '%%').replace(/%'; + postfix = postfix.replace(/%(s|d)/g, '%%'); + var translation = $.PrivateBin.Helper.htmlEntities(prefix + params[0] + postfix); + params.unshift(prefix + '%s' + postfix); + var result = $.PrivateBin.I18n.translate.apply(this, params); + $.PrivateBin.I18n.reset(); + var alias = $.PrivateBin.I18n._.apply(this, params); + $.PrivateBin.I18n.reset(); + return translation === result && translation === alias; + } + ); }); describe('getPluralForm', function () { From 76eff6a87afcfbcd184bc875b89d368ab4a2bd38 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 07:12:03 +0100 Subject: [PATCH 05/17] Revert "[TEST] Try to disallow vulnerable cases" to remove accidentally committed file and statement that breaks the tests This reverts commit ebc2d649c412afc0d82b1c3ad6a7990189d11289. --- js/privatebin.js | 3 --- test.diff | 40 ---------------------------------------- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 4 files changed, 2 insertions(+), 45 deletions(-) delete mode 100644 test.diff diff --git a/js/privatebin.js b/js/privatebin.js index 7fab6076..975b2123 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -616,9 +616,6 @@ jQuery.PrivateBin = (function($, RawDeflate) { // messageID may contain links, but only the first parameter, as that is from a trusted source (code or translation JSON files) let containsLinks = args[0].indexOf(' 0) { - throw new Error('security violation detected: do not concatenate links and untrusted data!'); - } // format string let output = Helper.sprintf.apply(this, args); diff --git a/test.diff b/test.diff deleted file mode 100644 index 97e384f2..00000000 --- a/test.diff +++ /dev/null @@ -1,40 +0,0 @@ -diff --git a/js/privatebin.js b/js/privatebin.js -index c83d8ff..5c0aecc 100644 ---- a/js/privatebin.js -+++ b/js/privatebin.js -@@ -620,6 +620,9 @@ jQuery.PrivateBin = (function($, RawDeflate) { - - // messageID may contain links, but only the first parameter, as that is from a trusted source (code or translation JSON files) - let containsLinks = args[0].indexOf(' 0) { -+ throw new Error('security violation detected: do not concatenate links and untrusted data!'); -+ } - - // format string - let output = Helper.sprintf.apply(this, args); -diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php -index 59c730e..0e949da 100644 ---- a/tpl/bootstrap.php -+++ b/tpl/bootstrap.php -@@ -72,7 +72,7 @@ endif; - ?> - - -- -+ - - - -diff --git a/tpl/page.php b/tpl/page.php -index 0f8df18..ff2d5f5 100644 ---- a/tpl/page.php -+++ b/tpl/page.php -@@ -50,7 +50,7 @@ endif; - ?> - - -- -+ - - - diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 03636d98..f7c19536 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 760d991c..55f5f785 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From cec5cb41d7093a2937098029c7228c781bc26ebe Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 07:20:05 +0100 Subject: [PATCH 06/17] Partial revert "Do not double-encode HTML in i18n", only revert the removal of required encoding logic - still has to be moved This reverts commit 01414e43ca22a8b0e7a2689f72552baf688dd310. --- js/privatebin.js | 8 +++++++- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index 975b2123..0adea3bb 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -614,8 +614,14 @@ jQuery.PrivateBin = (function($, RawDeflate) { args[0] = translations[messageId]; } - // messageID may contain links, but only the first parameter, as that is from a trusted source (code or translation JSON files) + // messageID may contain links, but should be from a trusted source (code or translation JSON files) let containsLinks = args[0].indexOf(' 0) may never contain HTML as they may come from untrusted parties + if (i > 0 || containsNoLinks) { + args[i] = Helper.htmlEntities(args[i]); + } + } // format string let output = Helper.sprintf.apply(this, args); diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index f7c19536..08f28c0c 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 55f5f785..99c78d8f 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From 0d08edbe559e677bc5d47c7a67106ac1bc4564a3 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 07:30:01 +0100 Subject: [PATCH 07/17] Revert "getting rid of htmlEntities (except for tests)" a0740ff79f9076ec7fa4d80bdfb32337a7136482 --- js/privatebin.js | 36 ++++++++++++++++++++++++++++++++++++ tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index 0adea3bb..3ce7fe91 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -189,6 +189,26 @@ jQuery.PrivateBin = (function($, RawDeflate) { const Helper = (function () { const me = {}; + /** + * character to HTML entity lookup table + * + * @see {@link https://github.com/janl/mustache.js/blob/master/mustache.js#L60} + * @name Helper.entityMap + * @private + * @enum {Object} + * @readonly + */ + var entityMap = { + '&': '&', + '<': '<', + '>': '>', + '"': '"', + "'": ''', + '/': '/', + '`': '`', + '=': '=' + }; + /** * cache for script location * @@ -392,6 +412,22 @@ jQuery.PrivateBin = (function($, RawDeflate) { return new Comment(data); }; + /** + * convert all applicable characters to HTML entities + * + * @see {@link https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#RULE_.231_-_HTML_Escape_Before_Inserting_Untrusted_Data_into_HTML_Element_Content} + * @name Helper.htmlEntities + * @function + * @param {string} str + * @return {string} escaped HTML + */ + me.htmlEntities = function(str) { + return String(str).replace( + /[&<>"'`=\/]/g, function(s) { + return entityMap[s]; + }); + } + /** * resets state, used for unit testing * diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 08f28c0c..92becfcf 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 99c78d8f..1a799c59 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From 7b87dc3ca934db835e1fb73074804693e6cb4b60 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 07:36:43 +0100 Subject: [PATCH 08/17] cleanup revert --- js/privatebin.js | 27 +++------------------------ tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 3 files changed, 5 insertions(+), 26 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index 3ce7fe91..94434384 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -415,7 +415,7 @@ jQuery.PrivateBin = (function($, RawDeflate) { /** * convert all applicable characters to HTML entities * - * @see {@link https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#RULE_.231_-_HTML_Escape_Before_Inserting_Untrusted_Data_into_HTML_Element_Content} + * @see {@link https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html} * @name Helper.htmlEntities * @function * @param {string} str @@ -425,7 +425,8 @@ jQuery.PrivateBin = (function($, RawDeflate) { return String(str).replace( /[&<>"'`=\/]/g, function(s) { return entityMap[s]; - }); + } + ); } /** @@ -478,28 +479,6 @@ jQuery.PrivateBin = (function($, RawDeflate) { return expirationDate; }; - /** - * encode all applicable characters to HTML entities - * - * @see {@link https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html} - * - * @name Helper.htmlEntities - * @function - * @param string str - * @return string escaped HTML - */ - me.htmlEntities = function(str) { - return str.replace( - /["'\/]/g, - function(s) { - return { - '"': '"', - "'": ''', - '/': '/' - }[s]; - }); - }; - return me; })(); diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 92becfcf..26c3d8e2 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 1a799c59..0d0f4078 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From fa9d3037bac6ae1baaaae65c0eef8562047c606e Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 07:44:32 +0100 Subject: [PATCH 09/17] fixing logic & indentation --- js/privatebin.js | 2 +- js/test/I18n.js | 34 +++++++++++++++++----------------- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index 94434384..9db8b006 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -633,7 +633,7 @@ jQuery.PrivateBin = (function($, RawDeflate) { let containsLinks = args[0].indexOf(' 0) may never contain HTML as they may come from untrusted parties - if (i > 0 || containsNoLinks) { + if (i > 0 || !containsLinks) { args[i] = Helper.htmlEntities(args[i]); } } diff --git a/js/test/I18n.js b/js/test/I18n.js index ba10e63f..061f05b4 100644 --- a/js/test/I18n.js +++ b/js/test/I18n.js @@ -57,23 +57,23 @@ describe('I18n', function () { } ); jsc.property( - 'replaces %s in strings with first given parameter, encoding params only, when a link is part of the messageID', - 'string', - '(small nearray) string', - 'string', - function (prefix, params, postfix) { - prefix = prefix.replace(/%(s|d)/g, '%%'); - params[0] = params[0].replace(/%(s|d)/g, '%%') + ''; - postfix = postfix.replace(/%(s|d)/g, '%%'); - var translation = $.PrivateBin.Helper.htmlEntities(prefix + params[0] + postfix); - params.unshift(prefix + '%s' + postfix); - var result = $.PrivateBin.I18n.translate.apply(this, params); - $.PrivateBin.I18n.reset(); - var alias = $.PrivateBin.I18n._.apply(this, params); - $.PrivateBin.I18n.reset(); - return translation === result && translation === alias; - } - ); + 'replaces %s in strings with first given parameter, encoding params only, when a link is part of the messageID', + 'string', + '(small nearray) string', + 'string', + function (prefix, params, postfix) { + prefix = prefix.replace(/%(s|d)/g, '%%'); + params[0] = params[0].replace(/%(s|d)/g, '%%') + ''; + postfix = postfix.replace(/%(s|d)/g, '%%'); + var translation = $.PrivateBin.Helper.htmlEntities(prefix) + params[0] + $.PrivateBin.Helper.htmlEntities(postfix); + params.unshift(prefix + '%s' + postfix); + var result = $.PrivateBin.I18n.translate.apply(this, params); + $.PrivateBin.I18n.reset(); + var alias = $.PrivateBin.I18n._.apply(this, params); + $.PrivateBin.I18n.reset(); + return translation === result && translation === alias; + } + ); }); describe('getPluralForm', function () { diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 26c3d8e2..90607b87 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 0d0f4078..1cae5e5c 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From 685c354d0eaa48d2871ed723d0749244e60cb4a7 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 10:44:35 +0100 Subject: [PATCH 10/17] several changes: - added tests for all 4 cases: output to string or into element vs first param contains link or not - cleaned up logic - skip HTML entity encoding only if we can ensure insertion to text node / when output to string, we always encode - DOMpurify sanitizes gopher, ws & wss links, which we previosly had tested for --- js/common.js | 2 +- js/privatebin.js | 44 ++++++++++--------- js/test/AttachmentViewer.js | 2 +- js/test/I18n.js | 84 +++++++++++++++++++++++++++++++++---- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 6 files changed, 105 insertions(+), 31 deletions(-) diff --git a/js/common.js b/js/common.js index a13a6daa..b153dfc6 100644 --- a/js/common.js +++ b/js/common.js @@ -36,7 +36,7 @@ var a2zString = ['a','b','c','d','e','f','g','h','i','j','k','l','m', return c.toUpperCase(); }) ), - schemas = ['ftp','gopher','http','https','ws','wss'], + schemas = ['ftp','http','https'], supportedLanguages = ['de', 'es', 'fr', 'it', 'no', 'pl', 'pt', 'oc', 'ru', 'sl', 'zh'], mimeTypes = ['image/png', 'application/octet-stream'], formats = ['plaintext', 'markdown', 'syntaxhighlighting'], diff --git a/js/privatebin.js b/js/privatebin.js index 9db8b006..fda2d563 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -631,28 +631,35 @@ jQuery.PrivateBin = (function($, RawDeflate) { // messageID may contain links, but should be from a trusted source (code or translation JSON files) let containsLinks = args[0].indexOf(' 0) may never contain HTML as they may come from untrusted parties - if (i > 0 || !containsLinks) { - args[i] = Helper.htmlEntities(args[i]); + + // prevent double encoding, when we insert into a text node + if (!containsLinks || $element === null) { + for (let i = 0; i < args.length; ++i) { + // parameters (i > 0) may never contain HTML as they may come from untrusted parties + if (i > 0 || !containsLinks) { + args[i] = Helper.htmlEntities(args[i]); + } } } - // format string let output = Helper.sprintf.apply(this, args); - // if $element is given, apply text to element + if (containsLinks) { + // only allow tags/attributes we actually use in translations + output = DOMPurify.sanitize( + output, { + ALLOWED_TAGS: ['a', 'br', 'i', 'span'], + ALLOWED_ATTR: ['href', 'id'] + } + ); + } + + // if $element is given, insert translation if ($element !== null) { if (containsLinks) { - // only allow tags/attributes we actually use in our translations - $element.html( - DOMPurify.sanitize(output, { - ALLOWED_TAGS: ['a', 'br', 'i', 'span'], - ALLOWED_ATTR: ['href', 'id'] - }) - ); + $element.html(output); } else { - // avoid HTML entity encoding if translation contains no links + // text node takes care of entity encoding $element.text(output); } } @@ -1946,11 +1953,10 @@ jQuery.PrivateBin = (function($, RawDeflate) { */ me.createPasteNotification = function(url, deleteUrl) { - $('#pastelink').html( - I18n._( - 'Your paste is %s (Hit [Ctrl]+[c] to copy)', - url, url - ) + I18n._( + $('#pastelink'), + 'Your paste is %s (Hit [Ctrl]+[c] to copy)', + url, url ); // save newly created element $pasteUrl = $('#pasteurl'); diff --git a/js/test/AttachmentViewer.js b/js/test/AttachmentViewer.js index f3b3bb57..842945fc 100644 --- a/js/test/AttachmentViewer.js +++ b/js/test/AttachmentViewer.js @@ -88,7 +88,7 @@ describe('AttachmentViewer', function () { if (prefix.indexOf('').html(prefix + $.PrivateBin.Helper.htmlEntities(filename) + postfix).html(); + result = prefix + $.PrivateBin.Helper.htmlEntities(filename) + postfix; } if (filename.length) { results.push( diff --git a/js/test/I18n.js b/js/test/I18n.js index 061f05b4..0086cc27 100644 --- a/js/test/I18n.js +++ b/js/test/I18n.js @@ -3,6 +3,7 @@ var common = require('../common'); describe('I18n', function () { describe('translate', function () { + this.timeout(30000); before(function () { $.PrivateBin.I18n.reset(); }); @@ -45,13 +46,13 @@ describe('I18n', function () { 'string', function (prefix, params, postfix) { prefix = prefix.replace(/%(s|d)/g, '%%'); - params[0] = params[0].replace(/%(s|d)/g, '%%').replace(/%'; + params[0] = params[0].replace(/%(s|d)/g, '%%'); postfix = postfix.replace(/%(s|d)/g, '%%'); - var translation = $.PrivateBin.Helper.htmlEntities(prefix) + params[0] + $.PrivateBin.Helper.htmlEntities(postfix); + const translation = DOMPurify.sanitize( + prefix + $.PrivateBin.Helper.htmlEntities(params[0]) + '' + postfix, { + ALLOWED_TAGS: ['a', 'br', 'i', 'span'], + ALLOWED_ATTR: ['href', 'id'] + } + ); + params.unshift(prefix + '%s' + postfix); + const result = $.PrivateBin.I18n.translate.apply(this, params); + $.PrivateBin.I18n.reset(); + const alias = $.PrivateBin.I18n._.apply(this, params); + $.PrivateBin.I18n.reset(); + return translation === result && translation === alias; + } + ); + jsc.property( + 'replaces %s in strings with first given parameter into an element, encoding all, when no link is in the messageID', + 'string', + '(small nearray) string', + 'string', + function (prefix, params, postfix) { + prefix = prefix.replace(/%(s|d)/g, '%%'); + params[0] = params[0].replace(/%(s|d)/g, '%%').replace(/'); + params.unshift($('#i18n')); + $.PrivateBin.I18n.translate.apply(this, params); + const result = $('#i18n').text(); $.PrivateBin.I18n.reset(); - var alias = $.PrivateBin.I18n._.apply(this, params); + clean(); + clean = jsdom(); + $('body').html('
'); + params[0] = $('#i18n'); + $.PrivateBin.I18n._.apply(this, params); + const alias = $('#i18n').text(); $.PrivateBin.I18n.reset(); + clean(); + return translation === result && translation === alias; + } + ); + jsc.property( + 'replaces %s in strings with first given parameter into an element, encoding params only, when a link is part of the messageID inserted', + 'string', + '(small nearray) string', + 'string', + function (prefix, params, postfix) { + prefix = prefix.replace(/%(s|d)/g, '%%'); + params[0] = params[0].replace(/%(s|d)/g, '%%'); + postfix = postfix.replace(/%(s|d)/g, '%%'); + const translation = $('
').html(DOMPurify.sanitize( + prefix + $.PrivateBin.Helper.htmlEntities(params[0]) + '' + postfix, { + ALLOWED_TAGS: ['a', 'br', 'i', 'span'], + ALLOWED_ATTR: ['href', 'id'] + } + )).html(); + let args = Array.prototype.slice.call(params); + args.unshift(prefix + '%s' + postfix); + let clean = jsdom(); + $('body').html('
'); + args.unshift($('#i18n')); + $.PrivateBin.I18n.translate.apply(this, args); + const result = $('#i18n').html(); + $.PrivateBin.I18n.reset(); + clean(); + clean = jsdom(); + $('body').html('
'); + args[0] = $('#i18n'); + $.PrivateBin.I18n._.apply(this, args); + const alias = $('#i18n').html(); + $.PrivateBin.I18n.reset(); + clean(); return translation === result && translation === alias; } ); diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 90607b87..04f05941 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 1cae5e5c..c83374ba 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From 42130e0468f85ab0990b4f7cf27150130c26c016 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 10:53:58 +0100 Subject: [PATCH 11/17] prevent potentially non-encoded string from getting returned --- js/privatebin.js | 1 + tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index fda2d563..1444007e 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -662,6 +662,7 @@ jQuery.PrivateBin = (function($, RawDeflate) { // text node takes care of entity encoding $element.text(output); } + return ''; } return output; diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 04f05941..8f6241e8 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index c83374ba..7a7ddb2b 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From aa3f1206b20ddc6e76e7aeec3142d35002c4ce77 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 25 Jan 2020 08:13:36 +0100 Subject: [PATCH 12/17] rewriting translations to pass jQuery element where easily possible --- js/privatebin.js | 21 ++++++++++----------- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index 1444007e..39537cd9 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -1895,11 +1895,10 @@ jQuery.PrivateBin = (function($, RawDeflate) { return a.length - b.length; })[0]; if (typeof shortUrl === 'string' && shortUrl.length > 0) { - $('#pastelink').html( - I18n._( - 'Your paste is %s (Hit [Ctrl]+[c] to copy)', - shortUrl, shortUrl - ) + I18n._( + $('#pastelink'), + 'Your paste is %s (Hit [Ctrl]+[c] to copy)', + shortUrl, shortUrl ); // we disable the button to avoid calling shortener again $shortenButton.addClass('buttondisabled'); @@ -1965,7 +1964,8 @@ jQuery.PrivateBin = (function($, RawDeflate) { $pasteUrl.click(pasteLinkClick); // delete link - $('#deletelink').html('' + I18n._('Delete data') + ''); + $('#deletelink').html(''); + I18n._($('#deletelink a').first(), 'Delete data'); // enable shortener button $shortenButton.removeClass('buttondisabled'); @@ -3722,8 +3722,9 @@ jQuery.PrivateBin = (function($, RawDeflate) { const $emailconfirmmodal = $('#emailconfirmmodal'); if ($emailconfirmmodal.length > 0) { if (expirationDate !== null) { - $emailconfirmmodal.find('#emailconfirm-display').text( - I18n._('Recipient may become aware of your timezone, convert time to UTC?') + I18n._( + $emailconfirmmodal.find('#emailconfirm-display'), + 'Recipient may become aware of your timezone, convert time to UTC?' ); const $emailconfirmTimezoneCurrent = $emailconfirmmodal.find('#emailconfirm-timezone-current'); const $emailconfirmTimezoneUtc = $emailconfirmmodal.find('#emailconfirm-timezone-utc'); @@ -3923,9 +3924,7 @@ jQuery.PrivateBin = (function($, RawDeflate) { }); } catch (error) { console.error(error); - Alert.showError( - I18n._('Cannot calculate expiration date.') - ); + Alert.showError('Cannot calculate expiration date.'); } } diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 8f6241e8..50b0b1c0 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 7a7ddb2b..0bae6f88 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - + From 62365880b40a1bc3c2b65fe1fb369733a112d825 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 25 Jan 2020 09:07:06 +0100 Subject: [PATCH 13/17] implement simplified translation logic, forcing the use of safe application via jQuery element --- js/privatebin.js | 80 +++++++++++++++---------- js/test/I18n.js | 149 ++++++++++++++++++++++------------------------ tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 4 files changed, 122 insertions(+), 111 deletions(-) diff --git a/js/privatebin.js b/js/privatebin.js index 39537cd9..82cc4e16 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -322,19 +322,12 @@ jQuery.PrivateBin = (function($, RawDeflate) { let format = args[0], i = 1; return format.replace(/%(s|d)/g, function (m) { - // m is the matched format, e.g. %s, %d let val = args[i]; - // A switch statement so that the formatter can be extended. - switch (m) - { - case '%d': - val = parseFloat(val); - if (isNaN(val)) { - val = 0; - } - break; - default: - // Default is %s + if (m === '%d') { + val = parseFloat(val); + if (isNaN(val)) { + val = 0; + } } ++i; return val; @@ -547,19 +540,23 @@ jQuery.PrivateBin = (function($, RawDeflate) { /** * translate a string * - * Optionally pass a jQuery element as the first parameter, to automatically - * let the text of this element be replaced. In case the (asynchronously + * As the first parameter a jQuery element has to be provided, to let + * the text of this element be replaced. In case the (asynchronously * loaded) language is not downloadet yet, this will make sure the string - * is replaced when it is actually loaded. - * So for easy translations passing the jQuery object to apply it to is - * more save, especially when they are loaded in the beginning. + * is replaced when it is actually loaded. This also handles HTML in + * secure fashion, to avoid XSS. + * The second parameter is the message ID, matching the ones found in + * the translation files under the i18n directory. + * Any additional parameters will get inserted into the message ID in + * place of %s (strings) or %d (digits), applying the appropriate plural + * in case of digits. See also Helper.sprintf(). * * @name I18n.translate * @function - * @param {jQuery} $element - optional + * @param {jQuery} $element * @param {string} messageId * @param {...*} args - one or multiple parameters injected into placeholders - * @return {string} + * @throws {string} */ me.translate = function() { @@ -573,6 +570,8 @@ jQuery.PrivateBin = (function($, RawDeflate) { // optional jQuery element as first parameter $element = args[0]; args.shift(); + } else { + throw 'translation requires a jQuery element to be passed, for secure insertion of messages and to avoid double encoding of HTML entities'; } // extract messageId from arguments @@ -633,10 +632,10 @@ jQuery.PrivateBin = (function($, RawDeflate) { let containsLinks = args[0].indexOf(' 0) may never contain HTML as they may come from untrusted parties - if (i > 0 || !containsLinks) { + if (i > 0) { args[i] = Helper.htmlEntities(args[i]); } } @@ -654,18 +653,37 @@ jQuery.PrivateBin = (function($, RawDeflate) { ); } - // if $element is given, insert translation - if ($element !== null) { - if (containsLinks) { - $element.html(output); - } else { - // text node takes care of entity encoding - $element.text(output); - } - return ''; + if (containsLinks) { + $element.html(output); + } else { + // text node takes care of entity encoding + $element.text(output); } + }; - return output; + /** + * translate a string, outputs the result + * + * This function is identical to I18n.translate, but doesn't require a + * jQuery element as the first parameter, instead it returns the + * translated message as string. + * Avoid using this function, if possible, as it may double encode your + * message's HTML entities. This is done to fail safe, preventing XSS. + * + * @name I18n.translate2string + * @function + * @param {string} messageId + * @param {...*} args - one or multiple parameters injected into placeholders + * @throws {string} + * @return {string} + */ + me.translate2string = function() + { + let args = Array.prototype.slice.call(arguments), + $element = $('