9436 - sanitize a href

- metamodel sanitization parameter constant

core/config.class.inc.php

@@ -1763,6 +1763,14 @@ class Config
 			'source_of_value'     => '',
 			'show_in_conf_sample' => false,
 		],
+		'security.disable_apply_params_auto_sanitization' => [
+			'type' => 'bool',
+			'description' => 'If true then the apply param function of the metamodel will not AUTOMATICALLY sanitize the parameters, it will still be sanitized when explicitly requested by the consumer.',
+			'default' => false,
+			'value' => false,
+			'source_of_value' => '',
+			'show_in_conf_sample' => false,
+		],
 		'behind_reverse_proxy' => [
 			'type' => 'bool',
 			'description' => 'If true, then proxies custom header (X-Forwarded-*) are taken into account. Use only if the webserver is not publicly accessible (reachable only by the reverse proxy)',

core/htmlsanitizer.class.inc.php

@@ -323,6 +323,7 @@ class HTMLDOMSanitizer extends DOMSanitizer
 
 	protected static $aAttrsWhiteList = [
 		'src' => '/^(http:|https:|data:)/i',
+		'href' => '/^(http:|https:|ftp:|mailto:|\$|%24)/i',
 	];
 
 	/**
@@ -354,30 +355,6 @@ class HTMLDOMSanitizer extends DOMSanitizer
 		'white-space',
 	];
 
-	public function __construct($sInlineImageClassName = InlineImage::class)
-	{
-		parent::__construct($sInlineImageClassName);
-
-		// Building href validation pattern from url and email validation patterns as the patterns are not used the same way in HTML content than in standard attributes value.
-		// eg. "foo@bar.com" vs "mailto:foo@bar.com?subject=Title&body=Hello%20world"
-		if (!array_key_exists('href', self::$aAttrsWhiteList)) {
-			// Regular urls
-			$sUrlPattern = utils::GetConfig()->Get('url_validation_pattern');
-
-			// Mailto urls
-			$sMailtoPattern = '(mailto:('.utils::GetConfig()->Get('email_validation_pattern').')(?:\?(?:subject|body)=([a-zA-Z0-9+\$_.-]*)(?:&(?:subject|body)=([a-zA-Z0-9+\$_.-]*))?)?)';
-
-			// Notification placeholders
-			// eg. $this->caller_id$, $this->hyperlink()$, $this->hyperlink(portal)$, $APP_URL$, $MODULES_URL$, ...
-			// Note: Authorize both $xxx$ and %24xxx%24 as the latter one is encoded when used in HTML attributes (eg. a[href])
-			$sPlaceholderPattern = '(\$|%24)[\w-]*(->[\w]*(\([\w-]*?\))?)?(\$|%24)';
-
-			$sPattern = $sUrlPattern.'|'.$sMailtoPattern.'|'.$sPlaceholderPattern;
-			$sPattern = '/'.str_replace('/', '\/', $sPattern).'/i';
-			self::$aAttrsWhiteList['href'] = $sPattern;
-		}
-	}
-
 	public function GetTagsWhiteList()
 	{
 		return static::$aTagsWhiteList;

core/metamodel.class.php

@@ -117,6 +117,11 @@ define('MYSQL_ENGINE', 'innodb');
  */
 abstract class MetaModel
 {
+	/** Constants used for apply param function sanitization parameter */
+	public const APPLY_PARAMS_SANITIZE_AUTO = 'sanitize_auto';
+	public const APPLY_PARAMS_SANITIZE_HTML = 'sanitize_html';
+	public const APPLY_PARAMS_SANITIZE_NONE = 'sanitize_none';
+
 	///////////////////////////////////////////////////////////////////////////
 	//
 	// STATIC Members
@@ -6732,14 +6737,20 @@ abstract class MetaModel
 	 *   * foo->bar : Another static placeholder
 	 *   * foo->object() : Will be developed into a set of placeholders depending on the attribute of the given \DBObject and their corresponding transformation methods {@see \AttributeDefinition::GetForTemplate()}
 	 *                     Example: foo->hyperlink(), foo->name, foo->html(name), foo->head(log), foo->head_html(log), ...
+	 * @param string $sApplySanitizeMode Allowed values are one of {@see self::APPLY_PARAMS_SANITIZE_AUTO}, {@see self::APPLY_PARAMS_SANITIZE_HTML}, {@see self::APPLY_PARAMS_SANITIZE_NONE}. Default is {@see self::APPLY_PARAMS_SANITIZE_AUTO}, which means that the result will be sanitized if it is
+	 *     HTML and if the configuration option security.disable_apply_params_auto_sanitization is not set to true.
 	 *
 	 * @link https://www.itophub.io/wiki/page?id=latest:admin:placeholders
 	 * @return string
 	 *
 	 * @throws \Exception
 	 */
-	public static function ApplyParams($sInput, $aParams)
+	public static function ApplyParams($sInput, $aParams, string $sApplySanitizeMode = self::APPLY_PARAMS_SANITIZE_AUTO)
 	{
+		if (in_array($sApplySanitizeMode, [self::APPLY_PARAMS_SANITIZE_AUTO, self::APPLY_PARAMS_SANITIZE_HTML, self::APPLY_PARAMS_SANITIZE_NONE], true) === false) {
+			throw new CoreException('Invalid value for $sApplySanitizeMode parameter', ['value' => $sApplySanitizeMode]);
+		}
+
 		$aParams = static::AddMagicPlaceholders($aParams);
 
 		// Declare magic parameters
@@ -6827,7 +6838,58 @@ abstract class MetaModel
 			}
 		}
 
-		return str_replace($aSearches, $aReplacements, $sInput);
+		$sTemplate = str_replace($aSearches, $aReplacements, $sInput);
+
+		// HTML sanitization
+		if ($sApplySanitizeMode === self::APPLY_PARAMS_SANITIZE_HTML
+			|| ($sApplySanitizeMode === self::APPLY_PARAMS_SANITIZE_AUTO && !utils::GetConfig()->Get('security.disable_apply_params_auto_sanitization') && self::StartsWithHtmlDocumentMarkup($sTemplate))) {
+			$sTemplate = DOMSanitizer::Sanitize($sTemplate);
+		}
+
+		return $sTemplate;
+	}
+
+	/**
+	 * Test if a string start with DOCTYPE or HTML tag, to detect if the string is a HTML document markup
+	 *
+	 * @param string $sText
+	 *
+	 * @return bool
+	 */
+	private static function StartsWithHtmlDocumentMarkup(string $sText): bool
+	{
+		if (!preg_match('/^\s*(?:<!DOCTYPE\s+html\b|<\/?[a-z][a-z0-9:-]*(?:\s|\/?>))/i', $sText)) {
+			return false;
+		}
+
+		$bInitialUseInternalErrorsFlag = libxml_use_internal_errors(true);
+
+		$oDom = new DOMDocument();
+		$ok = $oDom->loadHTML(
+			'<!DOCTYPE html><html><body>'.$sText.'</body></html>',
+			LIBXML_NOWARNING | LIBXML_NOERROR
+		);
+
+		libxml_clear_errors();
+		libxml_use_internal_errors($bInitialUseInternalErrorsFlag);
+
+		if (!$ok) {
+			return false;
+		}
+
+		// Si on trouve des balises "réelles" dans le body, on considère qu'il y a du HTML
+		$body = $oDom->getElementsByTagName('body')->item(0);
+		if (!$body) {
+			return false;
+		}
+
+		foreach ($body->childNodes as $node) {
+			if ($node->nodeType === XML_ELEMENT_NODE) {
+				return true;
+			}
+		}
+
+		return false;
 	}
 
 	/**

tests/php-unit-tests/unitary-tests/core/ActionEmailTest.php

@@ -46,7 +46,7 @@ class ActionEmailTest extends ItopDataTestCase
 		$sHtml =
 <<<HTML
 <body>
-	<table data-something-that-would-be-removed-by-the-sanitizer-through-ckeditor-but-that-will-stay-with-the-template="bar">
+	<table>
 		<tr><td>Formatted eMail</td></tr>
 		<tr><td>\$content\$</td></tr>
 </body>
@@ -260,13 +260,13 @@ HTML
 				['body' => '<p>Ticket "$this->title$" created.</p>', 'html_template' => true],
 				['body' =>
 <<<HTML
-<body>
-	<table data-something-that-would-be-removed-by-the-sanitizer-through-ckeditor-but-that-will-stay-with-the-template="bar">
+
+	<table>
 		<tr><td>Formatted eMail</td></tr>
 		<tr><td><div class="email-is-html-content">
 	<p>Ticket "Test UserRequest" created.</p>
 </div></td></tr>
-</body>
+</table>
 HTML
 				],
 			],

tests/php-unit-tests/unitary-tests/core/MetaModelTest.php

@@ -6,6 +6,7 @@ use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
 use CoreException;
 use DBObjectSearch;
 use MetaModel;
+use utils;
 
 /**
  * Class MetaModelTest
@@ -20,16 +21,25 @@ class MetaModelTest extends ItopDataTestCase
 	protected static $sDefaultUserRequestTitle = 'Unit test title';
 	protected static $sDefaultUserRequestDescription = 'Unit test description';
 	protected static $sDefaultUserRequestDescriptionAsHtml = '<p>Unit test description</p>';
+	private bool $bInitialSecurityFlagDisabled = false;
 
 	protected function setUp(): void
 	{
 		parent::setUp();
 		$this->RequireOnceItopFile('/core/metamodel.class.php');
+
+		// save the initial security flag as it will be modified in some tests and we want to restore it to its initial value at the end of the test
+		$this->bInitialSecurityFlagDisabled = utils::GetConfig()->Get('security.disable_apply_params_auto_sanitization');
+
 	}
 
 	protected function tearDown(): void
 	{
 		$this->InvokeNonPublicStaticMethod('PluginManager', 'ResetPlugins');
+
+		// restore the initial security flag value
+		utils::GetConfig()->Set('security.disable_apply_params_auto_sanitization', $this->bInitialSecurityFlagDisabled);
+
 		parent::tearDown();
 	}
 
@@ -144,7 +154,66 @@ class MetaModelTest extends ItopDataTestCase
 			'Placeholder for an object HTML attribute as HTML' => [
 				'$this->html(description)$',
 				$aParams,
-				'<div class="HTML ibo-is-html-content" ><p>Unit test description</p></div>', // As the AttributeText::GetForHTML() called by AttributeDefinition::GetForTemplate() adds this markup, we have to mock it here as well
+				'<div class="HTML ibo-is-html-content"><p>Unit test description</p></div>', // As the AttributeText::GetForHTML() called by AttributeDefinition::GetForTemplate() adds this markup, we have to mock it here as well
+			],
+		];
+	}
+
+	/**
+	 * @group itopRequestMgmt
+	 * @covers       MetaModel::ApplyParams()
+	 * @dataProvider ApplyParamsSanitizationProvider
+	 *
+	 * @param string $sInput
+	 * @param array $aParams
+	 * @param string $sExpectedOutput
+	 * @param string $sApplySanitizeMode
+	 *
+	 * @throws \Exception
+	 */
+	public function testApplyParamsSanitization($sInput, $aParams, $sExpectedOutput, string $sApplySanitizeMode = MetaModel::APPLY_PARAMS_SANITIZE_AUTO, bool $bSecurityFlagDisabled = true)
+	{
+		utils::GetConfig()->Set('security.disable_apply_params_auto_sanitization', $bSecurityFlagDisabled);
+
+		$sGeneratedOutput = MetaModel::ApplyParams($sInput, $aParams, $sApplySanitizeMode);
+
+		$this->assertEquals($sExpectedOutput, $sGeneratedOutput, "ApplyParams test returned $sGeneratedOutput");
+	}
+
+	public function ApplyParamsSanitizationProvider()
+	{
+		$aParams = [
+			'foo->javascript_injection' => 'javascript:alert(1)/*https://example.com*/',
+		];
+
+		return [
+			'Placeholder with malicious javascript injection (sanitization auto)' => [
+				'<a href="$foo->javascript_injection$">Hyperlink</a>',
+				$aParams,
+				'<a>Hyperlink</a>',
+				'sanitize_auto',
+				false,
+			],
+			'Placeholder with malicious javascript injection (sanitization auto but security flag disabled)' => [
+				'<a href="$foo->javascript_injection$">Hyperlink</a>',
+				$aParams,
+				'<a href="javascript:alert(1)/*https://example.com*/">Hyperlink</a>',
+				'sanitize_auto',
+				true,
+			],
+			'Placeholder with malicious javascript injection (sanitization html)' => [
+				'<a href="$foo->javascript_injection$">Hyperlink</a>',
+				$aParams,
+				'<a>Hyperlink</a>',
+				'sanitize_html',
+				false,
+			],
+			'Placeholder with malicious javascript injection (sanitization off)' => [
+				'<a href="$foo->javascript_injection$">Hyperlink</a>',
+				$aParams,
+				'<a href="javascript:alert(1)/*https://example.com*/">Hyperlink</a>',
+				'sanitize_none',
+				false,
 			],
 		];
 	}
