From 658820654f5789a786a5537c1b43991744b83d2c Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 26 Dec 2016 22:01:51 +0100 Subject: [ticket/security-203] Fully validate version check data in version helper This will also take care of SECURITY-204 as it's the same underlying issue. Admins still need to ensure they don't visit malicious sites for URLs provided by extensions. SECURITY-203 --- phpBB/includes/functions.php | 5 ++ phpBB/language/en/acp/common.php | 13 ++-- phpBB/phpbb/version_helper.php | 107 +++++++++++++++++++++++++++ tests/version/version_helper_remote_test.php | 98 +++++++++++++++--------- 4 files changed, 184 insertions(+), 39 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index ba448f3125..84178f74e4 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -3442,6 +3442,11 @@ function get_preg_expression($mode) case 'path_remove_dot_trailing_slash': return '#^(?:(\.)?)+(?:(.+)?)+(?:([\\/\\\])$)#'; break; + + case 'semantic_version': + // Regular expression to match semantic versions by http://rgxdb.com/ + return '/(?<=^[Vv]|^)(?:(?(?:0|[1-9](?:(?:0|[1-9])+)*))[.](?(?:0|[1-9](?:(?:0|[1-9])+)*))[.](?(?:0|[1-9](?:(?:0|[1-9])+)*))(?:-(?(?:(?:(?:[A-Za-z]|-)(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)?|(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)(?:[A-Za-z]|-)(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)?)|(?:0|[1-9](?:(?:0|[1-9])+)*))(?:[.](?:(?:(?:[A-Za-z]|-)(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)?|(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)(?:[A-Za-z]|-)(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)?)|(?:0|[1-9](?:(?:0|[1-9])+)*)))*))?(?:[+](?(?:(?:(?:[A-Za-z]|-)(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)?|(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)(?:[A-Za-z]|-)(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)?)|(?:(?:0|[1-9])+))(?:[.](?:(?:(?:[A-Za-z]|-)(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)?|(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)(?:[A-Za-z]|-)(?:(?:(?:0|[1-9])|(?:[A-Za-z]|-))+)?)|(?:(?:0|[1-9])+)))*))?)$/'; + break; } return ''; diff --git a/phpBB/language/en/acp/common.php b/phpBB/language/en/acp/common.php index 88e60d00a3..9d2723ceb3 100644 --- a/phpBB/language/en/acp/common.php +++ b/phpBB/language/en/acp/common.php @@ -417,11 +417,14 @@ $lang = array_merge($lang, array( 'UPLOAD_DIR_SIZE' => 'Size of posted attachments', 'USERS_PER_DAY' => 'Users per day', - 'VALUE' => 'Value', - 'VERSIONCHECK_FAIL' => 'Failed to obtain latest version information.', - 'VERSIONCHECK_FORCE_UPDATE' => 'Re-Check version', - 'VIEW_ADMIN_LOG' => 'View administrator log', - 'VIEW_INACTIVE_USERS' => 'View inactive users', + 'VALUE' => 'Value', + 'VERSIONCHECK_FAIL' => 'Failed to obtain latest version information.', + 'VERSIONCHECK_FORCE_UPDATE' => 'Re-Check version', + 'VERSIONCHECK_INVALID_ENTRY' => 'Latest version information contains an unsupported entry.', + 'VERSIONCHECK_INVALID_URL' => 'Latest version information contains invalid URL.', + 'VERSIONCHECK_INVALID_VERSION' => 'Latest version information contains an invalid version.', + 'VIEW_ADMIN_LOG' => 'View administrator log', + 'VIEW_INACTIVE_USERS' => 'View inactive users', 'WELCOME_PHPBB' => 'Welcome to phpBB', 'WRITABLE_CONFIG' => 'Your config file (config.php) is currently world-writable. We strongly encourage you to change the permissions to 640 or at least to 644 (for example: chmod 640 config.php).', diff --git a/phpBB/phpbb/version_helper.php b/phpBB/phpbb/version_helper.php index a1e66ba8fe..dc95f6d001 100644 --- a/phpBB/phpbb/version_helper.php +++ b/phpBB/phpbb/version_helper.php @@ -61,6 +61,23 @@ class version_helper /** @var \phpbb\user */ protected $user; + protected $version_schema = array( + 'stable' => array( + 'current' => 'version', + 'download' => 'url', + 'announcement' => 'url', + 'eol' => 'url', + 'security' => 'bool', + ), + 'unstable' => array( + 'current' => 'version', + 'download' => 'url', + 'announcement' => 'url', + 'eol' => 'url', + 'security' => 'bool', + ), + ); + /** * Constructor * @@ -298,9 +315,99 @@ class version_helper $info['stable'] = (empty($info['stable'])) ? array() : $info['stable']; $info['unstable'] = (empty($info['unstable'])) ? $info['stable'] : $info['unstable']; + $this->validate_versions($info); + $this->cache->put($cache_file, $info, 86400); // 24 hours } return $info; } + + /** + * Validate versions info input + * + * @param array $versions_info Decoded json data array. Will be modified + * and cleaned by this method + */ + public function validate_versions(&$versions_info) + { + $array_diff = array_diff_key($versions_info, array($this->version_schema)); + + // Remove excessive data + if (count($array_diff) > 0) + { + $old_versions_info = $versions_info; + $versions_info = array( + 'stable' => !empty($old_versions_info['stable']) ? $old_versions_info['stable'] : array(), + 'unstable' => !empty($old_versions_info['unstable']) ? $old_versions_info['unstable'] : array(), + ); + unset($old_versions_info); + } + + foreach ($versions_info as $stability_type => &$versions_data) + { + foreach ($versions_data as $branch => &$version_data) + { + if (!preg_match('/^[0-9]+\.[0-9]+$/', $branch)) + { + unset($versions_data[$branch]); + continue; + } + + $stability_diff = array_diff_key($version_data, $this->version_schema[$stability_type]); + + if (count($stability_diff) > 0) + { + $old_version_data = $version_data; + $version_data = array(); + foreach ($this->version_schema[$stability_type] as $key => $value) + { + if (isset($old_version_data[$key]) || $old_version_data[$key] === null) + { + $version_data[$key] = $old_version_data[$key]; + } + } + unset($old_version_data); + } + + foreach ($version_data as $key => &$value) + { + if (!isset($this->version_schema[$stability_type][$key])) + { + unset($version_data[$key]); + throw new \RuntimeException($this->user->lang('VERSIONCHECK_INVALID_ENTRY')); + } + + switch ($this->version_schema[$stability_type][$key]) + { + case 'bool': + $value = (bool) $value; + break; + + case 'url': + if (!empty($value) && !preg_match('#^' . get_preg_expression('url') . '$#iu', $value) && + !preg_match('#^' . get_preg_expression('www_url') . '$#iu', $value)) + { + $value = ''; + throw new \RuntimeException($this->user->lang('VERSIONCHECK_INVALID_URL')); + } + break; + + case 'version': + $value = $value ?: ''; + if (!preg_match(get_preg_expression('semantic_version'), $value)) + { + $value = ''; + throw new \RuntimeException($this->user->lang('VERSIONCHECK_INVALID_VERSION')); + } + break; + + default: + // Shouldn't be possible to trigger this + throw new \RuntimeException($this->user->lang('VERSIONCHECK_INVALID_ENTRY')); + } + } + } + } + } } diff --git a/tests/version/version_helper_remote_test.php b/tests/version/version_helper_remote_test.php index 65ae7646b9..596b7194de 100644 --- a/tests/version/version_helper_remote_test.php +++ b/tests/version/version_helper_remote_test.php @@ -37,21 +37,21 @@ class version_helper_remote_test extends \phpbb_test_case ->will($this->returnValue(false)); $this->file_downloader = new phpbb_mock_file_downloader(); + $this->user = new \phpbb\user('\phpbb\datetime'); + $this->user->add_lang('acp/common'); $this->version_helper = new \phpbb\version_helper( $this->cache, $config, $this->file_downloader, - new \phpbb\user('\phpbb\datetime') + $this->user ); - $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('', false, '', 'VERSIONCHECK_FAIL'), + array('foobar', false, '', 'VERSIONCHECK_FAIL'), array('{ "stable": { "1.0": { @@ -92,7 +92,7 @@ class version_helper_remote_test extends \phpbb_test_case "security": false } } -}', false), +}', false, '', 'VERSIONCHECK_FAIL'), array('{ "stable": { "1.0": { @@ -103,26 +103,7 @@ class version_helper_remote_test extends \phpbb_test_case "security": "" } } -}', true, array ( - 'stable' => array ( - '1.0' => array ( - '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>', - ), - ), - 'unstable' => array ( - '1.0' => array ( - '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>', - ), - ), - )), +}', false, null, 'VERSIONCHECK_INVALID_VERSION'), array('{ "unstable": { "1.0": { @@ -133,25 +114,74 @@ class version_helper_remote_test extends \phpbb_test_case "security": "" } } +}', false, null, 'VERSIONCHECK_INVALID_VERSION'), + array('{ + "unstable": { + "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": "", + "security": "" + } + } +}', false, array('stable' => array(), 'unstable' => array()), 'VERSIONCHECK_INVALID_VERSION'), + array('{ + "\"\n\n": "test", + "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<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>', + '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, ), ), - 'stable' => array(), )), + array('{ + "unstable": { + "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, + "foobar": "": "1.0.1", + "download2": "https://www.phpbb.com/customise/db/download/104136", + "bannouncement": "https://www.phpbb.com/customise/db/extension/boardrules/", + "eol": null, + "security": false, + "foobar": "