N°9436 - sanitize a href

application/utils.inc.php

@@ -3300,4 +3300,47 @@ TXT
 			}
 		}
 	}
+
+	/**
+	 * 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
+	 */
+	public 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);
+
+		$dom = new DOMDocument();
+		$ok = $dom->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 = $dom->getElementsByTagName('body')->item(0);
+		if (!$body) {
+			return false;
+		}
+
+		foreach ($body->childNodes as $node) {
+			if ($node->nodeType === XML_ELEMENT_NODE) {
+				return true;
+			}
+		}
+
+		return false;
+	}
 }

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

@@ -361,6 +361,10 @@ class HTMLDOMSanitizer extends DOMSanitizer
 		// 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)) {
+
+			// Forbidden patterns
+			$sForbiddenPattern = '(javascript:|vbscript:|data:)';
+
 			// Regular urls
 			$sUrlPattern = utils::GetConfig()->Get('url_validation_pattern');
 
@@ -372,8 +376,9 @@ class HTMLDOMSanitizer extends DOMSanitizer
 			// 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';
+			// Final pattern: allow urls, mailto and placeholders, but not if they start with a forbidden pattern
+			$sPattern = '(?!(?:'.$sForbiddenPattern.')$)(?:'.$sUrlPattern.'|'.$sMailtoPattern.'|'.$sPlaceholderPattern.')';
+			$sPattern = '/^'.str_replace('/', '\/', $sPattern).'/i';
 			self::$aAttrsWhiteList['href'] = $sPattern;
 		}
 	}

core/metamodel.class.php

@@ -6732,14 +6732,19 @@ 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 One of [auto,html,none] to specify if the result should be sanitized or not. Default is '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 = 'auto')
 	{
+		if(in_array($sApplySanitizeMode, ['auto', 'html', 'none'], true) === false) {
+			throw new CoreException('Invalid value for $sApplySanitizeMode parameter', ['value' => $sApplySanitizeMode]);
+		}
+
 		$aParams = static::AddMagicPlaceholders($aParams);
 
 		// Declare magic parameters
@@ -6827,7 +6832,15 @@ abstract class MetaModel
 			}
 		}
 
-		return str_replace($aSearches, $aReplacements, $sInput);
+		$sTemplate = str_replace($aSearches, $aReplacements, $sInput);
+
+		// HTML sanitization
+		if ($sApplySanitizeMode === 'html'
+		|| ($sApplySanitizeMode === 'auto' && !utils::GetConfig()->Get('security.disable_apply_params_auto_sanitization') && utils::StartsWithHtmlDocumentMarkup($sTemplate))) {
+			$sTemplate = DOMSanitizer::Sanitize($sTemplate);
+		}
+
+		return $sTemplate;
 	}
 
 	/**

tests/php-unit-tests/unitary-tests/application/utilsTest.php

@@ -1069,4 +1069,28 @@ INI;
 		utils::Unserialize($sData);
 
 	}
+
+	/**
+	 * @dataProvider StartsWithHtmlDocumentMarkupProvider
+	 */
+	public function testStartsWithHtmlDocumentMarkup(string $sInput, bool $bExpected)
+	{
+		$bTested = utils::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],
+		];
+	}
 }

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 = '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>',
+				'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>',
+				'auto',
+				true,
+			],
+			'Placeholder with malicious javascript injection (sanitization html)' => [
+				'<a href="$foo->javascript_injection$">Hyperlink</a>',
+				$aParams,
+				'<a>Hyperlink</a>',
+				'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>',
+				'none',
+				false,
 			],
 		];
 	}

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

@@ -218,6 +218,42 @@ 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>',
+			],
 		];
 	}