From 61683f895cff778d722175a8e5ddd2a5facbc42f Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 13 Nov 2016 11:43:17 +0100 Subject: [PATCH 01/21] [ticket/security-181] Deny access to migrations folders SECURITY-181 --- phpBB/phpbb/db/migration/data/v30x/.htaccess | 33 ++++++++++++++++++++ phpBB/phpbb/db/migration/data/v310/.htaccess | 33 ++++++++++++++++++++ phpBB/phpbb/db/migration/data/v31x/.htaccess | 33 ++++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 phpBB/phpbb/db/migration/data/v30x/.htaccess create mode 100644 phpBB/phpbb/db/migration/data/v310/.htaccess create mode 100644 phpBB/phpbb/db/migration/data/v31x/.htaccess diff --git a/phpBB/phpbb/db/migration/data/v30x/.htaccess b/phpBB/phpbb/db/migration/data/v30x/.htaccess new file mode 100644 index 0000000000..44242b5418 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v30x/.htaccess @@ -0,0 +1,33 @@ +# With Apache 2.4 the "Order, Deny" syntax has been deprecated and moved from +# module mod_authz_host to a new module called mod_access_compat (which may be +# disabled) and a new "Require" syntax has been introduced to mod_authz_host. +# We could just conditionally provide both versions, but unfortunately Apache +# does not explicitly tell us its version if the module mod_version is not +# available. In this case, we check for the availability of module +# mod_authz_core (which should be on 2.4 or higher only) as a best guess. + + + + Order Allow,Deny + Deny from All + + + = 2.4> + + Require all denied + + + + + + + Order Allow,Deny + Deny from All + + + + + Require all denied + + + diff --git a/phpBB/phpbb/db/migration/data/v310/.htaccess b/phpBB/phpbb/db/migration/data/v310/.htaccess new file mode 100644 index 0000000000..44242b5418 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v310/.htaccess @@ -0,0 +1,33 @@ +# With Apache 2.4 the "Order, Deny" syntax has been deprecated and moved from +# module mod_authz_host to a new module called mod_access_compat (which may be +# disabled) and a new "Require" syntax has been introduced to mod_authz_host. +# We could just conditionally provide both versions, but unfortunately Apache +# does not explicitly tell us its version if the module mod_version is not +# available. In this case, we check for the availability of module +# mod_authz_core (which should be on 2.4 or higher only) as a best guess. + + + + Order Allow,Deny + Deny from All + + + = 2.4> + + Require all denied + + + + + + + Order Allow,Deny + Deny from All + + + + + Require all denied + + + diff --git a/phpBB/phpbb/db/migration/data/v31x/.htaccess b/phpBB/phpbb/db/migration/data/v31x/.htaccess new file mode 100644 index 0000000000..44242b5418 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v31x/.htaccess @@ -0,0 +1,33 @@ +# With Apache 2.4 the "Order, Deny" syntax has been deprecated and moved from +# module mod_authz_host to a new module called mod_access_compat (which may be +# disabled) and a new "Require" syntax has been introduced to mod_authz_host. +# We could just conditionally provide both versions, but unfortunately Apache +# does not explicitly tell us its version if the module mod_version is not +# available. In this case, we check for the availability of module +# mod_authz_core (which should be on 2.4 or higher only) as a best guess. + + + + Order Allow,Deny + Deny from All + + + = 2.4> + + Require all denied + + + + + + + Order Allow,Deny + Deny from All + + + + + Require all denied + + + From 7ba9b06881ddd70bd3b10e2785b91908e851cdaa Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 13 Nov 2016 11:50:23 +0100 Subject: [PATCH 02/21] [ticket/security-181] Port .htaccess changes to other webserver types SECURITY-181 --- phpBB/docs/lighttpd.sample.conf | 2 +- phpBB/docs/nginx.sample.conf | 2 +- phpBB/web.config | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/phpBB/docs/lighttpd.sample.conf b/phpBB/docs/lighttpd.sample.conf index 5b04122267..f5b509e002 100644 --- a/phpBB/docs/lighttpd.sample.conf +++ b/phpBB/docs/lighttpd.sample.conf @@ -37,7 +37,7 @@ $HTTP["host"] == "www.myforums.com" { accesslog.filename = "/var/log/lighttpd/access-www.myforums.com.log" # Deny access to internal phpbb files. - $HTTP["url"] =~ "^/(config\.php|common\.php|includes|cache|files|store|images/avatars/upload)" { + $HTTP["url"] =~ "^/(config\.php|common\.php|cache|files|images/avatars/upload|includes|phpbb|store|vendor)" { url.access-deny = ( "" ) } diff --git a/phpBB/docs/nginx.sample.conf b/phpBB/docs/nginx.sample.conf index 2ead3552fd..bf33f4e73d 100644 --- a/phpBB/docs/nginx.sample.conf +++ b/phpBB/docs/nginx.sample.conf @@ -72,7 +72,7 @@ http { } # Deny access to internal phpbb files. - location ~ /(config\.php|common\.php|includes|cache|files|store|images/avatars/upload) { + location ~ /(config\.php|common\.php|cache|files|images/avatars/upload|includes|phpbb|store|vendor) { deny all; # deny was ignored before 0.8.40 for connections over IPv6. # Use internal directive to prohibit access on older versions. diff --git a/phpBB/web.config b/phpBB/web.config index 99a1fe6023..d0a3cb33fe 100644 --- a/phpBB/web.config +++ b/phpBB/web.config @@ -18,7 +18,10 @@ + + + From 44dd1ef9842c83f7ba4a37bf4a17489d5fe73991 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 13 Nov 2016 12:26:35 +0100 Subject: [PATCH 03/21] [ticket/security-181] Update INSTALL.html to ask for more secure apache config SECURITY-181 --- phpBB/docs/INSTALL.html | 18 +++++++++++++++--- phpBB/docs/assets/css/stylesheet.css | 11 +++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/phpBB/docs/INSTALL.html b/phpBB/docs/INSTALL.html index 9f8bbe74b8..53c18da733 100644 --- a/phpBB/docs/INSTALL.html +++ b/phpBB/docs/INSTALL.html @@ -148,7 +148,7 @@
  • Oracle
    • -
  • PHP 5.3.3+ and PHP < 7.0 with support for the database you intend to use.
    • +
  • PHP 5.3.3+ and PHP < 7.0 with support for the database you intend to use.
  • The following PHP modules are required:
    • json
      • @@ -455,9 +455,21 @@

      6.ii. Webserver configuration

      -

      Depending on your web server, you may have to configure your server to deny web access to the cache/, files/, store/ and other directories. This is to prevent users from accessing sensitive files.

      +

      Depending on your web server, you may have to configure your server to deny web access to the cache/, files/, includes, phpbb, store/, and vendor directories. This is to prevent users from accessing sensitive files.

      -

      For Apache there are .htaccess files already in place to do this for you. Similarly, for Windows based servers using IIS there are web.config files already in place to do this for you. For other webservers, you will have to adjust the configuration yourself. Sample files for nginx and lighttpd to help you get started may be found in docs/ directory.

      +

      + For Apache there are .htaccess files already in place to do this for the most sensitive files and folders. We do however recommend to completely deny all access to the aforementioned folders and their respective subfolders in your Apache configuration.
      + On Apache 2.4, denying access to the phpbb folder in a phpBB instance located at /var/www/html/ would work like this: + 

      +<Directory /var/www/html/phpbb/*>
+	Require all denied
+</Directory>
+<Directory /var/www/html/phpbb>
+	Require all denied
+</Directory>
      +
      +

      The same settings can be applied to the other mentioned directories by replacing phpbb by the respective directory name. Please pay attention to the difference in syntax between Apache version 2.2 and 2.4.

      +

      For Windows based servers using IIS there are web.config files already in place to do this for you. For other webservers, you will have to adjust the configuration yourself. Sample files for nginx and lighttpd to help you get started may be found in docs/ directory.

      diff --git a/phpBB/docs/assets/css/stylesheet.css b/phpBB/docs/assets/css/stylesheet.css index 192a6f9f79..c090ab7e07 100644 --- a/phpBB/docs/assets/css/stylesheet.css +++ b/phpBB/docs/assets/css/stylesheet.css @@ -115,6 +115,17 @@ code { padding: 0 4px; } +pre { + color: #006600; + font-weight: normal; + font-family: 'Courier New', monospace; + border-color: #D1D7DC; + border-width: 1px; + border-style: solid; + background-color: #FAFAFA; + padding: 0 4px +} + #wrap { padding: 0 20px; min-width: 650px; From 78c2e31ec2cab20f5fc0555706b27b63ad7d9437 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 13 Nov 2016 11:45:42 +0100 Subject: [PATCH 04/21] [ticket/security-181] Add .htaccess to 3.2.x migrations SECURITY-181 --- phpBB/phpbb/db/migration/data/v320/.htaccess | 33 ++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 phpBB/phpbb/db/migration/data/v320/.htaccess diff --git a/phpBB/phpbb/db/migration/data/v320/.htaccess b/phpBB/phpbb/db/migration/data/v320/.htaccess new file mode 100644 index 0000000000..44242b5418 --- /dev/null +++ b/phpBB/phpbb/db/migration/data/v320/.htaccess @@ -0,0 +1,33 @@ +# With Apache 2.4 the "Order, Deny" syntax has been deprecated and moved from +# module mod_authz_host to a new module called mod_access_compat (which may be +# disabled) and a new "Require" syntax has been introduced to mod_authz_host. +# We could just conditionally provide both versions, but unfortunately Apache +# does not explicitly tell us its version if the module mod_version is not +# available. In this case, we check for the availability of module +# mod_authz_core (which should be on 2.4 or higher only) as a best guess. + + + + Order Allow,Deny + Deny from All + + + = 2.4> + + Require all denied + + + + + + + Order Allow,Deny + Deny from All + + + + + Require all denied + + + From 658820654f5789a786a5537c1b43991744b83d2c Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 26 Dec 2016 22:01:51 +0100 Subject: [PATCH 05/21] [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 | 100 +++++++++++------ 4 files changed, 185 insertions(+), 40 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 ( - 'unstable' => 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>', + '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, ), ), - '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": "