XSS in action=info
Closed, ResolvedPublic

Description

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


Version: unspecified
Severity: normal

bzimport set Reference to bz63251.
csteipp created this task.Via LegacyMar 29 2014, 5:31 AM
csteipp added a comment.Via ConduitMar 29 2014, 5:48 AM

Created attachment 14969
Escape sortKey

This is patch suggested by Cindy in the report, and fixes this particular issue. Should be safe to deploy it as is.

attachment bug63251.patch ignored as obsolete

csteipp added a comment.Via ConduitMar 29 2014, 6:03 AM

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.

csteipp added a comment.Via ConduitMar 31 2014, 8:53 PM

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.

Krinkle added a comment.Via ConduitMar 31 2014, 9:18 PM

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.

csteipp added a comment.Via ConduitMar 31 2014, 11:49 PM

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.

csteipp added a comment.Via ConduitMar 31 2014, 11:53 PM

Created attachment 14987
Escape sortKey

Attached: bug63251b.patch

csteipp added a comment.Via ConduitApr 4 2014, 6:18 PM

Liangent just reported a bug with the current patch.

I'll get this updated to use the htmlspecialchars version today.

liangent added a comment.Via ConduitApr 4 2014, 6:44 PM

(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?

csteipp added a comment.Via ConduitApr 4 2014, 7:11 PM

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

liangent added a comment.Via ConduitApr 11 2014, 2:27 PM

wikitech wiki is still vulnerable.

We should really maintain a list of wikis to fix in case of security issues.

csteipp added a comment.Via ConduitApr 11 2014, 11:25 PM

Ops patched wikitech today. Thanks for pointing that out

cicalese added a comment.Via ConduitApr 14 2014, 6:04 PM

A CVE number for this bug has been reserved: CVE-2014-2853.

csteipp added a comment.Via ConduitApr 22 2014, 4:22 AM

Adding debian and wikia-- we're planning to include this fix in a tarball release this Thursday, April 24th.

Mglaser added a comment.Via ConduitApr 22 2014, 12:22 PM

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.

Grunny added a comment.Via ConduitApr 22 2014, 12:35 PM

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.

Aklapper added a comment.Via ConduitApr 29 2014, 2:36 PM

Anything left to do here, or all branches covered so this can be closed as RESOLVED FIXED?

csteipp added a project: Security.Via WebThu, Mar 26, 8:39 PM

Add Comment

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.