Merge pull request #10 from phpbb/ticket/security-171

[ticket/security-171] Use type cast helper for json data
author: Joas Schilling <nickvergessen@gmx.de> 2014-11-22 17:48:37 +0100
committer: Joas Schilling <nickvergessen@gmx.de> 2014-11-22 17:48:37 +0100
commit: 5c3b6a855944d7a370186987ebebe3a896cf76a2 (patch)
tree: a4e50394f86019133c053e3dd0339b291c6a5bae
parent: a3b18d87ddef00424a2dc35061780e1a7dc2c0c6 (diff)
parent: 0f06b88ee7aa508ea44704f2973ce1e57f9accf2 (diff)
download: forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.tar
forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.tar.gz
forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.tar.bz2
forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.tar.xz
forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.zip
6 files changed, 236 insertions, 20 deletions
diff --git a/phpBB/phpbb/extension/metadata_manager.php b/phpBB/phpbb/extension/metadata_manager.php
index edca8ee1af..a64d88fe39 100644
--- a/phpBB/phpbb/extension/metadata_manager.php
+++ b/phpBB/phpbb/extension/metadata_manager.php
@@ -177,6 +177,7 @@ class metadata_manager
 				throw new \phpbb\extension\exception($this->user->lang('FILE_JSON_DECODE_ERR', $this->metadata_file));
 			}
 
+			array_walk_recursive($metadata, array($this, 'sanitize_json'));
 			$this->metadata = $metadata;
 
 			return true;
