r75222 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r75221‎ | r75222 | r75223 >
Date:23:51, 22 October 2010
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
fix for Bug 25564 (won't fix stats for meta), to test on Monday
Modified paths:
  • /trunk/extensions/CentralNotice/CentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/SpecialBannerController.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/CentralNotice.php
@@ -145,7 +145,7 @@
146146 global $wgUser, $wgOut, $wgCentralDBname, $wgScript;
147147
148148 // Include '.js' to exempt script from squid cache override
149 - $centralLoader = $wgScript . '?title=' . SpecialPage::getTitleFor( 'BannerController' ) . '&cn.js';
 149+ $centralLoader = $wgScript . '?title=' . SpecialPage::getTitleFor( 'BannerController' ) . '&cache=cn.js';
150150
151151 // Insert the banner controller Javascript into the <head>
152152 $wgOut->addScriptFile( $centralLoader );
Index: trunk/extensions/CentralNotice/SpecialBannerController.php
@@ -69,9 +69,8 @@
7070 } else {
7171 var geoLocation = Geo.country; // pull the geo info
7272 }
73 - var bannerListQuery = $.param( { 'language': wgContentLanguage, 'project': wgNoticeProject, 'country': geoLocation } );
74 - var bannerListPage = 'Special:BannerListLoader?' + bannerListQuery;
75 - bannerListURL = wgArticlePath.replace( '$1', bannerListPage );
 73+ var bannerListQuery = $.param( { 'language': wgContentLanguage, 'project': wgNoticeProject, 'country': geoLocation, 'cache': 'cn.js' } );
 74+ bannerListURL = wgScript + '?title=' + wgFormattedNamespaces[-1] + ':BannerListLoader&' + bannerListQuery;
7675 var request = $.ajax( {
7776 url: bannerListURL,
7877 dataType: 'json',

Follow-up revisions

RevisionCommit summaryAuthorDate
r75370fixes for r75222 and r75181kaldari17:53, 25 October 2010
r75388follow up to r75222 - need colon and slash unescapedkaldari22:40, 25 October 2010
r75409follow up to r75222, using encodeURIComponent() and adding missing ampersandkaldari03:20, 26 October 2010

Comments

#Comment by Catrope (talk | contribs)   13:59, 23 October 2010
+				var bannerListQuery = $.param( { 'language': wgContentLanguage, 'project': wgNoticeProject, 'country': geoLocation, 'cache': 'cn.js' } );
+				bannerListURL = wgScript + '?title=' + wgFormattedNamespaces[-1] + ':BannerListLoader&' + bannerListQuery;

Just put the title parameter in the $.param() call. This is embedding stuff in the URL unsafely again (that's why I introduced the param() call in the first place).

#Comment by Kaldari (talk | contribs)   18:44, 25 October 2010

Moved the title to param() in r75370. Is there still any vulnerability? I guess I'm not sure I understand what the original vulnerability is. Any additional explanation would be appreciated.

#Comment by Kaldari (talk | contribs)   21:56, 25 October 2010

Actually, I'd like to revert this back to how I had it and also move the cache part out of param() since it needs a slash and param() escapes colons and slashes. Normally it wouldn't be a problem, but the regex is looking for the exact character, not the escaped version. Thoughts?

#Comment by Kaldari (talk | contribs)   22:46, 25 October 2010

Went ahead and reverted for now (r75388) so that I can have the colon and slash unescaped. Not pushing out, however, until I hear back from you. If there is a vulnerability, let me know, but I'm not sure I see what you're talking about. Of course this is probably just my ignorance. If there is a vulnerability, let me know if there is a way to pass parameters using param() without it escaping the slash. Otherwise, I'll need to get Mark to modify the regex. Thanks!

#Comment by Catrope (talk | contribs)   19:34, 26 October 2010

Having the namespace unescaped is probably fine.

The vulnerability was caused by your code pulling things from the URL and using them to build another URL without escaping them first. This caused a URL like http://en.wikipedia.org/wiki/Main_Page?banner=hello";arbitraryCodeHere(); to execute the injected JS. (This problem wasn't here but elsewhere in the file; I converted both instances to $.param() URL building for good measure.)

Status & tagging log