From 384715bbd43501bdb14a5848a460d018727de67f Mon Sep 17 00:00:00 2001 From: benakamoorthi Date: Wed, 5 Dec 2012 04:33:47 +0000 Subject: Refs #3456, add extra security for GeoIP auto-updater/downloader & fix a couple bugs in the code. Also made the UX a tiny bit more robust. git-svn-id: http://dev.piwik.org/svn/trunk@7576 59fd770c-687e-43c8-a1e3-f5a4ff64c105 --- plugins/UserCountry/GeoIPAutoUpdater.php | 98 ++++++++++++++++++++-- plugins/UserCountry/LocationProvider/GeoIp.php | 26 +++++- plugins/UserCountry/LocationProvider/GeoIp/Php.php | 44 +++++++++- plugins/UserCountry/templates/admin.js | 3 + plugins/UserCountry/templates/adminIndex.tpl | 1 + 5 files changed, 163 insertions(+), 9 deletions(-) (limited to 'plugins/UserCountry') diff --git a/plugins/UserCountry/GeoIPAutoUpdater.php b/plugins/UserCountry/GeoIPAutoUpdater.php index 1d87cd6436..aac72ec5bd 100755 --- a/plugins/UserCountry/GeoIPAutoUpdater.php +++ b/plugins/UserCountry/GeoIPAutoUpdater.php @@ -29,6 +29,14 @@ class Piwik_UserCountry_GeoIPAutoUpdater 'isp' => self::ISP_URL_OPTION_NAME, 'org' => self::ORG_URL_OPTION_NAME, ); + + /** + * PHP Error caught through a custom error handler while trying to use a downloaded + * GeoIP database. See catchGeoIPError for more info. + * + * @var array + */ + private static $unzipPhpError = null; /** * Attempts to download new location, ISP & organization GeoIP databases and @@ -122,7 +130,11 @@ class Piwik_UserCountry_GeoIPAutoUpdater public static function unzipDownloadedFile( $path, $unlink = false ) { $parts = explode('.', basename($path)); - $outputPath = Piwik_UserCountry_LocationProvider_GeoIp::getPathForGeoIpDatabase($parts[0].'.dat'); + $filenameStart = $parts[0]; + + $dbFilename = $filenameStart.'.dat'; + $tempFilename = $filenameStart.'.dat.new'; + $outputPath = Piwik_UserCountry_LocationProvider_GeoIp::getPathForGeoIpDatabase($tempFilename); // extract file if (substr($path, -7, 7) == '.tar.gz') @@ -141,7 +153,7 @@ class Piwik_UserCountry_GeoIPAutoUpdater foreach ($content as $info) { $archivedPath = $info['filename']; - if (basename($archivedPath) === basename($outputPath)) + if (basename($archivedPath) === $dbFilename) { $datFile = $archivedPath; } @@ -150,7 +162,7 @@ class Piwik_UserCountry_GeoIPAutoUpdater if ($datFile === null) { throw new Exception(Piwik_Translate('UserCountry_CannotFindGeoIPDatabaseInArchive', - array(basename($outputPath), "'$path'"))); + array($dbFilename, "'$path'"))); } // extract JUST the .dat file @@ -184,6 +196,59 @@ class Piwik_UserCountry_GeoIPAutoUpdater throw new Exception(Piwik_Translate('UserCountry_UnsupportedArchiveType', "'$ext'")); } + // test that the new archive is a valid GeoIP database + $dbType = Piwik_UserCountry_LocationProvider_GeoIp::getGeoIPDatabaseTypeFromFilename($dbFilename); + if ($dbType === false) // sanity check + { + throw new Exception("Unexpected GeoIP archive file name '$path'."); + } + + $customDbNames = array( + 'loc' => array(), + 'isp' => array(), + 'org' => array() + ); + $customDbNames[$dbType] = array($tempFilename); + + $phpProvider = new Piwik_UserCountry_LocationProvider_GeoIp_Php($customDbNames); + + // note: in most cases where this will fail, the error will usually be a PHP fatal error/notice. + // in order to delete the files in such a case (which can be caused by a man-in-the-middle attack) + // we need to catch them, so we set a new error handler. + self::$unzipPhpError = null; + set_error_handler(array('Piwik_UserCountry_GeoIPAutoUpdater', 'catchGeoIPError')); + + $location = $phpProvider->getLocation(array('ip' => Piwik_UserCountry_LocationProvider_GeoIp::TEST_IP)); + + restore_error_handler(); + + if (empty($location) + || self::$unzipPhpError !== null) + { + if (self::$unzipPhpError !== null) + { + list($errno, $errstr, $errfile, $errline) = self::$unzipPhpError; + Piwik::log("Piwik_UserCountry_GeoIPAutoUpdater: Encountered PHP error when testing newly downloaded". + " GeoIP database: $errno: $errstr on line $errline of $errfile."); + } + + // remove downloaded files in this case + unlink($outputPath); + unlink($path); + + throw new Exception(Piwik_Translate('UserCountry_ThisUrlIsNotAValidGeoIPDB')); + } + + // delete the existing GeoIP database (if any) and rename the downloaded file + $oldDbFile = Piwik_UserCountry_LocationProvider_GeoIp::getPathForGeoIpDatabase($dbFilename); + if (file_exists($oldDbFile)) + { + unlink($oldDbFile); + } + + $tempFile = Piwik_UserCountry_LocationProvider_GeoIp::getPathForGeoIpDatabase($tempFilename); + rename($existing = $tempFile, $newName = $oldDbFile); + // delete original archive if ($unlink) { @@ -251,7 +316,7 @@ class Piwik_UserCountry_GeoIPAutoUpdater // set url options foreach (self::$urlOptions as $optionKey => $optionName) { - if (empty($options[$optionKey])) + if (!isset($options[$optionKey])) { continue; } @@ -317,7 +382,7 @@ class Piwik_UserCountry_GeoIPAutoUpdater */ public static function getConfiguredUrl( $key ) { - if(!empty(self::$urlOptions[$key])) { + if(empty(self::$urlOptions[$key])) { throw new Exception("Invalid key $key"); } $url = Piwik_GetOption(self::$urlOptions[$key]); @@ -361,7 +426,7 @@ class Piwik_UserCountry_GeoIPAutoUpdater $result = array(); foreach (self::getConfiguredUrls() as $key => $url) { - if ($url !== false) + if (!empty($url)) { // if a database of the type does not exist, but there's a url to update, then // a database is missing @@ -398,4 +463,25 @@ class Piwik_UserCountry_GeoIPAutoUpdater return reset($filenameParts); } } + + /** + * Custom PHP error handler used to catch any PHP errors that occur when + * testing a downloaded GeoIP file. + * + * If we download a file that is supposed to be a GeoIP database, we need to make + * sure it is one. This is done simply by attempting to use it. If this fails, it + * will most of the time fail as a PHP error, which we catch w/ this function + * after it is passed to set_error_handler. + * + * The PHP error is stored in self::$unzipPhpError. + * + * @param int $errno + * @param string $errstr + * @param string $errfile + * @param int $errline + */ + public static function catchGeoIPError( $errno, $errstr, $errfile, $errline ) + { + self::$unzipPhpError = array($errno, $errstr, $errfile, $errline); + } } diff --git a/plugins/UserCountry/LocationProvider/GeoIp.php b/plugins/UserCountry/LocationProvider/GeoIp.php index 4cb0157169..d9e8f0224f 100755 --- a/plugins/UserCountry/LocationProvider/GeoIp.php +++ b/plugins/UserCountry/LocationProvider/GeoIp.php @@ -19,6 +19,7 @@ abstract class Piwik_UserCountry_LocationProvider_GeoIp extends Piwik_UserCountr { /* For testing, use: 'http://piwik-team.s3.amazonaws.com/GeoLiteCity.dat.gz' */ const GEO_LITE_URL = 'http://geolite.maxmind.com/download/geoip/database/GeoLiteCity.dat.gz'; + const TEST_IP = '194.57.91.215'; public static $geoIPDatabaseDir = 'misc'; @@ -215,7 +216,7 @@ abstract class Piwik_UserCountry_LocationProvider_GeoIp extends Piwik_UserCountr $expected = array(self::COUNTRY_CODE_KEY => 'FR', self::REGION_CODE_KEY => 'A6', self::CITY_NAME_KEY => 'Besançon'); - $result = array('194.57.91.215', $expected); + $result = array(self::TEST_IP, $expected); } return $result; } @@ -231,6 +232,29 @@ abstract class Piwik_UserCountry_LocationProvider_GeoIp extends Piwik_UserCountr || self::getPathToGeoIpDatabase(self::$dbNames['isp']) || self::getPathToGeoIpDatabase(self::$dbNames['org']); } + + /** + * Returns the type of GeoIP database ('loc', 'isp' or 'org') based on the + * filename (eg, 'GeoLiteCity.dat', 'GeoIPISP.dat', etc). + * + * @param string $filename + * @return string|false 'loc', 'isp', 'org', or false if cannot find a database + * type. + */ + public static function getGeoIPDatabaseTypeFromFilename( $filename ) + { + foreach (self::$dbNames as $key => $names) + { + foreach ($names as $name) + { + if ($name === $filename) + { + return $key; + } + } + } + return false; + } } diff --git a/plugins/UserCountry/LocationProvider/GeoIp/Php.php b/plugins/UserCountry/LocationProvider/GeoIp/Php.php index 63c9e1eb1a..3f1ea53f78 100755 --- a/plugins/UserCountry/LocationProvider/GeoIp/Php.php +++ b/plugins/UserCountry/LocationProvider/GeoIp/Php.php @@ -31,6 +31,46 @@ class Piwik_UserCountry_LocationProvider_GeoIp_Php extends Piwik_UserCountry_Loc */ private $geoIpCache = array(); + /** + * Possible filenames for each type of GeoIP database. When looking for a database + * file in the 'misc' subdirectory, files with these names will be looked for. + * + * This variable is an array mapping either the 'loc', 'isp' or 'org' strings with + * an array of filenames. + * + * By default, this will be set to Piwik_UserCountry_LocationProvider_GeoIp_Php::$dbNames. + * + * @var array + */ + private $customDbNames; + + /** + * Constructor. + * + * @param array|false $customDbNames The possible filenames for each type of GeoIP database. + * eg array( + * 'loc' => array('GeoLiteCity.dat'), + * 'isp' => array('GeoIP.dat', 'GeoIPISP.dat') + * 'org' => array('GeoIPOrg.dat') + * ) + * If a key is missing (or the parameter not supplied), then the + * default database names are used. + */ + public function __construct( $customDbNames = false ) + { + $this->customDbNames = parent::$dbNames; + if ($customDbNames !== false) + { + foreach ($this->customDbNames as $key => $names) + { + if (isset($customDbNames[$key])) + { + $this->customDbNames[$key] = $customDbNames[$key]; + } + } + } + } + /** * Closes all open geoip instances. */ @@ -140,7 +180,7 @@ class Piwik_UserCountry_LocationProvider_GeoIp_Php extends Piwik_UserCountry_Loc */ public function isAvailable() { - $path = self::getPathToGeoIpDatabase(parent::$dbNames['loc']); + $path = self::getPathToGeoIpDatabase($this->customDbNames['loc']); return $path !== false; } @@ -303,7 +343,7 @@ class Piwik_UserCountry_LocationProvider_GeoIp_Php extends Piwik_UserCountry_Loc parent::getRegionNames(); require_once PIWIK_INCLUDE_PATH . '/libs/MaxMindGeoIP/geoipcity.inc'; - $pathToDb = self::getPathToGeoIpDatabase(parent::$dbNames[$key]); + $pathToDb = self::getPathToGeoIpDatabase($this->customDbNames[$key]); if ($pathToDb !== false) { $this->geoIpCache[$key] = geoip_open($pathToDb, GEOIP_STANDARD); // TODO support shared memory diff --git a/plugins/UserCountry/templates/admin.js b/plugins/UserCountry/templates/admin.js index 1077039cba..fc5e71c762 100755 --- a/plugins/UserCountry/templates/admin.js +++ b/plugins/UserCountry/templates/admin.js @@ -102,6 +102,9 @@ $(document).ready(function() { } } }); + ajaxRequest.setErrorCallback(function() { + callback({error: _pk_translate('UserCountry_FatalErrorDuringDownload_js')}); + }); ajaxRequest.send(false); }; diff --git a/plugins/UserCountry/templates/adminIndex.tpl b/plugins/UserCountry/templates/adminIndex.tpl index ecf51a265d..7c5fe69131 100755 --- a/plugins/UserCountry/templates/adminIndex.tpl +++ b/plugins/UserCountry/templates/adminIndex.tpl @@ -1,3 +1,4 @@ +{loadJavascriptTranslations plugins='UserCountry'} {assign var=showSitesSelection value=false} {assign var=showPeriodSelection value=false} {include file="CoreAdminHome/templates/header.tpl"} -- cgit v1.2.3