r61286 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r61285‎ | r61286 | r61287 >
Date:02:11, 20 January 2010
Author:jeroendedauw
Status:deferred (Comments)
Tags:
Comment:
Changes for 0.5.3.
* Google Maps v3 support
Modified paths:
  • /trunk/extensions/Maps/GoogleMaps3/Maps_GoogleMaps3.php (modified) (history)
  • /trunk/extensions/Maps/Maps_Settings.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Maps/GoogleMaps3/Maps_GoogleMaps3.php
@@ -22,11 +22,11 @@
2323
2424 $egMapsServices['googlemaps3'] = array(
2525 'pf' => array(
26 - 'display_point' => array('class' => 'MapsGoogleMaps3DispPoint', 'file' => 'GoogleMaps/Maps_GoogleMaps3DispPoint.php', 'local' => true),
27 - 'display_map' => array('class' => 'MapsGoogleMaps3DispMap', 'file' => 'GoogleMaps/Maps_GoogleMaps3DispMap.php', 'local' => true),
 26+ 'display_point' => array('class' => 'MapsGoogleMaps3DispPoint', 'file' => 'GoogleMaps3/Maps_GoogleMaps3DispPoint.php', 'local' => true),
 27+ 'display_map' => array('class' => 'MapsGoogleMaps3DispMap', 'file' => 'GoogleMaps3/Maps_GoogleMaps3DispMap.php', 'local' => true),
2828 ),
2929 'classes' => array(
30 - array('class' => 'MapsGoogleMaps3', 'file' => 'GoogleMaps/Maps_GoogleMaps3.php', 'local' => true)
 30+ array('class' => 'MapsGoogleMaps3', 'file' => 'GoogleMaps3/Maps_GoogleMaps3.php', 'local' => true)
3131 ),
3232 'aliases' => array('google3', 'googlemap3', 'gmap3', 'gmaps3'),
3333 );
@@ -38,7 +38,7 @@
3939 *
4040 * @author Jeroen De Dauw
4141 */
42 -class MapsGoogleMaps {
 42+class MapsGoogleMaps3 {
4343
4444 const SERVICE_NAME = 'googlemaps3';
4545
Index: trunk/extensions/Maps/Maps_Settings.php
@@ -62,13 +62,14 @@
6363 # Include the mapping services that should be loaded into Maps.
6464 # Commenting or removing a mapping service will cause Maps to completely ignore it, and so improve performance.
6565 include_once $egMapsIP . '/GoogleMaps/Maps_GoogleMaps.php'; // Google Maps
 66+include_once $egMapsIP . '/GoogleMaps3/Maps_GoogleMaps3.php'; // Google Maps v3
6667 include_once $egMapsIP . '/OpenLayers/Maps_OpenLayers.php'; // OpenLayers
6768 include_once $egMapsIP . '/YahooMaps/Maps_YahooMaps.php'; // Yahoo! Maps
6869 include_once $egMapsIP . '/OpenStreetMap/Maps_OSM.php'; // OpenLayers optimized for OSM
6970
7071 # Array of String. Array containing all the mapping services that will be made available to the user.
7172 # Currently Maps provides the following services: googlemaps, yahoomaps, openlayers
72 -$egMapsAvailableServices = array('googlemaps', 'yahoomaps', 'openlayers', 'osm');
 73+$egMapsAvailableServices = array('googlemaps', 'googlemaps3', 'yahoomaps', 'openlayers', 'osm');
7374
7475 # String. The default mapping service, which will be used when no default service is present in the
7576 # $egMapsDefaultServices array for a certain feature. A service that supports all features is recommended.
@@ -149,7 +150,7 @@
150151
151152 # Specific map properties configuration
152153
153 -# Google maps
 154+# Google Maps
154155
155156 # Your Google Maps API key. Required for displaying Google Maps, and using the Google Geocoder services.
156157 if (empty($egGoogleMapsKey)) $egGoogleMapsKey = ''; # http://code.google.com/apis/maps/signup.html
@@ -181,8 +182,18 @@
182183
183184
184185
185 -# Yahoo maps
 186+# Google Maps v3
186187
 188+# String. The Google Maps v3 map name prefix. It can not be identical to the one of another mapping service.
 189+$egMapsGoogleMaps3Prefix = 'map_google3';
 190+
 191+# Integer. The default zoom of a map. This value will only be used when the user does not provide one.
 192+$egMapsGoogleMaps3Zoom = 14;
 193+
 194+
 195+
 196+# Yahoo! Maps
 197+
187198 # Your Yahoo! Maps API key. Required for displaying Yahoo! Maps.
188199 # Haven't got an API key yet? Get it here: https://developer.yahoo.com/wsregapp/
189200 if (empty($egYahooMapsKey)) $egYahooMapsKey = '';
@@ -227,7 +238,7 @@
228239
229240
230241
231 -# OpenStreetMaps (OpenLayers optimized for OSM)
 242+# OpenStreetMap (OpenLayers optimized for OSM)
232243
233244 # String. The OSM map name prefix. It can not be identical to the one of another mapping service.
234245 $egMapsOSMPrefix = 'map_osm';

Comments

#Comment by Nikerabbit (talk | contribs)   07:29, 20 January 2010

if (empty($egYahooMapsKey)) $egYahooMapsKey = ;

That looks dangerous. Why is that check there?

Also, the indentation in Maps_GoogleMaps3.php is inconsistent.

#Comment by Jeroen De Dauw (talk | contribs)   15:47, 20 January 2010

Why is that check there? -> To not override anything that might already have been set.

the indentation in Maps_GoogleMaps3.php is inconsistent. -> Maps_GoogleMaps3.php is not finished yet. Same goes for the other GoogleMaps3 files.

#Comment by Platonides (talk | contribs)   20:44, 20 January 2010

Then if the user doesn't set a $egYahooMapsKey, an attacker could set it to whaever he wants if the system has register_globals enabled, which could be an attack vector (in your case it really isn't since $egYahooMapsKey isn't used yet, but might be in the future). Users should only set $egYahooMapsKey below requiring the extension file.

#Comment by Jeroen De Dauw (talk | contribs)   23:58, 20 January 2010

What part of "if (empty($egYahooMapsKey)) $egYahooMapsKey = ;" is dangerous, and could be exploited when register_globals is enabled?

#Comment by Platonides (talk | contribs)   00:49, 21 January 2010

Suppose you use $egYahooMapsKey as:

if (empty($egYahooMapsKey)) $egYahooMapsKey = '';
echo "<script>var mykey='$egYahooMapsKey';</script>";


If the user sets $egYahooMapsKey above that, nothing happens.

However, if the user doesn't set it, or the attacker gets a different entry point from which it isn't set (eg. if the MEDIAWIKI guard wasn't there), he could call the script as index.php?egYahooMapsKey=';%20while(1)alert('Vulnerable_page


Since register_globals is enabled (which shouldn't) the variable $egYahooMapsKey gets defined, and your output is the one determined by the attacker. In this sample, it is an inocuous payload which continously show a message in the browser.


This is called a XSS vulnerability.

It can be prevented by initialising all variables before using them. Thus, we set a default on the extension files and let the user override them by setting a different value later.

#Comment by Jeroen De Dauw (talk | contribs)   01:30, 21 January 2010

Thanks, that's clear. I'll fix this, and check for similar code in both mapping extensions :)

Status & tagging log