If you create an account with some HTML in its name (which is not allowed by default, see T308465), after the account creation when it sets the page title to "Welcome, [username]" (using the welcomeuser), the username is not escaped: SpecialCreateAccount::successfulAction() calls ::showSuccessPage() with a message as second parameter, and OutputPage::setPageTitle() uses text().
Filing as a security task out of an abundance of caution.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Reedy | T305199 Release MediaWiki 1.35.7/1.37.3/1.38.2 | |||
| Resolved | Reedy | T305200 Tracking bug for MediaWiki 1.35.7/1.37.3/1.38.2 | |||
| Open | None | T2212 Some MediaWiki: messages not safe in HTML (tracking) | |||
| Resolved | Security | sbassett | T308471 CVE-2022-34911: Username is not escaped in the "welcomeuser" message |
Untested patch that should fix the issue:
Change 805208 had a related patch set uploaded (by SBassett; author: SBassett):
[mediawiki/core@master] SECURITY: Escape welcomeuser message passed to showSuccessPage()
For a lot of common XSS vectors, sure. But it'll allow lower-level obfuscations and similar, minor CSS hacks, like: <span style=display:none>username</span>. I'm also going to guess that this bug was possibly found via phan-taint-check-plugin which I'm not certain adjusts any taints for Sanitizer::removeSomeTags. Which are all reasons why this is a very low-risk bug and really mostly code-hardening.
Change 805208 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Escape welcomeuser message passed to showSuccessPage()
Yeah, better be safe. FWIW, "<" is not allowed in usernames unless you change the config, and ">" is forbidden regardless of config because it's reserved for external usernames.
I'm also going to guess that this bug was possibly found via phan-taint-check-plugin which I'm not certain adjusts any taints for Sanitizer::removeSomeTags.
Actually... I created an account with "<" in its name for unrelated tests, and though I'd explore the interface a bit just to make sure that there was no obvious XSS around.
Change 807005 had a related patch set uploaded (by SBassett; author: SBassett):
[mediawiki/core@REL1_38] SECURITY: Escape welcomeuser message passed to showSuccessPage()
Change 807146 had a related patch set uploaded (by SBassett; author: SBassett):
[mediawiki/core@REL1_37] SECURITY: Escape welcomeuser message passed to showSuccessPage()
Change 807147 had a related patch set uploaded (by SBassett; author: SBassett):
[mediawiki/core@REL1_35] SECURITY: Escape welcomeuser message passed to showSuccessPage()
This picked cleanly to all supported release branches. If those test fine, I'll plan to merge them and this task can be resolved.
Change 807147 merged by jenkins-bot:
[mediawiki/core@REL1_35] SECURITY: Escape welcomeuser message passed to showSuccessPage()
Change 807146 merged by jenkins-bot:
[mediawiki/core@REL1_37] SECURITY: Escape welcomeuser message passed to showSuccessPage()
Change 807005 merged by jenkins-bot:
[mediawiki/core@REL1_38] SECURITY: Escape welcomeuser message passed to showSuccessPage()
I'm contemplating changing the first parameter of LoginSignupSpecialPage::showSuccessPage() from string|Message to just plain Message, at least in part to improve phan-taint-check's accuracy: T343849. I could use some help understanding the impact of this bug, however (and some similar code in SpecialContributions). It seems to be that either you're double-escaping here (maybe deliberately?) or we should be using something other than Message::text() when OutputPage::setPageTitle is given a Message as its argument.
IIRC, this was due to some oddities in the way Sanitizer::removeSomeTags and Sanitizer::stripAllTags can interfere with each other, e.g.
> Sanitizer::stripAllTags( Sanitizer::removeSomeTags( "<script>user</script>" ) ); = "<script>user</script>"
along with this warning for Sanitizer::stripAllTags and that OutputPage->setHTMLTitle assumes htmlspecialchars will be called at some later point. Though @Daimona might have more input on his original findings.
Oh, wow, that sure /looks/ like a bug in ::stripAllTags(), but in fact is exactly as it is documented (as you linked):
/**
* Take a fragment of (potentially invalid) HTML and return
* a version with any tags removed, encoded as plain text.
*
* Warning: this return value must be further escaped for literal
* inclusion in HTML output as of 1.10!
*
* @param string $html HTML fragment
* @return string
* @return-taint tainted
*/
public static function stripAllTags( $html ) {But I totally agree this is a gotcha "candy machine" interface, since the assumption seems to be that the result is safe to interpolate into HTML. Sanitizer::removeSomeTags() *is* always safe to interpolate into HTML.
I think the taint annotations on both Sanitizer::removeSomeTags() as well as Sanitizer::stripAllTags() are correct, though.
I'm not touching OutputPage::setHTMLTitle() just to try to contain scope but it has a similar string|Message issue; the issue you note would probably also be remedied by storing it explicitly as a Message, never a string -- basically this lets the value carry its type (escaped or unescaped) along with it.
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/947003 is the patch containing similar code in SpecialContributions which looks (to me) to be a double-escape situation.
In any case, I'm hoping that by collapsing string|Message down to just Message we can reason about the various possibilities better.
Change 947935 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):
[mediawiki/core@master] LoginSignupSpecialPage::showSuccessPage(): only allow Message as 1st param
Change 947935 merged by jenkins-bot:
[mediawiki/core@master] LoginSignupSpecialPage::showSuccessPage(): only allow Message as 1st param