@@ -184,6 +185,17 @@ class metadata_manager
 	}
 
 	/**
+	 * Sanitize input from JSON array using htmlspecialchars()
+	 *
+	 * @param mixed		$value	Value of array row
+	 * @param string	$key	Key of array row
+	 */
+	public function sanitize_json(&$value, $key)
+	{
+		$value = htmlspecialchars($value);
+	}
+
+	/**
 	* This array handles the cleaning of the array
 	*
 	* @return array Contains the cleaned metadata array
@@ -337,30 +349,30 @@ class metadata_manager
 	public function output_template_data()
 	{
 		$this->template->assign_vars(array(
-			'META_NAME'			=> htmlspecialchars($this->metadata['name']),
-			'META_TYPE'			=> htmlspecialchars($this->metadata['type']),
-			'META_DESCRIPTION'	=> (isset($this->metadata['description'])) ? htmlspecialchars($this->metadata['description']) : '',
+			'META_NAME'			=> $this->metadata['name'],
+			'META_TYPE'			=> $this->metadata['type'],
+			'META_DESCRIPTION'	=> (isset($this->metadata['description'])) ? $this->metadata['description'] : '',
 			'META_HOMEPAGE'		=> (isset($this->metadata['homepage'])) ? $this->metadata['homepage'] : '',
-			'META_VERSION'		=> (isset($this->metadata['version'])) ? htmlspecialchars($this->metadata['version']) : '',
-			'META_TIME'			=> (isset($this->metadata['time'])) ? htmlspecialchars($this->metadata['time']) : '',
-			'META_LICENSE'		=> htmlspecialchars($this->metadata['license']),
+			'META_VERSION'		=> (isset($this->metadata['version'])) ? $this->metadata['version'] : '',
+			'META_TIME'			=> (isset($this->metadata['time'])) ? $this->metadata['time'] : '',
+			'META_LICENSE'		=> $this->metadata['license'],
 
-			'META_REQUIRE_PHP'		=> (isset($this->metadata['require']['php'])) ? htmlspecialchars($this->metadata['require']['php']) : '',
+			'META_REQUIRE_PHP'		=> (isset($this->metadata['require']['php'])) ? $this->metadata['require']['php'] : '',
 			'META_REQUIRE_PHP_FAIL'	=> !$this->validate_require_php(),
 
-			'META_REQUIRE_PHPBB'		=> (isset($this->metadata['extra']['soft-require']['phpbb/phpbb'])) ? htmlspecialchars($this->metadata['extra']['soft-require']['phpbb/phpbb']) : '',
+			'META_REQUIRE_PHPBB'		=> (isset($this->metadata['extra']['soft-require']['phpbb/phpbb'])) ? $this->metadata['extra']['soft-require']['phpbb/phpbb'] : '',
 			'META_REQUIRE_PHPBB_FAIL'	=> !$this->validate_require_phpbb(),
 
-			'META_DISPLAY_NAME'	=> (isset($this->metadata['extra']['display-name'])) ? htmlspecialchars($this->metadata['extra']['display-name']) : '',
+			'META_DISPLAY_NAME'	=> (isset($this->metadata['extra']['display-name'])) ? $this->metadata['extra']['display-name'] : '',
 		));
 
 		foreach ($this->metadata['authors'] as $author)
 		{
 			$this->template->assign_block_vars('meta_authors', array(
-				'AUTHOR_NAME'		=> htmlspecialchars($author['name']),
+				'AUTHOR_NAME'		=> $author['name'],
 				'AUTHOR_EMAIL'		=> (isset($author['email'])) ? $author['email'] : '',
 				'AUTHOR_HOMEPAGE'	=> (isset($author['homepage'])) ? $author['homepage'] : '',
-				'AUTHOR_ROLE'		=> (isset($author['role'])) ? htmlspecialchars($author['role']) : '',
+				'AUTHOR_ROLE'		=> (isset($author['role'])) ? $author['role'] : '',
 			));
 		}
 	}
diff --git a/phpBB/phpbb/version_helper.php b/phpBB/phpbb/version_helper.php
index e34bd0ba60..7387ad296e 100644
--- a/phpBB/phpbb/version_helper.php
+++ b/phpBB/phpbb/version_helper.php
@@ -270,6 +270,16 @@ class version_helper
 
 			$info = json_decode($info, true);
 
+			// Sanitize any data we retrieve from a server
+			if (!empty($info))
+			{
+				$json_sanitizer = function (&$value, $key) {
+					$type_cast_helper = new \phpbb\request\type_cast_helper();
+					$type_cast_helper->set_var($value, $value, gettype($value), true);
+				};
+				array_walk_recursive($info, $json_sanitizer);
+			}
+
 			if (empty($info['stable']) && empty($info['unstable']))
 			{
 				$this->user->add_lang('acp/common');
@@ -277,15 +287,6 @@ class version_helper
 				throw new \RuntimeException($this->user->lang('VERSIONCHECK_FAIL'));
 			}
 
-			// Replace & with &amp; on announcement links
-			foreach ($info as $stability => $branches)
-			{
-				foreach ($branches as $branch => $branch_data)
-				{
-					$info[$stability][$branch]['announcement'] = (!empty($branch_data['announcement'])) ? str_replace('&', '&amp;', $branch_data['announcement']) : '';
-				}
-			}
-
 			$info['stable'] = (empty($info['stable'])) ? array() : $info['stable'];
 			$info['unstable'] = (empty($info['unstable'])) ? $info['stable'] : $info['unstable'];
 
diff --git a/tests/extension/metadata_manager_test.php b/tests/extension/metadata_manager_test.php
index 8e27b39459..fab1d3af3a 100644
--- a/tests/extension/metadata_manager_test.php
+++ b/tests/extension/metadata_manager_test.php
@@ -123,6 +123,7 @@ class phpbb_extension_metadata_manager_test extends phpbb_database_test_case
 		}
 
 		$json = json_decode(file_get_contents($this->phpbb_root_path . 'ext/vendor2/foo/composer.json'), true);
+		array_walk_recursive($json, array($manager, 'sanitize_json'));
 
 		$this->assertEquals($metadata, $json);
 	}
diff --git a/tests/mock/file_downloader.php b/tests/mock/file_downloader.php
new file mode 100644
index 0000000000..d8951cebf6
--- /dev/null
+++ b/tests/mock/file_downloader.php
@@ -0,0 +1,27 @@
+<?php
+/**
+*
+* This file is part of the phpBB Forum Software package.
+*
+* @copyright (c) phpBB Limited <https://www.phpbb.com>
+* @license GNU General Public License, version 2 (GPL-2.0)
+*
+* For full copyright and license information, please see
+* the docs/CREDITS.txt file.
+*
+*/
+
+class phpbb_mock_file_downloader extends \phpbb\file_downloader
+{
+	public $data;
+
+	public function set($data)
+	{
+		$this->data = $data;
+	}
+
+	public function get($host, $directory, $filename, $port = 80, $timeout = 6)
+	{
+		return $this->data;
+	}
+}
diff --git a/tests/mock/metadata_manager.php b/tests/mock/metadata_manager.php
index 16900a0fc1..2443fad560 100644
--- a/tests/mock/metadata_manager.php
+++ b/tests/mock/metadata_manager.php
@@ -15,11 +15,13 @@ class phpbb_mock_metadata_manager extends \phpbb\extension\metadata_manager
 {
 	public function set_metadata($metadata)
 	{
+		array_walk_recursive($metadata, array($this, 'sanitize_json'));
 		$this->metadata = $metadata;
 	}
 
 	public function merge_metadata($metadata)
 	{
+		array_walk_recursive($metadata, array($this, 'sanitize_json'));
 		$this->metadata = array_merge($this->metadata, $metadata);
 	}
 }