@@ -498,6 +567,30 @@ class MetaModelTest extends ItopDataTestCase
 			'Purge 10 items with a max_chunk_size of 1000 (default value) should be perfomed in 1 step' => [1000, 3],
 		];
 	}
+
+	/**
+	 * @dataProvider StartsWithHtmlDocumentMarkupProvider
+	 */
+	public function testStartsWithHtmlDocumentMarkup(string $sInput, bool $bExpected)
+	{
+		$bTested = $this->InvokeNonPublicStaticMethod('MetaModel', 'StartsWithHtmlDocumentMarkup', [$sInput]);
+		self::assertSame($bExpected, $bTested);
+	}
+
+	public static function StartsWithHtmlDocumentMarkupProvider()
+	{
+		return [
+			"CONTENT_EMPTY" => ['', false],
+			"CONTENT_WITH_DOCTYPE" => ['<!DOCTYPE html><html><head><title>Title</title></head><body>Content</body></html>', true],
+			"CONTENT_WITH_FULL_HTML" => ['<html><head><title>Title</title></head><body>Content</body></html>', true],
+			"CONTENT_WITH_SIMPLE_TAG" => ['<a href="https://www.itop.com">link</a>', true],
+			"CONTENT_WITH_UNKNOWN_TAG" => ['<unknown></unknown>', true],
+			"CONTENT_WITH_UNCLOSED_TAG" => ['<unknown>', true],
+			"CONTENT_WITH_RAW_TEXT" => ['raw text', false],
+			"CONTENT_WITH_LEADING_WHITESPACE" => [" <p>this paragraph</p>", true],
+			"CONTENT_WITH_RAW_TEXTWITH_NESTED_HTML" => ['raw text <p>this paragraph</p>', false],
+		];
+	}
 }
 
 abstract class Wizzard

