Security: stored XSS by members through editor HTML, user attributes and page names

Start with the affected PHP files listed in the issue: system/core/document.php, system/core/pages.php, system/core/base.php, and the two files under system/datatypes/. Review the suggested patch and check the HTML sanitization, output escaping, username validation, and page creation against the described attack paths. Done means all listed paths are addressed while existing editor content continues to work; the payload names no tests.

Summary

Editor fields are only checked for <script> elements. Event handler attributes, javascript: URLs and similar constructs pass the check and are stored. In addition, several views print the user name, full name and e-mail address of members, and page names, into HTML without escaping. Any member can thereby store markup that runs script in the browser of other members and administrators, and a link from the editor can create a page whose name carries markup.

Severity: High. Any registered member (also an invitee who just registered) can run script in the session of an administrator, who can then be made to change site settings, users or themes.

Affected code (dev, 5dae13a)

• system/core/document.php:735-753 HTMLForm::validateHtmlField() only rejects script elements; every <editor> field of every datatype (text page, mailing list, peer-reviewed article) relies on it.
• system/core/pages.php:577 wikify_link() creates a new page for any link of the form <language>/<name> in editor HTML, without validatePagename(), so the name can contain markup.
• system/datatypes/index.php:70 prints page names (and the data type) unescaped in the page index.
• system/core/base.php:815 hypha_indexLanguages() prints the page name unescaped in a link.
• system/datatypes/settings.php:59, 66, 68, 78, 188, 190, 200, 631, 635, 637, 641, 643, 646, 678, 679, 681 print user name, full name, e-mail address (and the user name from the URL) unescaped; :646 puts the user name in a JavaScript string.
• system/core/base.php:470-488 hypha_setUser() accepts any character in a user name, while the name is used in URLs and in JavaScript string arguments built by makeAction() (settings.php:59, 686, 694).

The same code is present on master (other line numbers).

Impact

A member can store content that executes script when another member or an administrator views a page, the page index, the language selector or the member list in the settings. The script runs with the rights of the viewer, so an administrator's session can be used to perform administrative actions.

Suggested fix

• Sanitise editor HTML on the server instead of only looking for script: remove elements that execute or redirect (script, style, base, meta, link, frame(set), applet, template, noscript, svg, math, ...), all on* attributes and srcdoc, styles with script constructs, and every URL attribute whose scheme is not on an allow-list (relative, http, https, mailto, tel, and hypha: page links as stored by wikify(); raster data:image only for img src). Elements the editor produces, including the iframe, embed, object, video and audio of its embed plugin, are kept, so existing content keeps working. A <script> element is still refused as before, the rest is removed silently so the member keeps the text.
• Escape every user attribute and page name printed into HTML with htmlspecialchars(), and encode the user name in the JavaScript string with json_encode().
• Refuse < > " ' & \ / and control characters in new user names.
• Create pages from editor links only when the name survives validatePagename() unchanged.

Content that is already stored is not cleaned by this patch; it is cleaned when it is saved again. Sanitising on output (for example in dewikify()) could be a follow-up.

Patch

Note: this patch is a suggestion and a starting point, not a finished fix. Please review it and adapt it as you see fit.

diff --git a/system/core/base.php b/system/core/base.php
index 13a7dd0..fe58378 100644
--- a/system/core/base.php
+++ b/system/core/base.php
@@ -474,7 +474,12 @@
 		    && hypha_getUserByName($username) != $user && !isAdmin()) {
 			return __('user-exists');
 		} else if (($username == 'hypha')
-		           || (strpos($username, 'newUser_') !== false)) {
+		           || (strpos($username, 'newUser_') !== false)
+		           // The user name ends up in URLs and in
+		           // JavaScript string arguments of buttons
+		           // (makeAction), so refuse characters that
+		           // can break out of those.
+		           || preg_match('#[<>"\'&\\\\/\x00-\x1f\x7f]#', $username)) {
 			return __('cant-use-that-name');
 		} else {
 			if ($username) $user->setAttribute('username', $username);
@@ -812,7 +817,7 @@
 		foreach($langList as $lang) {
 			if ($lang == $language) $index.= '<span class="language selected">'.$lang.'</span>';
 			elseif (!$page || array_key_exists($lang, $pageLangList)) {
-				if ($page) $index.= '<span class="language"><a href="'.$lang.'/'.$pageLangList[$lang].'">'.$lang.'</a></span>';
+				if ($page) $index.= '<span class="language"><a href="'.htmlspecialchars($lang.'/'.$pageLangList[$lang]).'">'.$lang.'</a></span>';
 				else $index.= '<span class="language"><a href="index/'.$lang.'">'.$lang.'</a></span>';
 			}
 			else $index.= '<span class="language disabled">'.$lang.'</span>';
diff --git a/system/core/document.php b/system/core/document.php
index 08417e5..669e845 100644
--- a/system/core/document.php
+++ b/system/core/document.php
@@ -273,6 +273,95 @@
 		}
 	}
 
