Wikimedia Bugzilla is closed!

Thanks for the report and research Michael M! I'm able to reproduce this on my dev instance.

Gergő, is this an area of code you can help write a patch for? I'm not sure what is supposed to be in the expected data that is loaded in (line 46), so I'm not sure if we can best sanitize that.

(In reply to Chris Steipp from comment #1)
> Gergő, is this an area of code you can help write a patch for? I'm not sure
> what is supposed to be in the expected data that is loaded in (line 46), so
> I'm not sure if we can best sanitize that.

I'll check. The data contains a HTML snippet with preview of the current/prev/next page and associated links, I don't think it can be safely sanitized. The fix suggested by Michael sounds OK for Wikimedia sites; I'm not sure "links cannot have custom classes" is an assumption we would want to base an XSS protection on. Sanitizing the URL is another possibility, although it might conflict with nice URL schemes.

Created attachment 15670 [details]
quick hack to close the XSS vulnerability, at least with WMF-like config

The relevant parts for creating the HTML source for the prev/next page thumbnails are ImagePage::openShowImage lines 470/490. Adding a class to the links in the thumbnail captions is easy, but there doesn't seem to be any sane way to add a class or other markup to the links which wrap the thumbnail images. Attached a patch which uses an ugly workaround to get around this.

A nicer and more generic solution could be to dump an array of safe URLs into the page (as a JS variable), and use that to verify that the link has not been tempered with.

What about building the link in the JS code instead of retrieving it from the HTML?

safeHref = mw.util.getUrl( mw.config.get( 'wgPageName' ), {
 page: mw.util.getParamValue( 'page', this.href )
} );

"page" seems to be the only parameter, and any url with only "page" as parameter should be safe, even if a user manages to sneak an invalid value for page in.

I like how simple Michael M's solution is, and it seems like that will minimize the possible confusion. I also added a conversion to int, just to clarify that's what we expect (and worst case it passes page=NaN in the url).

$( '.multipageimagenavbox' ).one( 'click', 'a', function ( e ) {
	loadPage( mw.util.getUrl( mw.config.get( 'wgPageName' ), {
		page: +( mw.util.getParamValue( 'page', this.href ) )
	} ) );
	e.preventDefault();
} );

This looks like it works for my simple test cases, is commons doing anything odd that might have relied on the old functionality?

This will result in a different URL than the one used in the <a> tag, and I am not sure it will work with sufficiently funny file names. Compare:

2106643628).jpg?action=raw">https://commons.wikimedia.org/wiki/File:%3F_and_Michael_(2106643628).jpg?action=raw
2106643628).jpg&action=raw">https://commons.wikimedia.org/w/index.php?title=File:%3F_and_Michael_(2106643628).jpg&action=raw

Something like

loadPage( mw.config.get( 'wgScript' ) + '?' +  $.param( {
    title: mw.config.get( 'wgPageName' ),
    page: +( mw.util.getParamValue( 'page', this.href ) )
} ) );

would work though.

Let me try that again: http://pastebin.com/raw.php?i=K9x9FaYp

I'm not sure how this vulnerability would be make possible.


Though I think the mediawiki.page.image.pagination.js module is not up to our standards and merged too early, it doesn't interpret text or wikitext as HTML or anything. It is only parsing as HTML was our servers are serving as HTML in the first place.

Since it only loads content from our own domain, a third party url has no influence, so the snippet with <img onerror> would have to be on our own domain. The writer suggests on a subpage "/x" of the file description page. Presumably as wikitext.

Well, then there is your problem and solution.

1) We don't allow <img> in the wikitext anyway. If you do, it'll just render as plain text. No problem here.

2) If a third-party wiki has chosen to configure their wiki and allow <img> tags the Sanitizer still doens't allow inline javascript event handlers (of obvious reasons), so "onerror" would just be stripped far before mediawiki.page.image.pagination.js comes into play.

Ah, I see now it is using action=raw (which serves the unparsed wiki text as plain text, not html). So another thing we should do better in general is honour the Content-Type header of our server and not unconditionally parse it as HTML.

$.ajax /X?action=raw
getAllResponseHeaders()
 - Content-Type: text/x-wiki; charset=UTF-8


And yeah, I agree the url shouldn't be retrieved in this fashion (unless the css selector is restricted to scope outside page content). I'd recommend either parsing the url and extracting the 'page' parameter only, or using data attributes.

