Skip to content

Commit

Permalink
Fix XSS clean/unclean process; fixes #140
Browse files Browse the repository at this point in the history
  • Loading branch information
@cedric-anne
cedric-anne committed Mar 5, 2021
1 parent 6ad390b commit a2eb419
Show file tree
Hide file tree
Showing 5 changed files with 105 additions and 15 deletions.
4 changes: 2 additions & 2 deletions inc/commonitilobject.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7177,7 +7177,7 @@ function showTimeline($rand) {
echo "</p>";

echo "<div class='rich_text_container'>";
$richtext = Html::setRichTextContent('', $content, '', true);
$richtext = Html::getContentAsRichText($content);
$richtext = Html::replaceImagesByGallery($richtext);
echo $richtext;
echo "</div>";
Expand Down Expand Up @@ -7459,7 +7459,7 @@ function showTimeline($rand) {
echo "</div>";

echo "<div class='rich_text_container'>";
$richtext = Html::setRichTextContent('', $this->fields['content'], '', true);
$richtext = Html::getContentAsRichText($this->fields['content']);
$richtext = Html::replaceImagesByGallery($richtext);
echo $richtext;
echo "</div>";
Expand Down
25 changes: 23 additions & 2 deletions inc/html.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1310,6 +1310,7 @@ static function includeHeader($title = '', $sector = 'none', $item = 'none', $op
}

if (in_array('tinymce', $jslibs)) {
echo Html::css('public/lib/prismjs.css');
Html::requireJs('tinymce');
}

Expand Down Expand Up @@ -3937,9 +3938,9 @@ static function initEditorSystem($name, $rand = '', $display = true, $readonly =
typeof tinymce.AddOnManager.PluginManager.lookup.glpi_upload_doc != 'undefined'
? 'glpi_upload_doc'
: '',
'lists'
'lists', 'codesample'
],
toolbar: 'styleselect | bold italic | forecolor backcolor | bullist numlist outdent indent | table link image | code fullscreen',
toolbar: 'styleselect | bold italic | forecolor backcolor | bullist numlist outdent indent | table link image | code fullscreen | codesample',
$readonlyjs
});
Expand Down Expand Up @@ -3999,6 +4000,25 @@ static function setRichTextContent($name, $content, $rand, $readonly = false) {
Html::initEditorSystem($name, $rand, true, $readonly);
}

$content = self::getContentAsRichText($content);

// Re-encode content as it will be displayed as "encoded html" inside rich text editor
$content = Html::entities_deep($content);

return $content;
}

/**
* Get rich text content initially input inside rich text editor.
*
* @param string $content
*
* @return string
*
* @since 9.5.0
*/
static function getContentAsRichText($content) {

// Neutralize non valid HTML tags
$content = Html::clean($content, false, 1);

Expand Down Expand Up @@ -6475,6 +6495,7 @@ static public function requireJs($name) {
break;
case 'tinymce':
$_SESSION['glpi_js_toload'][$name][] = 'public/lib/tinymce.js';
$_SESSION['glpi_js_toload'][$name][] = 'public/lib/prismjs.js';
break;
case 'planning':
$_SESSION['glpi_js_toload'][$name][] = 'js/planning.js';
Expand Down
32 changes: 29 additions & 3 deletions inc/toolbox.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,8 @@ static function clean_cross_side_scripting_deep($value) {
return $value;
}

$in = ['<', '>'];
$out = ['&lt;', '&gt;'];
return str_replace($in, $out, $value);
$mapping = self::getXssCleanCharsMapping();
return str_replace(array_keys($mapping), array_values($mapping), $value);
}


Expand All @@ -393,11 +392,38 @@ static function unclean_cross_side_scripting_deep($value) {
return $value;
}

$mapping = self::getXssCleanCharsMapping();
foreach ($mapping as $htmlentity) {
if (strpos($value, $htmlentity) !== false) {
// Value was cleaned using new char mapping, so it must be uncleaned with same mapping
$mapping = array_reverse(self::getXssCleanCharsMapping());
return str_replace(array_values($mapping), array_keys($mapping), $value);
}
}

// Fallback to old chars mapping
$in = ['<', '>'];
$out = ['&lt;', '&gt;'];
return str_replace($out, $in, $value);
}

/**
* Get chars mapping used for XSS cleaning process.
* Keys are chars and values are corresponding html entities.
*
* @return string[]
*/
private static function getXssCleanCharsMapping() {
// Order is important here.
// `str_replace` acts as a loop, so `&` replacement must be at first place, as other replacements
// will produce new `&` that should not be replaced.
return [
'&' => '&#38;',
'<' => '&#60;',
'>' => '&#62;',
];
}


