Last modified: 2014-04-29 15:26:42 UTC
Reported to security@, confirmed in master. The fix in the report fixes this particular issue, although I'm suspicious that we'll find another way to inject javascript in the array returned from pageInfo(). >>>> I have found an XSS vulnerability in the MediaWiki info action. If the default sort key is set to a string containing a script, the script will be executed when the page is viewed using the info action. For example, if the wikitext {{DEFAULTSORT:<script>alert("hi");</script>}} is included on a page and then the page is views with action=info (accessible using the “Page information” link in the Tools sidebar), the code will be executed displaying an alert box. This can be reproduced by creating a page named Test containing only the above text and then viewing the page with the URL http://url.to.wiki/index.php?title=Test&action=info. The vulnerability can be remediated by adding one line of code: $sortKey = htmlentities($sortKey, ENT_QUOTES); to the file mediawiki/includes/actions/InfoAction.php before line 265 (line number according to http://git.wikimedia.org/blob/mediawiki%2Fcore.git/dd3de3dbb71f09fca8a97642626e3c84d562d8f2/includes%2Factions%2FInfoAction.php) yielding … // Default sort key $sortKey = $title->getCategorySortkey(); if ( !empty( $pageProperties['defaultsort'] ) ) { $sortKey = $pageProperties['defaultsort']; } $sortKey = htmlentities($sortKey, ENT_QUOTES); $pageInfo['header-basic'][] = array( $this->msg( 'pageinfo-default-sort' ), $sortKey ); … __________________________ Dr. Cindy Cicalese Lead Software Systems Engineer The MITRE Corporation
Created attachment 14969 [details] Escape sortKey This is patch suggested by Cindy in the report, and fixes this particular issue. Should be safe to deploy it as is.
Since the patch was simple enough and low risk, I've gone ahead and deployed it to both wmf19 and wmf20. I'll take another look through that info action code on Monday and see if there are any other injections we need to fix.
It looks like sortKey was the only unescaped value from pageInfo(), so the current patch should fix the whole problem. Markus, can we add this into the next tarball release? It seems like onView or addRow should be aware if the values are properly escaped, and escape them if not instead of relying on pageInfo to return everything correctly escaped, but that type of refactor is probably better done in a public change after this is public.
htmlentities() seems like overkill (encodes way more than necessary, bloating the html, we output UTF-8 and stuff, most of it can and should go straight through), plus it has the downside of needing a bitflag to encode both quotes (easy to mess up or confuse). We generally use the more performant and lighweight htmlspecialchars() which in turn does encode both quotes by default. The original submitter presumably suggested htmlentities because they are not familiar with our coding conventions.
Yeah, I forgot I was going to update that. htmlspecialchars() without ENT_QUOTES is fine for mediawiki, since this is being written into the html body. It doesn't encode single quotes, but we don't need to in this context.
Created attachment 14987 [details] Escape sortKey
Liangent just reported a bug with the current patch. I'll get this updated to use the htmlspecialchars version today.
(In reply to Chris Steipp from comment #3) > It seems like onView or addRow should be aware if the values are properly > escaped, and escape them if not instead of relying on pageInfo to return > everything correctly escaped, but that type of refactor is probably better > done in a public change after this is public. The problem should be ... displaytitle is stored partly escaped (ie. <script>-escaped but <b>-preserved) and we want it outputted as-is, so we can't simply escape everything again ... Anyway is storing an escaped version a good idea?
Yeah, having display title partly escaped (I'm pretty sure it's parsed wikitext) makes it difficult to deal with-- it's safe to output into the html body, but not into an html attribute. So no, I'm not a fan of storing things "escaped". If we follow "Escape as close to the output as possible", then we really should wait to escape the output of pageInfo() in addRow(), or probably the caller of addRow() which is onView(). But again, we can do that patch publicly in gerrit after this is released. Oh, and the htmlspecialchars patch was deployed today: 18:43 csteipp: redeployed updated patch for bug63251 to fix a reported bug
wikitech wiki is still vulnerable. We should really maintain a list of wikis to fix in case of security issues.
Ops patched wikitech today. Thanks for pointing that out
A CVE number for this bug has been reserved: CVE-2014-2853.
Adding debian and wikia-- we're planning to include this fix in a tarball release this Thursday, April 24th.
The patch applies nicely to REL1_21 and REL1_22. I can confirm the described XSS is fixed by the patch on those releases. On REL1_19, afaik there's no action=info, so I assume, the vulnerability is also not present there. Can someone confirm this? One further notice: we must not forget to patch the yet unreleased REL1_23 on Thursday as well.
Yeah, this shouldn't affect MW 1.19. MW 1.19 does have action=info: https://git.wikimedia.org/blob/mediawiki%2Fcore.git/REL1_19/includes%2Factions%2FInfoAction.php But, it's disabled by default with [[mw:Manual:$wgAllowPageInfo]] for performance reasons, and it doesn't have the sortkey part that is vulnerable anyway, only info on page views, watchers, etc. The rest was added later as part of bug 38450.
Fixed relased in https://github.com/wikimedia/mediawiki-core/commit/0b695ae09aada343ab59be4a3c9963995a1143b6
Anything left to do here, or all branches covered so this can be closed as RESOLVED FIXED?
