r65864 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r65863‎ | r65864 | r65865 >
Date:16:01, 3 May 2010
Author:avar
Status:resolved (Comments)
Tags:
Comment:
Warn if we *can* get data from mediawiki.org but the returned data is bad

Catches cases where we're installing behind evil man in the middle
wireless networks which violate the end-to-end model.
Modified paths:
  • /branches/new-installer/phase3/includes/installer/Installer.i18n.php (modified) (history)
  • /branches/new-installer/phase3/includes/installer/Installer.php (modified) (history)

Diff [purge]

Index: branches/new-installer/phase3/includes/installer/Installer.i18n.php
@@ -71,6 +71,7 @@
7272 'config-env-latest-ok' => 'You are installing the latest version of Mediawiki.',
7373 'config-env-latest-new' => "'''Note:''' You are installing a development version of Mediawiki.",
7474 'config-env-latest-can-not-check' => "'''Note:''' We were unable to retrieve information about the latest MediaWiki release (from [$1]).",
 75+ 'config-env-latest-data-invalid' => "'''Warning:''' When trying to check if this version was outdated we got invalid data from [$1].",
7576 'config-env-latest-old' => "'''Warning:''' You are installing an outdated version of Mediawiki.",
7677 'config-env-latest-help' => 'You are installing version $1, but the latest version is $2.
7778 You are advised to use the latest release, which can be downloaded from [http://www.mediawiki.org/wiki/Download mediawiki.org]',
Index: branches/new-installer/phase3/includes/installer/Installer.php
@@ -361,6 +361,12 @@
362362 return;
363363 }
364364 $latestInfo = unserialize($latestInfo);
 365+ if ($latestInfo === false || !isset( $latestInfo['mwreleases'] ) ) {
 366+ # For when the request is successful but there's e.g. some silly man in
 367+ # the middle firewall blocking us, e.g. one of those annoying airport ones
 368+ $this->showMessage( 'config-env-latest-data-invalid', $latestInfoUrl );
 369+ return;
 370+ }
365371 foreach( $latestInfo['mwreleases'] as $rel ) {
366372 if( isset( $rel['current'] ) )
367373 $currentVersion = $rel['version'];

Follow-up revisions

RevisionCommit summaryAuthorDate
r65900Follow up r65864 comments. Changing unserialize() to Json::decode().platonides15:26, 4 May 2010

Comments

#Comment by Platonides (talk | contribs)   19:55, 3 May 2010

I wouldn't really use unserialize there. If there really was some man in the middle you are providing him a code injection.

#Comment by Catrope (talk | contribs)   16:58, 4 May 2010

unserialize doesn't actually allow code injection. It's only scary if you've got classes with magic methods like __wakeup() and __destruct() lying around: you could serialize an instance of such a class with malicious values for its attributes, which a magic method would then do something interesting with (like write to a file or whatever). These methods are not used in MediaWiki.

So while unserialize is a bit scary and JSON is safer, it doesn't trigger any code execution by itself, only in combination with poorly-secured magic methods. It definitely doesn't allow code injection, unless you've got a really scary magic method lying around.

#Comment by Ævar Arnfjörð Bjarmason (talk | contribs)   19:17, 4 May 2010

I thought that was stupid too. But I was offline when I wrote this so I couldn't check if the server supported something else. Thanks for adding JSON support. That's much better.

#Comment by Bryan (talk | contribs)   14:53, 4 May 2010

I agree, Json::decode is much safer.

Status & tagging log