/**
* Invert fonction from clean_cross_side_scripting_deep to display HTML striping XSS code
Expand Down
1 change: 1 addition & 0 deletions lib/bundles/tinymce.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ require('tinymce/plugins/fullscreen');
require('tinymce/plugins/textcolor');
require('tinymce/plugins/colorpicker');
require('tinymce/plugins/lists');
require('tinymce/plugins/codesample');

// Custom plugins
require('../tiny_mce/custom_plugins/stickytoolbar/plugin')
58 changes: 50 additions & 8 deletions tests/units/Toolbox.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,14 +473,27 @@ function () use (&$result) {
protected function cleanProvider() {
return [
['mystring', 'mystring', null, 15, 0.56, false],
['<strong>string</strong>', '&lt;strong&gt;string&lt;/strong&gt;', null, 15, 0.56, false],
['<strong>string</strong>', '&#60;strong&#62;string&#60;/strong&#62;', null, 15, 0.56, false],
[
[null, '<strong>string</strong>', 3.2, 'string', true, '<p>my</p>', 9798],
[null, '&lt;strong&gt;string&lt;/strong&gt;', 3.2, 'string', true, '&lt;p&gt;my&lt;/p&gt;', 9798]
[null, '&#60;strong&#62;string&#60;/strong&#62;', 3.2, 'string', true, '&#60;p&#62;my&#60;/p&#62;', 9798]
]
];
}

protected function uncleanProvider() {
$dataset = $this->cleanProvider();

// Data produced by old XSS cleaning process
$dataset[] = ['<strong>string</strong>', '&lt;strong&gt;string&lt;/strong&gt;', null, 15, 0.56, false];
$dataset[] = [
[null, '<strong>string</strong>', 3.2, 'string', true, '<p>my</p>', 9798],
[null, '&lt;strong&gt;string&lt;/strong&gt;', 3.2, 'string', true, '&lt;p&gt;my&lt;/p&gt;', 9798]
];

return $dataset;
}

/**
* @dataProvider cleanProvider
*/
Expand All @@ -490,7 +503,7 @@ public function testClean_cross_side_scripting_deep($value, $expected) {
}

/**
* @dataProvider cleanProvider
* @dataProvider uncleanProvider
*/
public function testUnclean_cross_side_scripting_deep($expected, $value) {
$this->variable(\Toolbox::unclean_cross_side_scripting_deep($value))
Expand All @@ -503,9 +516,41 @@ protected function cleanHtmlProvider() {
// nested list should be preserved
$dataset[] = [
'<div>Here a list example: <ul><li>one, with nested<ul><li>nested list</li></ul></li><li>two</li></ul></div>',
'&lt;div&gt;Here a list example: &lt;ul&gt;&lt;li&gt;one, with nested&lt;ul&gt;&lt;li&gt;nested list&lt;/li&gt;&lt;/ul&gt;&lt;/li&gt;&lt;li&gt;two&lt;/li&gt;&lt;/ul&gt;'
'&#60;div&#62;Here a list example: &#60;ul&#62;&#60;li&#62;one, with nested&#60;ul&#62;&#60;li&#62;nested list&#60;/li&#62;&#60;/ul&#62;&#60;/li&#62;&#60;li&#62;two&#60;/li&#62;&#60;/ul&#62;'
];
// on* attributes are not allowed
$dataset[] = [
'<img src="test.png" alt="test image" />',
'&#60;img src="test.png" alt="test image" onerror="javascript:alert(document.cookie);" /&#62;'
];
$dataset[] = [
'<img src="test.png" alt="test image" />',
'&#60;img src="test.png" alt="test image" onload="javascript:alert(document.cookie);" /&#62;'
];
// iframes should not be preserved by default
$dataset[] = [
'Here is an iframe: ', 'Here is an iframe: &#60;iframe src="http://glpi-project.org/"&#62;&#60;/iframe&#62;'
];
// HTML comments should be removed
$dataset[] = [
'<p>Legit text</p>', '&#60;p&#62;Legit&#60;!-- This is an HTML comment --&#62; text&#60;/p&#62;'
];
// CDATA should be removed
$dataset[] = [
'<p>Legit text</p>', '&#60;p&#62;Legit&#60;![CDATA[Some CDATA]]&#62; text&#60;/p&#62;'
];

return $dataset;
}

protected function uncleanHtmlProvider() {
$dataset = $this->cleanHtmlProvider();

// Data produced by old XSS cleaning process
$dataset[] = [
'<div>Here a list example: <ul><li>one, with nested<ul><li>nested list</li></ul></li><li>two</li></ul></div>',
'&lt;div&gt;Here a list example: &lt;ul&gt;&lt;li&gt;one, with nested&lt;ul&gt;&lt;li&gt;nested list&lt;/li&gt;&lt;/ul&gt;&lt;/li&gt;&lt;li&gt;two&lt;/li&gt;&lt;/ul&gt;'
];
$dataset[] = [
'<img src="test.png" alt="test image" />',
'&lt;img src="test.png" alt="test image" onerror="javascript:alert(document.cookie);" /&gt;'
Expand All @@ -514,15 +559,12 @@ protected function cleanHtmlProvider() {
'<img src="test.png" alt="test image" />',
'&lt;img src="test.png" alt="test image" onload="javascript:alert(document.cookie);" /&gt;'
];
// iframes should not be preserved by default
$dataset[] = [
'Here is an iframe: ', 'Here is an iframe: &lt;iframe src="http://glpi-project.org/"&gt;&lt;/iframe&gt;'
];
// HTML comments should be removed
$dataset[] = [
'<p>Legit text</p>', '&lt;p&gt;Legit&lt;!-- This is an HTML comment --&gt; text&lt;/p&gt;'
];
// CDATA should be removed
$dataset[] = [
'<p>Legit text</p>', '&lt;p&gt;Legit&lt;![CDATA[Some CDATA]]&gt; text&lt;/p&gt;'
];
Expand All @@ -531,7 +573,7 @@ protected function cleanHtmlProvider() {
}

/**
* @dataProvider cleanHtmlProvider
* @dataProvider uncleanHtmlProvider
*/
public function testUnclean_html_cross_side_scripting_deep($expected, $value) {
$this->variable(\Toolbox::unclean_html_cross_side_scripting_deep($value))
Expand Down

0 comments on commit a2eb419

Please sign in to comment.