diff --git a/tests/version/version_helper_remote_test.php b/tests/version/version_helper_remote_test.php
new file mode 100644
index 0000000000..65ae7646b9
--- /dev/null
+++ b/tests/version/version_helper_remote_test.php
@@ -0,0 +1,173 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ * For full copyright and license information, please see
+ * the docs/CREDITS.txt file.
+ *
+ */
+
+class version_helper_remote_test extends \phpbb_test_case
+{
+	protected $file_downloader;
+	protected $cache;
+	protected $version_helper;
+
+	public function setUp()
+	{
+		parent::setUp();
+
+		global $phpbb_root_path, $phpEx;
+
+		include_once($phpbb_root_path . 'includes/functions.' . $phpEx);
+
+		$config = new \phpbb\config\config(array(
+			'version'	=> '3.1.0',
+		));
+		$container = new \phpbb_mock_container_builder();
+		$db = new \phpbb\db\driver\factory($container);
+		$this->cache = $this->getMock('\phpbb\cache\service', array('get'), array(new \phpbb\cache\driver\null(), $config, $db, '../../', 'php'));
+		$this->cache->expects($this->any())
+			->method('get')
+			->with($this->anything())
+			->will($this->returnValue(false));
+		$this->file_downloader = new phpbb_mock_file_downloader();
+
+		$this->version_helper = new \phpbb\version_helper(
+			$this->cache,
+			$config,
+			$this->file_downloader,
+			new \phpbb\user('\phpbb\datetime')
+		);
+		$this->user = new \phpbb\user('\phpbb\datetime');
+		$this->user->add_lang('acp/common');
+	}
+
+	public function provider_get_versions()
+	{
+		return array(
+			array('', false),
+			array('foobar', false),
+			array('{
+    "stable": {
+        "1.0": {
+            "current": "1.0.1",
+            "download": "https://www.phpbb.com/customise/db/download/104136",
+            "announcement": "https://www.phpbb.com/customise/db/extension/boardrules/",
+            "eol": null,
+            "security": false
+        }
+    }
+}', true, array (
+				'stable' => array (
+					'1.0' => array (
+						'current' => '1.0.1',
+						'download' => 'https://www.phpbb.com/customise/db/download/104136',
+						'announcement' => 'https://www.phpbb.com/customise/db/extension/boardrules/',
+						'eol' => NULL,
+						'security' => false,
+					),
+				),
+				'unstable' => array (
+					'1.0' => array (
+						'current' => '1.0.1',
+						'download' => 'https://www.phpbb.com/customise/db/download/104136',
+						'announcement' => 'https://www.phpbb.com/customise/db/extension/boardrules/',
+						'eol' => NULL,
+						'security' => false,
+					),
+				),
+			)),
+			array('{
+    "foobar": {
+        "1.0": {
+            "current": "1.0.1",
+            "download": "https://www.phpbb.com/customise/db/download/104136",
+            "announcement": "https://www.phpbb.com/customise/db/extension/boardrules/",
+            "eol": null,
+            "security": false
+        }
+    }
+}', false),
+			array('{
+    "stable": {
+        "1.0": {
+            "current": "1.0.1<script>alert(\'foo\');</script>",
+            "download": "https://www.phpbb.com/customise/db/download/104136<script>alert(\'foo\');</script>",
+            "announcement": "https://www.phpbb.com/customise/db/extension/boardrules/<script>alert(\'foo\');</script>",
+            "eol": "<script>alert(\'foo\');</script>",
+            "security": "<script>alert(\'foo\');</script>"
+        }
+    }
+}', true, array (
+				'stable' => array (
+					'1.0' => array (
+						'current' => '1.0.1&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'download' => 'https://www.phpbb.com/customise/db/download/104136&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'announcement' => 'https://www.phpbb.com/customise/db/extension/boardrules/&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'eol' => '&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'security' => '&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+					),
+				),
+				'unstable' => array (
+					'1.0' => array (
+						'current' => '1.0.1&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'download' => 'https://www.phpbb.com/customise/db/download/104136&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'announcement' => 'https://www.phpbb.com/customise/db/extension/boardrules/&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'eol' => '&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'security' => '&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+					),
+				),
+			)),
+			array('{
+    "unstable": {
+        "1.0": {
+            "current": "1.0.1<script>alert(\'foo\');</script>",
+            "download": "https://www.phpbb.com/customise/db/download/104136<script>alert(\'foo\');</script>",
+            "announcement": "https://www.phpbb.com/customise/db/extension/boardrules/<script>alert(\'foo\');</script>",
+            "eol": "<script>alert(\'foo\');</script>",
+            "security": "<script>alert(\'foo\');</script>"
+        }
+    }
+}', true, array (
+				'unstable' => array (
+					'1.0' => array (
+						'current' => '1.0.1&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'download' => 'https://www.phpbb.com/customise/db/download/104136&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'announcement' => 'https://www.phpbb.com/customise/db/extension/boardrules/&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'eol' => '&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+						'security' => '&lt;script&gt;alert(\'foo\');&lt;/script&gt;',
+					),
+				),
+				'stable' => array(),
+			)),
+		);
+	}
+
+	/**
+	 * @dataProvider provider_get_versions
+	 */
+	public function test_get_versions($input, $valid_data, $expected_return = '')
+	{
+		$this->file_downloader->set($input);
+
+		if (!$valid_data)
+		{
+			try {
+				$return = $this->version_helper->get_versions();
+			} catch (\RuntimeException $e) {
+				$this->assertEquals((string)$e->getMessage(), $this->user->lang('VERSIONCHECK_FAIL'));
+			}
+		}
+		else
+		{
+			$return = $this->version_helper->get_versions();
+		}
+
+		$this->assertEquals($expected_return, $return);
+	}
+}
author	Joas Schilling <nickvergessen@gmx.de>	2014-11-22 17:48:37 +0100
committer	Joas Schilling <nickvergessen@gmx.de>	2014-11-22 17:48:37 +0100
commit	5c3b6a855944d7a370186987ebebe3a896cf76a2 (patch)
tree	a4e50394f86019133c053e3dd0339b291c6a5bae
parent	a3b18d87ddef00424a2dc35061780e1a7dc2c0c6 (diff)
parent	0f06b88ee7aa508ea44704f2973ce1e57f9accf2 (diff)
download	forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.tar forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.tar.gz forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.tar.bz2 forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.tar.xz forums-5c3b6a855944d7a370186987ebebe3a896cf76a2.zip