From 34f64c61f6a989608b461b12c3bec7d663d6a8f7 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Tue, 12 Oct 2021 16:56:15 +0200 Subject: [PATCH 1/3] privUITransaction fix inspections errors + formatting --- application/transaction.class.inc.php | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/application/transaction.class.inc.php b/application/transaction.class.inc.php index 0e4ca4be1..f818eba67 100644 --- a/application/transaction.class.inc.php +++ b/application/transaction.class.inc.php @@ -26,8 +26,6 @@ * @copyright Copyright (C) 2010-2012 Combodo SARL * @license http://opensource.org/licenses/AGPL-3.0 */ - - class privUITransaction { /** @@ -101,7 +99,6 @@ class privUITransaction /** * The original (and by default) mechanism for storing transaction information * as an array in the $_SESSION variable - * */ class privUITransactionSession { @@ -207,6 +204,7 @@ class privUITransactionFile { throw new Exception('The directory "'.APPROOT.'data" must be writable to the application.'); } + /** @noinspection MkdirRaceConditionInspection */ if (!@mkdir(APPROOT.'data/transactions')) { throw new Exception('Failed to create the directory "'.APPROOT.'data/transactions". Ajust the rights on the parent directory or let an administrator create the transactions directory and give the web sever enough rights to write into it.'); @@ -268,27 +266,26 @@ class privUITransactionFile /** * Removes the transaction specified by its id * @param int $id The Identifier (as returned by GetNewTransactionId) of the transaction to be removed. - * @return void + * @return bool true if the token can be removed */ public static function RemoveTransaction($id) { - $bSuccess = true; $sFilepath = APPROOT.'data/transactions/'.$id; clearstatcache(true, $sFilepath); - if(!file_exists($sFilepath)) - { + if (!file_exists($sFilepath)) { $bSuccess = false; - self::Error("RemoveTransaction: Transaction '$id' not found. Pending transactions for this user:\n".implode("\n", self::GetPendingTransactions())); + self::Error("RemoveTransaction: Transaction '$id' not found. Pending transactions for this user:\n" + .implode("\n", self::GetPendingTransactions())); + } else { + $bSuccess = @unlink($sFilepath); } - $bSuccess = @unlink($sFilepath); - if (!$bSuccess) - { + + if (!$bSuccess) { self::Error('RemoveTransaction: FAILED to remove transaction '.$id); - } - else - { + } else { self::Info('RemoveTransaction: OK '.$id); } + return $bSuccess; } From eaf8a187aa8482ff794c7799633d776d35f6ca47 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Mon, 18 Oct 2021 11:36:17 +0200 Subject: [PATCH 2/3] =?UTF-8?q?N=C2=B03332=20report=20function=20rename=20?= =?UTF-8?q?The=20method=20was=20renamed=20in=2018d52319=20but=20only=20on?= =?UTF-8?q?=20support/2.7=20and=20above?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- js/breadcrumb.js | 4 ++-- js/utils.js | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/js/breadcrumb.js b/js/breadcrumb.js index 09b0a6d81..de0afba07 100644 --- a/js/breadcrumb.js +++ b/js/breadcrumb.js @@ -69,8 +69,8 @@ $(function() if (sTitle.length == 0) { sTitle = oEntry['label']; } - sTitle = SanitizeHtml(sTitle, false); - sLabel = SanitizeHtml(sLabel, false); + sTitle = EncodeHtml(sTitle, false); + sLabel = EncodeHtml(sLabel, false); if ((this.options.new_entry !== null) && (iEntry == aBreadCrumb.length-1)) { // Last entry is the current page diff --git a/js/utils.js b/js/utils.js index 72f3adb13..59af2b480 100644 --- a/js/utils.js +++ b/js/utils.js @@ -671,16 +671,16 @@ function DisplayHistory(sSelector, sFilter, iCount, iStart) { /** * @param sValue value to escape - * @param bReplaceAmp if false don't replace "&" (can be useful when dealing with html entities) - * @returns {string} sanitized value, ready to insert in the DOM without XSS risk + * @param bReplaceAmp if false don't replace "&" (can be useful when sValue contrains html entities we want to keep) + * @returns {string} escaped value, ready to insert in the DOM without XSS risk * * @since 2.6.5, 2.7.2, 3.0.0 N°3332 * @see https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#rule-1-html-encode-before-inserting-untrusted-data-into-html-element-content * @see https://stackoverflow.com/questions/295566/sanitize-rewrite-html-on-the-client-side/430240#430240 why inserting in the DOM (for * example the text() JQuery way) isn't safe */ -function SanitizeHtml(sValue, bReplaceAmp) { - var sSanitizedValue = (sValue+'') +function EncodeHtml(sValue, bReplaceAmp) { + var sEncodedValue = (sValue+'') .replace(//g, '>') .replace(/"/g, '"') @@ -688,10 +688,10 @@ function SanitizeHtml(sValue, bReplaceAmp) { .replace(/\//g, '/'); if (bReplaceAmp) { - sSanitizedValue = sSanitizedValue.replace(/&/g, '&'); + sEncodedValue = sEncodedValue.replace(/&/g, '&'); } - return sSanitizedValue; + return sEncodedValue; } // Very simple equivalent to format: placeholders are %1$s %2$d ... From b3f827ed5ee8b5d78998cbc72d9560e227df6848 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Mon, 18 Oct 2021 11:38:00 +0200 Subject: [PATCH 3/3] =?UTF-8?q?N=C2=B04367=20Security=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- application/ui.extkeywidget.class.inc.php | 4 +- js/utils.js | 66 ++++++++++++++ test/VisualTest/sanitize_test.php | 105 ++++++++++++++++++++++ 3 files changed, 173 insertions(+), 2 deletions(-) create mode 100644 test/VisualTest/sanitize_test.php diff --git a/application/ui.extkeywidget.class.inc.php b/application/ui.extkeywidget.class.inc.php index b371d6204..bf918bf72 100644 --- a/application/ui.extkeywidget.class.inc.php +++ b/application/ui.extkeywidget.class.inc.php @@ -373,10 +373,10 @@ EOF $sHTML .= "\n"; $sHTML .= ''; - $sDialogTitle = addslashes($sTitle); + $sDialogTitleSanitized = utils::HtmlToText($sTitle); $oPage->add_ready_script( <<iId}').dialog({ width: $(window).width()*0.8, height: $(window).height()*0.8, autoOpen: false, modal: true, title: '$sDialogTitle', resizeStop: oACWidget_{$this->iId}.UpdateSizes, close: oACWidget_{$this->iId}.OnClose }); + $('#ac_dlg_{$this->iId}').dialog({ width: $(window).width()*0.8, height: $(window).height()*0.8, autoOpen: false, modal: true, title: '$sDialogTitleSanitized', resizeStop: oACWidget_{$this->iId}.UpdateSizes, close: oACWidget_{$this->iId}.OnClose }); $('#fs_{$this->iId}').bind('submit.uiAutocomplete', oACWidget_{$this->iId}.DoSearchObjects); $('#dc_{$this->iId}').resize(oACWidget_{$this->iId}.UpdateSizes); EOF diff --git a/js/utils.js b/js/utils.js index 59af2b480..b1d1f9585 100644 --- a/js/utils.js +++ b/js/utils.js @@ -739,4 +739,70 @@ Dict.Format = function () { var args = Array.from(arguments); args[0] = Dict.S(arguments[0]); return Format(args); +} + + + +/** + * Helper to Sanitize string + * + * Note: Same as in php (see \utils::Sanitize) + * + * @api + * @since 2.6.5 2.7.6 3.0.0 N°4367 + */ +const CombodoSanitizer = { + ENUM_SANITIZATION_FILTER_INTEGER: 'integer', + ENUM_SANITIZATION_FILTER_STRING: 'string', + ENUM_SANITIZATION_FILTER_CONTEXT_PARAM: 'context_param', + ENUM_SANITIZATION_FILTER_PARAMETER: 'parameter', + ENUM_SANITIZATION_FILTER_FIELD_NAME: 'field_name', + ENUM_SANITIZATION_FILTER_TRANSACTION_ID: 'transaction_id', + ENUM_SANITIZATION_FILTER_ELEMENT_IDENTIFIER: 'element_identifier', + ENUM_SANITIZATION_FILTER_VARIABLE_NAME: 'variable_name', + + /** + * @param {String} sValue The string to sanitize + * @param {String} sDefaultValue The string to return if sValue not match (used for some filters) + * @param {String} sSanitizationFilter one of the ENUM_SANITIZATION_FILTERs + */ + Sanitize: function (sValue, sDefaultValue, sSanitizationFilter) { + switch (sSanitizationFilter) { + case CombodoSanitizer.ENUM_SANITIZATION_FILTER_INTEGER: + return this._CleanString(sValue, sDefaultValue, /[^0-9-+]*/g); + + case CombodoSanitizer.ENUM_SANITIZATION_FILTER_STRING: + return $("
").text(sValue).text(); + + case CombodoSanitizer.ENUM_SANITIZATION_FILTER_TRANSACTION_ID: + return this._ReplaceString(sValue, sDefaultValue, /^([\. A-Za-z0-9_=-]*)$/g, ''); + + case CombodoSanitizer.ENUM_SANITIZATION_FILTER_PARAMETER: + return this._ReplaceString(sValue, sDefaultValue, /^([ A-Za-z0-9_=-]*)$/g); + + case CombodoSanitizer.ENUM_SANITIZATION_FILTER_FIELD_NAME: + return this._ReplaceString(sValue, sDefaultValue, /^[A-Za-z0-9_]+(->[A-Za-z0-9_]+)*$/g); + + case CombodoSanitizer.ENUM_SANITIZATION_FILTER_CONTEXT_PARAM: + return this._ReplaceString(sValue, sDefaultValue, /^[ A-Za-z0-9_=%:+-]*$/g); + + case CombodoSanitizer.ENUM_SANITIZATION_FILTER_ELEMENT_IDENTIFIER: + return this._CleanString(sValue, sDefaultValue, /[^a-zA-Z0-9_]/g); + + case CombodoSanitizer.ENUM_SANITIZATION_FILTER_VARIABLE_NAME: + return this._CleanString(sValue, sDefaultValue, /[^a-zA-Z0-9_]/g); + + } + return sDefaultValue; + }, + _CleanString: function (sValue, sDefaultValue, sRegExp) { + return sValue.replace(sRegExp, ''); + }, + _ReplaceString: function (sValue, sDefaultValue, sRegExp) { + if (sRegExp.test(sValue)) { + return sValue; + } else { + return sDefaultValue; + } + } } \ No newline at end of file diff --git a/test/VisualTest/sanitize_test.php b/test/VisualTest/sanitize_test.php new file mode 100644 index 000000000..f1a498392 --- /dev/null +++ b/test/VisualTest/sanitize_test.php @@ -0,0 +1,105 @@ + + {$sType} + {$sValue} + {$sSanitizedValue} + + + + +HTML; + + $index++; +} + +$aValues = array( + "test", + "t;e-s_t$", + "123test", + "\"('èé&=hcb test", + "
Hello!
", + "*-+7464+guigez cfuze", + "", + "()=°²€", + "éèç", +); + +$aTypes = array( + 'context_param', + 'element_identifier', + 'field_name', + 'integer', + 'parameter', + 'string', + 'transaction_id', +// 'variable_name', // introduced in 3.0.0 +); + +?> + + + + + + + + + + + + + + + + + + +
Typechaine initialechaine sanitize by phpchaine sanitize by js status test
+ +