(In reply to Tisza Gergő from comment #8)
> Let me try that again: http://pastebin.com/raw.php?i=K9x9FaYp

(In reply to Tisza Gergő from comment #7)
> Something like
> 
> loadPage( mw.config.get( 'wgScript' ) + '?' +  $.param( {
>     title: mw.config.get( 'wgPageName' ),
>     page: +( mw.util.getParamValue( 'page', this.href ) )
> } ) );
> 
> would work though.

Note that with $.param() it does not use the appropriate url encoding for the page title, as a result the URLs will look ugly and not match the canonical url.


url = new mw.Uri( mw.util.wikiScript() )
  .extend({ title: mw.config.get( 'wgPageName' ), page: 2 })
  .toString();

> "https://commons.wikimedia.org/w/index.php?title=File:Example.jpg&page=2"

url = mw.util.wikiScript() + '?' + $.param({
   title: mw.config.get( 'wgPageName' ),
   page: 2
  });

> "/w/index.php?title=File%3AExample.jpg&page=2"

url = mw.util.getUrl(mw.config.get( 'wgPageName' ), {
   page: 2
  });

> "/wiki/File:Example.jpg?page=2"

Notice the garbled colon with '%3A'. In foreign languages and more complex page names this gets exponentially worse.

It works as a quick hack, but it should never pass code review. To make it match server-side behaviour I'd recommend using mw.Uri (be sure to add "dependency => mediawiki.Uri" for this module, ResourceLoader takes care of the rest).

You may also want to add a comment pointing to ImagePage::openShowImage saying this should be kept in sync with that (and visa versa) since we're not hardcoding something we kinda shouldn't.

Created attachment 15676 [details]
Generate URL on the client side

New patch based on Krinkle's suggestions.

Thanks Gergo, that patch works looks good to me. Krinkle, if that seems ok to you, I can deploy it this afternoon.

Looks good in principle, though one important and one less important point:

diff --git a/resources/Resources.php b/resources/Resources.php
index ea7d397..230a652 100644
@@ -1220,7 +1220,10 @@ return array(
 	),
 	'mediawiki.page.image.pagination' => array(
 		'scripts' => 'resources/src/mediawiki.page/mediawiki.page.image.pagination.js',
-		'dependencies' => array( 'jquery.spinner' )
+		'dependencies' => array(
+			'mediawiki.Uri',
+			'jquery.spinner',
+		)
 	),
 

This also needs a dependency on mediawiki.util since the code introduces a call to mw.util.getParamValue

diff --git a/resources/src/mediawiki.page/mediawiki.page.image.pagination.js b/resources/src/mediawiki.page/mediawiki.page.image.pagination.js
index 4819be0..1c1feed 100644
+			page = +( mw.util.getParamValue( 'page', this.href ) );

Coding style, use the Number() number to explicitly cast to a number (currently our minifier keeps this, but UglifyJS minifies it down to +).

--

Created attachment 15758 [details]
Generate URL on the client side

Rebased patch and fixed the issues mentioned by Timo.

After Timo's refactoring of mediawiki.page.image.pagination.js, the original exploit does not work anymore. To reproduce, use this code for step 2:

{| class="multipageimage"
<div class="multipageimagenavbox">[{{fullurl:File:Carroll.pdf/x|action=raw}} Click me!]</div>
|}

Created attachment 16085 [details]
Backport to REL1_23

This is a backport to REL1_23. If someone could "+2", I'd be very thankful.

Created attachment 16086 [details]
Backport to REL1_22

This is a backport to REL1_23. If someone could "+2", I'd be very thankful.

In REL1_19, there's no mediawiki.page.image.pagination.js. I assume, a backport is not neccessary, right?

It was introduced by Icd1cde7c62c4d462f5b697b9f49f5c08f6e7482b (1.22wmf14), so 1.19 isn't affected.

Adding Wikia and debian.

(In reply to Chris Steipp from comment #18)
> Adding Wikia and debian.

Markus is planning to release this in the next security release (probably tomorrow, July 30th).

Both backports fix the issue on my dev instances, and don't seem to be causing issues. +2.

Moved to MediaWiki product as fix is now public.

This was assigned CVE-2014-5242

Fix included in 1.23.2 and 1.22.9 tarballs; fixed in https://git.wikimedia.org/commit/mediawiki%2Fcore.git/410d33d36e9a4fe28f54aa4347310c52762b6e5e

Hence closing ticket as FIXED (but please reopen if I missed something).