+	/*
+		Function: sanitizeHtml
+
+		Removes everything from the children of the given element
+		that can run script in the browser of a visitor: elements
+		that execute or redirect (script, base, meta, ...), event
+		handler attributes (on*), srcdoc, CSS with script, and URL
+		attributes whose scheme is not on an allow-list.
+
+		Elements and attributes the editor produces (including the
+		iframe, embed, object, video and audio of its embed plugin)
+		are kept, so existing content keeps working. Links in the
+		form hypha:<id> as stored by wikify() stay allowed.
+
+		Parameters:
+		$elem - HyphaDomElement whose descendants are cleaned
+	*/
+	function sanitizeHtml(HyphaDomElement $elem) {
+		// Removed together with their content. svg and math
+		// are removed because their elements (animate, set,
+		// foreignObject) can set URLs and carry script in ways
+		// the attribute checks below do not cover; the editor
+		// never produces them.
+		$forbiddenElements = ['script', 'style', 'base', 'meta', 'link', 'frame', 'frameset', 'applet', 'template', 'noscript', 'noembed', 'noframes', 'xmp', 'plaintext', 'svg', 'math'];
+		// Attributes that hold a URL. Names are compared
+		// without prefix, so xlink:href counts as href.
+		$urlAttributes = ['href', 'src', 'action', 'data', 'poster', 'background', 'cite', 'longdesc', 'codebase', 'dynsrc', 'lowsrc', 'usemap', 'classid', 'archive'];
+
+		foreach (iterator_to_array($elem->getElementsByTagName('*')) as $node) {
+			// Skip nodes already detached with a removed
+			// ancestor.
+			if (!$node->parentNode)
+				continue;
+			$tag = strtolower(preg_replace('/^.*:/', '', $node->nodeName));
+			if (in_array($tag, $forbiddenElements, true)) {
+				$node->parentNode->removeChild($node);
+				continue;
+			}
+
+			foreach (iterator_to_array($node->attributes) as $attr) {
+				$name = strtolower(preg_replace('/^.*:/', '', $attr->nodeName));
+				if (str_starts_with($name, 'on') || in_array($name, ['srcdoc', 'srcset', 'formaction'], true)) {
+					$node->removeAttributeNode($attr);
+				} else if (in_array($name, $urlAttributes, true)) {
+					if (!sanitizeHtmlIsSafeUrl($attr->value, $tag == 'img' && $name == 'src'))
+						$node->removeAttributeNode($attr);
+				} else if ($name == 'style') {
+					// Browsers ignore comments and
+					// whitespace inside these keywords, so
+					// ignore them here too. CSS escapes
+					// (backslashes) can hide a keyword and
+					// the editor never writes them, so a
+					// style with one is dropped as well.
+					$css = strtolower(preg_replace('%/\*.*?\*/|\s%s', '', $attr->value));
+					foreach (['\\', 'expression(', 'javascript:', 'vbscript:', '-moz-binding', 'behavior:'] as $bad) {
+						if (str_contains($css, $bad)) {
+							$node->removeAttributeNode($attr);
+							break;
+						}
+					}
+				}
+			}
+
+			// A param can hand a URL to an embedded object.
+			if ($tag == 'param' && in_array(strtolower($node->getAttribute('name')), ['src', 'movie', 'url', 'href', 'data', 'code'], true) && !sanitizeHtmlIsSafeUrl($node->getAttribute('value')))
+				$node->removeAttribute('value');
+		}
+	}
+
+	/*
+		Function: sanitizeHtmlIsSafeUrl
+
+		Returns true when the URL is relative or uses an allowed
+		scheme: http, https, mailto, tel and hypha (page links as
+		stored by wikify()). Images may also be inline data:image
+		URLs of raster formats, as pasted into the editor.
+	*/
+	function sanitizeHtmlIsSafeUrl(string $url, bool $allowDataImage = false) : bool {
+		// Browsers strip control characters and spaces around
+		// a URL and tabs and newlines inside the scheme, so
+		// strip those before looking at the scheme.
+		$url = strtolower(preg_replace('/[\x00-\x20\x7f]+/', '', $url));
+		if (!preg_match('/^([a-z][a-z0-9+.\-]*):/', $url, $matches))
+			return true; // relative URL
+		if (in_array($matches[1], ['http', 'https', 'mailto', 'tel', 'hypha'], true))
+			return true;
+		return $allowDataImage && preg_match('#^data:image/(png|gif|jpeg|webp);#', $url) === 1;
+	}
+
 	/*
 		Class: HTMLForm
 
@@ -726,7 +815,8 @@
 
 		/**
 		 * Ensures that the given field, if not empty, contains
-		 * no obviously unsafe HTML.
+		 * no script: refuses script elements and removes
+		 * other unsafe HTML (see sanitizeHtml()).
 		 *
 		 * Normally, you should use the `editor` field to make
 		 * validate() apply this check automatically instead of
@@ -749,6 +839,14 @@
 				$this->errors[$name] = __('html-field-contains-script');
 				return false;
 			}
+
+			// Anything else that can run script (event
+			// handlers, javascript: URLs, ...) is removed
+			// rather than refused, so the member keeps the
+			// rest of the text. Callers read the cleaned
+			// value through dataFor().
+			sanitizeHtml($dom);
+			$this->data[$name] = $dom->getInnerHtml();
 			return true;
 		}
 
diff --git a/system/core/pages.php b/system/core/pages.php
index 786c398..029304c 100644
--- a/system/core/pages.php
+++ b/system/core/pages.php
@@ -568,6 +568,12 @@ EOF;
 
 		$page = hypha_getPage($language, $pagename);
 
+		// Only create pages with names that the regular page
+		// creation would also accept; the name comes from
+		// member-supplied HTML and is printed in page lists.
+		if (!$page && validatePagename($pagename) !== $pagename)
+			return;
+
 		if (!$page) {
 			$hyphaXml->lockAndReload();
 			// recheck just in case it got added in the
diff --git a/system/datatypes/index.php b/system/datatypes/index.php
index e5c743a..e477fe6 100644
--- a/system/datatypes/index.php
+++ b/system/datatypes/index.php
@@ -67,7 +67,7 @@
 				}
 				$privatePos = strpos($pagename, '&#;');
 				if ($privatePos) $pagename = substr($pagename, 0, $privatePos);
-				$htmlList[] = '<div class="index-item type_'.$pageListDatatype[$pagename].' '.($privatePos ? 'is-private' : 'is-public').'"><a href="'.$language.'/'.$pagename.'">'.showPagename($pagename).'</a></div>';
+				$htmlList[] = '<div class="index-item type_'.htmlspecialchars($pageListDatatype[$pagename]).' '.($privatePos ? 'is-private' : 'is-public').'"><a href="'.htmlspecialchars($language.'/'.$pagename).'">'.htmlspecialchars(showPagename($pagename)).'</a></div>';
 			}
 
 			$html = '<div class="index">';
diff --git a/system/datatypes/settings.php b/system/datatypes/settings.php
index 3e01998..96e32bf 100644
--- a/system/datatypes/settings.php
+++ b/system/datatypes/settings.php
@@ -56,16 +56,16 @@
 			$isoLangList = Language::getIsoList();
 			if (isAdmin() || $hyphaUser->getAttribute('username') == $username) {
 				$account = hypha_getUserByName($username);
-				$this->html->writeToElement('pagename', __('change-account').' `'.$username.'`');
+				$this->html->writeToElement('pagename', __('change-account').' `'.htmlspecialchars($username).'`');
 				if ($account->getAttribute('rights')=='admin') $this->html->writeToElement('pageCommands', makeButton(__('unadmin'), makeAction('settings', 'settingsUnadmin', $username)));
 				ob_start();
 ?>
 <table class="section" width="100%">
 	<tr>
 		<th><?=__('username')?>:</th>
-		<td><input name="settingsUsername" id="settingsUsername" type="text" size="20" value="<?=$account->getAttribute('username')?>" /></td>
+		<td><input name="settingsUsername" id="settingsUsername" type="text" size="20" value="<?=htmlspecialchars($account->getAttribute('username'))?>" /></td>
 		<th><?=__('fullname')?>:</th>
-		<td><input name="settingsFullname" id="settingsFullname" type="text" size="20" value="<?=$account->getAttribute('fullname')?>" /></td>
+		<td><input name="settingsFullname" id="settingsFullname" type="text" size="20" value="<?=htmlspecialchars($account->getAttribute('fullname'))?>" /></td>
 	</tr>
 	<tr>
 		<th><?=__('password')?>:</th>
@@ -75,7 +75,7 @@
 	</tr>
 	<tr>
 		<th><?=__('email')?>:</th>
-		<td><input name="settingsEmail" id="settingsEmail" type="text" size="20" value="<?=$account->getAttribute('email')?>" /></td>
+		<td><input name="settingsEmail" id="settingsEmail" type="text" size="20" value="<?=htmlspecialchars($account->getAttribute('email'))?>" /></td>
 		<th><?=__('interface-language')?>:</th>
 		<td><select name="settingsUiLang" id="settingsUiLang"><?php foreach($uiLangList as $lang) echo '<option value="'.$lang.'"'.($account->getAttribute('language')==$lang?' selected':'').'>'.$isoLangList[$lang].'</option>'; ?></select></td>
 	</tr>
@@ -185,9 +185,9 @@
 <table class="section">
 	<tr>
 		<th><?=__('username')?>:</th>
-		<td><input name="settingsUsername" id="settingsUsername" type="text" size="20" value="<?=$user->getAttribute('username')?>" /></td>
+		<td><input name="settingsUsername" id="settingsUsername" type="text" size="20" value="<?=htmlspecialchars($user->getAttribute('username'))?>" /></td>
 		<th><?=__('fullname')?>:</th>
-		<td><input name="settingsFullname" id="settingsFullname" type="text" size="20" value="<?=$user->getAttribute('fullname')?>" /></td>
+		<td><input name="settingsFullname" id="settingsFullname" type="text" size="20" value="<?=htmlspecialchars($user->getAttribute('fullname'))?>" /></td>
 	</tr>
 	<tr>
 		<th><?=__('password')?>:</th>
@@ -197,7 +197,7 @@
 	</tr>
 	<tr>
 		<th><?=__('email')?>:</th>
-		<td><input name="settingsEmail" id="settingsEmail" type="text" size="20" value="<?=$user->getAttribute('email')?>" /></td>
+		<td><input name="settingsEmail" id="settingsEmail" type="text" size="20" value="<?=htmlspecialchars($user->getAttribute('email'))?>" /></td>
 		<th><?=__('interface-language')?>:</th>
 		<td><select name="settingsUiLang" id="settingsUiLang"><?php foreach($uiLangList as $lang) echo '<option value="'.$lang.'"'.($user->getAttribute('language')==$lang?' selected':'').'>'.$isoLangList[$lang].'</option>'; ?></select></td>
 	</tr>
@@ -628,22 +628,22 @@
 			}
 			ob_start();
 ?>
-<h3><?=__('personal-settings')?> `<?=$hyphaUser->getAttribute('username')?>`</h3>
+<h3><?=__('personal-settings')?> `<?=htmlspecialchars($hyphaUser->getAttribute('username'))?>`</h3>
 <table class="section personal-settings">
 	<tr>
 		<th><?=__('username')?>:</th>
-		<td><?=$hyphaUser->getAttribute('username')?></td>
+		<td><?=htmlspecialchars($hyphaUser->getAttribute('username'))?></td>
 		<th><?=__('fullname')?>:</th>
-		<td><?=$hyphaUser->getAttribute('fullname')?></td>
+		<td><?=htmlspecialchars($hyphaUser->getAttribute('fullname'))?></td>
 	</tr>
 	<tr>
 		<th><?=__('email')?>:</th>
-		<td><?=$hyphaUser->getAttribute('email')?></td>
+		<td><?=htmlspecialchars($hyphaUser->getAttribute('email'))?></td>
 		<th><?=__('interface-language')?>:</th>
-		<td><?=$hyphaUser->getAttribute('language')?></td>
+		<td><?=htmlspecialchars($hyphaUser->getAttribute('language'))?></td>
 	</tr>
 	<tr>
-		<td colspan="4"><input type="button" class="button edit" value="<?=__('edit')?>" onclick="hypha('settings/user/<?=$hyphaUser->getAttribute('username')?>', '', '');" /><input type="button" class="button quit" value="<?=__('quit')?>" onclick="hypha('settings/quit', '', '');" /></td>
+		<td colspan="4"><input type="button" class="button edit" value="<?=__('edit')?>" onclick="hypha(<?=htmlspecialchars(json_encode('settings/user/'.$hyphaUser->getAttribute('username')))?>, '', '');" /><input type="button" class="button quit" value="<?=__('quit')?>" onclick="hypha('settings/quit', '', '');" /></td>
 	</tr>
 </table>
 <h3><?=__('member-list')?></h3>
@@ -675,10 +675,10 @@
 				echo '<td>'.asterisk($user->getAttribute('rights')=='admin').'</td>';
 				if ($user->getAttribute('rights') == 'invite') echo '<td colspan="2" style="text-align:center;"><i>'.__('invitation-pending').'</i></td>';
 				elseif ($user->getAttribute('rights')!='exmember' || isAdmin()) {
-					echo '<td>'.$user->getAttribute('username').'</td>';
-					echo '<td>'.$user->getAttribute('fullname').'</td>';
+					echo '<td>'.htmlspecialchars($user->getAttribute('username')).'</td>';
+					echo '<td>'.htmlspecialchars($user->getAttribute('fullname')).'</td>';
 				}
-				echo '<td>'.$user->getAttribute('email').'</td>';
+				echo '<td>'.htmlspecialchars($user->getAttribute('email')).'</td>';
 				if (isAdmin()) {
 					echo '<td>';
 					if ($user->getAttribute('rights')!='exmember') {

Testing

• php -l on all changed files with PHP 8.2 and 8.5: no syntax errors.
• sanitizeHtml() on PHP 8.2 and 8.5 with sample input: onclick/onerror removed, javascript: links (also with mixed case and an embedded tab) lose their href, https:, relative, mailto: and hypha: links unchanged, data/images/... and data:image/png image sources kept, data:text/html removed, a YouTube iframe kept without its srcdoc, base, meta and svg removed, style="color:red" kept and styles with expression( or CSS escapes removed.
• HTMLForm::isValid() on PHP 8.5 with an editor field: event handler and javascript: link removed from dataFor(), hypha: link kept; a <script> still gives the existing validation error.
• User name check: alice, Jan Jansen, a.b-c_d accepted; names with a quote, <, /, \, & or a tab refused.
• To do on a test site: edit a text page with links, images and an embedded video and check that it looks the same after saving; open the page index, the settings member list and the account editor with a full name containing markup characters.

Combined test. The security patches of this series (all but three, which conflict with each other and say so) were also applied together on dev (5dae13a) plus the fixes of #405 and #406, and run in a php:8.2-apache container against a copy of a ~1,330-page data set: anonymous request of every page (1,333 requests) and a logged-in request of every page and its edit view; no failures and no new PHP warnings. php -l was clean on 8.2 and 8.5.

Internal reference: SEC-007