Skip to content

Commit ebbf6e5

Browse files
committed
N°4127 - Security: Fix XSS vulnerability in object attribute's tooltip
1 parent c76d4f1 commit ebbf6e5

File tree

6 files changed

+54
-23
lines changed

6 files changed

+54
-23
lines changed

application/cmdbabstract.class.inc.php

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2533,14 +2533,13 @@ public static function GetFormElementForField($oPage, $sClass, $sAttCode, $oAttD
25332533
$sDisplayValueForHtml = utils::EscapeHtml($sDisplayValue);
25342534

25352535
// Adding tooltip so we can read the whole value when its very long (eg. URL)
2536-
$sTip = '';
2536+
$sTip = '';
25372537
if (!empty($sDisplayValue)) {
25382538
$sTip = 'data-tooltip-content="'.$sDisplayValueForHtml.'"';
2539-
$oPage->add_ready_script(
2540-
<<<EOF
2539+
$oPage->add_ready_script(<<<JS
25412540
$('#{$iId}').on('keyup', function(evt, sFormId){
2542-
var sVal = $('#{$iId}').val();
2543-
var oTippy = this._tippy;
2541+
let sVal = $('#{$iId}').val();
2542+
const oTippy = this._tippy;
25442543
25452544
if(sVal === '')
25462545
{
@@ -2553,7 +2552,7 @@ public static function GetFormElementForField($oPage, $sClass, $sAttCode, $oAttD
25532552
}
25542553
oTippy.setContent(sVal);
25552554
});
2556-
EOF
2555+
JS
25572556
);
25582557
}
25592558

css/backoffice/vendors/_all.scss

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
@import "bulma-variables-overload";
2020
@import "../../../node_modules/bulma-scss/bulma";
2121
@import "ckeditor";
22+
@import "tippy";
2223
@import "jqueryui";
2324
@import "jquery-multiselect";
2425
@import "datatables";

css/backoffice/vendors/_tippy.scss

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
/*!
2+
* @copyright Copyright (C) 2010-2021 Combodo SARL
3+
* @license http://opensource.org/licenses/AGPL-3.0
4+
*/
5+
6+
// Allow plain text (opposite of HTML) multi-lines tooltips to be displayed correctly, otherwise it will all be on a single line.
7+
.tippy-content {
8+
white-space: pre-line;
9+
}

datamodels/2.x/itop-structure/precompiled-themes/fullmoon/main.css

Lines changed: 8 additions & 1 deletion
Large diffs are not rendered by default.

datamodels/2.x/itop-structure/precompiled-themes/test-red/main.css

Lines changed: 8 additions & 1 deletion
Large diffs are not rendered by default.

js/utils.js

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,22 @@ const CombodoGlobalToolbox = {
709709
|| oDOMElem.contains(efp(oRect.left, oRect.bottom))
710710
);
711711
}
712+
},
713+
/**
714+
* This method should be a JS mirror of the PHP {@see utils::FilterXSS} method
715+
*
716+
* @param sInput {string} Input text to filter from XSS attacks
717+
* @returns {string} The sInput string filtered from possible XSS attacks
718+
* @constructor
719+
* @since 3.0.0
720+
*/
721+
FilterXSS: function (sInput) {
722+
let sOutput = sInput;
723+
724+
// Remove HTML script tags
725+
sOutput = sOutput.replace(/<script/g, '&lt;script WARNING: scripts are not allowed in tooltips');
726+
727+
return sOutput;
712728
}
713729
};
714730

@@ -731,9 +747,7 @@ const CombodoTooltip = {
731747
* @constructor
732748
*/
733749
InitTooltipFromMarkup: function (oElem, bForce = false) {
734-
const oOptions = {
735-
allowHTML: true, // Always true so line breaks can work. Don't worry content will be sanitized.
736-
};
750+
const oOptions = {};
737751

738752
// First, check if the tooltip isn't already instantiated
739753
if ((oElem.attr('data-tooltip-instantiated') === 'true') && (bForce === false)) {
@@ -746,24 +760,18 @@ const CombodoTooltip = {
746760
// Content must be reworked before getting into the tooltip
747761
// - Should we enable HTML content or keep text as is
748762
const bEnableHTML = oElem.attr('data-tooltip-html-enabled') === 'true';
763+
oOptions['allowHTML'] = bEnableHTML;
749764

750765
// - Content should be sanitized unless the developer says otherwise
751766
// Note: Condition is inversed on purpose. When the developer is instantiating a tooltip,
752-
// we want him/her to explicitly declare that he/she wants the sanitizer to be skipped.
767+
// we want they to explicitly declare that they want the sanitizer to be skipped.
753768
// Whereas in this code, it's easier to follow the logic with the variable oriented this way.
754769
const bSanitizeContent = oElem.attr('data-tooltip-sanitizer-skipped') !== 'true';
755770

756-
// - Sanitize content and make sure line breaks are kept
757-
const oTmpContentElem = $('<div />').html(oElem.attr('data-tooltip-content'));
758-
let sContent = '';
759-
if (bEnableHTML) {
760-
sContent = oTmpContentElem.html();
761-
if (bSanitizeContent) {
762-
sContent = sContent.replace(/<script/g, '&lt;script WARNING: scripts are not allowed in tooltips');
763-
}
764-
} else {
765-
sContent = oTmpContentElem.text();
766-
sContent = sContent.replace(/(\r\n|\n\r|\r|\n)/g, '<br/>');
771+
let sContent = oElem.attr('data-tooltip-content');
772+
// - Check if both HTML and sanitizer are enabled
773+
if (bEnableHTML && bSanitizeContent) {
774+
sContent = CombodoGlobalToolbox.FilterXSS(sContent);
767775
}
768776
oOptions['content'] = sContent;
769777

0 commit comments

Comments
 (0)