tests/php-unit-tests/unitary-tests/core/sanitizer/HTMLDOMSanitizerTest.php

@@ -218,6 +218,54 @@ class HTMLDOMSanitizerTest extends AbstractDOMSanitizerTest
 				'expected' => '<ol start="100" reversed="reversed" type="I"><li value="101">Some list item</li></ol>',
 			],
 
+			'link href url' => [
+				'html' => '<a href="https://www.itop.com">Hyperlink</a>',
+				'expected' => '<a href="https://www.itop.com">Hyperlink</a>',
+			],
+			'link href short url without protocol' => [
+				'html' => '<a href="www.itop.com">Hyperlink</a>',
+				'expected' => '<a>Hyperlink</a>',
+			],
+			'link href javascript injection' => [
+				'html' => '<a href="javascript:alert(1)/*https://example.com*/">Hyperlink</a>',
+				'expected' => '<a>Hyperlink</a>',
+			],
+			'link href with placeholder' => [
+				'html' => '<a href="https://www.itop.com/$this->link$">Hyperlink</a>',
+				'expected' => '<a href="https://www.itop.com/%24this-&gt;link%24">Hyperlink</a>',
+			],
+			'link href with placeholders combinaison' => [
+				'html' => '<a href="https://www.itop.com/$this->link$/$this->ref$">Hyperlink</a>',
+				'expected' => '<a href="https://www.itop.com/%24this-&gt;link%24/%24this-&gt;ref%24">Hyperlink</a>',
+			],
+			'link href with placeholder as base ref' => [
+				'html' => '<a href="$this->link$/$this->ref$">Hyperlink</a>',
+				'expected' => '<a href="%24this-&gt;link%24/%24this-&gt;ref%24">Hyperlink</a>',
+			],
+			'link href with placeholder constant' => [
+				'html' => '<a href="$APPLICATION_URL$">Hyperlink</a>',
+				'expected' => '<a href="%24APPLICATION_URL%24">Hyperlink</a>',
+			],
+			'link href with placeholder mixed' => [
+				'html' => '<a href="$APPLICATION_URL$/$this->link$/$this->ref$">Hyperlink</a>',
+				'expected' => '<a href="%24APPLICATION_URL%24/%24this-&gt;link%24/%24this-&gt;ref%24">Hyperlink</a>',
+			],
+			'link href with javascript injection and placeholder' => [
+				'html' => '<a href="javascript:alert(1)/*$APPLICATION_URL$*">Hyperlink</a>',
+				'expected' => '<a>Hyperlink</a>',
+			],
+			'link href with vbscript injection and placeholder' => [
+				'html' => '<a href="vbscript:alert(1)/*$APPLICATION_URL$*">Hyperlink</a>',
+				'expected' => '<a>Hyperlink</a>',
+			],
+			'link href with data injection and placeholder' => [
+				'html' => '<a href="data:alert(1)/*$APPLICATION_URL$*">Hyperlink</a>',
+				'expected' => '<a>Hyperlink</a>',
+			],
+			'link href with toto injection and placeholder' => [
+				'html' => '<a href="toto:alert(1)/*$APPLICATION_URL$*">Hyperlink</a>',
+				'expected' => '<a>Hyperlink</a>',
+			],
 		];
 	}