From 067268942cb2d19b3f36603b4480b6c6f849ce36 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 8 Nov 2016 14:38:58 +0800 Subject: [PATCH 1/3] MDL-48498 admin: new setting types for curl host/port restrictions Two new classes in lib/adminlib. One providing validation for a newline-delimited textarea supporting domain names, domain wildcards, IPv4/IPv6 addresses and IPv4/IPv6 ranges. The second providing validation for a newline-delimited textarea of port numbers. --- admin/settings/security.php | 9 +- lang/en/admin.php | 7 ++ lib/adminlib.php | 173 ++++++++++++++++++++++++++++++++++++ version.php | 2 +- 4 files changed, 189 insertions(+), 2 deletions(-) diff --git a/admin/settings/security.php b/admin/settings/security.php index c892566929f01..1d89d42fed8f1 100644 --- a/admin/settings/security.php +++ b/admin/settings/security.php @@ -118,8 +118,15 @@ $temp->add(new admin_setting_configcheckbox('cookiehttponly', new lang_string('cookiehttponly', 'admin'), new lang_string('configcookiehttponly', 'admin'), 0)); $temp->add(new admin_setting_configcheckbox('allowframembedding', new lang_string('allowframembedding', 'admin'), new lang_string('allowframembedding_help', 'admin'), 0)); $temp->add(new admin_setting_configcheckbox('loginpasswordautocomplete', new lang_string('loginpasswordautocomplete', 'admin'), new lang_string('loginpasswordautocomplete_help', 'admin'), 0)); - $ADMIN->add('security', $temp); + // Settings elements used by the \core\files\curl_security_helper class. + $temp->add(new admin_setting_configmixedhostiplist('curlsecurityblockedhosts', + new lang_string('curlsecurityblockedhosts', 'admin'), + new lang_string('curlsecurityblockedhostssyntax', 'admin'), "")); + $temp->add(new admin_setting_configportlist('curlsecurityallowedport', + new lang_string('curlsecurityallowedport', 'admin'), + new lang_string('curlsecurityallowedportsyntax', 'admin'), "")); + $ADMIN->add('security', $temp); // "notifications" settingpage $temp = new admin_settingpage('notifications', new lang_string('notifications', 'admin')); diff --git a/lang/en/admin.php b/lang/en/admin.php index d650274d037ca..3a36e9c44395d 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -394,6 +394,11 @@ $string['cronwarning'] = 'The cron.php maintenance script has not been run for at least 24 hours.'; $string['cronwarningcli'] = 'The cli/cron.php maintenance script has not been run for at least 24 hours.'; $string['ctyperequired'] = 'The ctype PHP extension is now required by Moodle, in order to improve site performance and to offer multilingual compatibility.'; +$string['curlsecurityallowedport'] = 'cURL allowed ports list'; +$string['curlsecurityallowedportsyntax'] = 'Put every entry on one line. Valid entries are integer numbers only.'; +$string['curlsecurityblockedhosts'] = 'cURL blocked hosts list'; +$string['curlsecurityblockedhostssyntax'] = 'Put each entry on a new line. Valid entries are either full IPv4 or IPv6 addresses (such as 192.168.10.1, 0:0:0:0:0:0:0:1, ::1, fe80::) which match a single host; or CIDR notation (such as 231.54.211.0/20 or fe80::/64); or a range of IP addresses (such as 231.3.56.10-20 or fe80::1111-bbbb) where the range applies to the last group of the address; or domain names (such as localhost or example.com); or wildcard domain names (such as *.example.com or *.sub.example.com). Blank lines are not allowed.'; +$string['curlsecurityurlblocked'] = 'The URL is blocked.'; $string['curlcache'] = 'cURL cache TTL'; $string['curlrequired'] = 'The cURL PHP extension is now required by Moodle, in order to communicate with Moodle repositories.'; $string['curltimeoutkbitrate'] = 'Bitrate to use when calculating cURL timeouts (Kbps)'; @@ -1213,7 +1218,9 @@ $string['userquota'] = 'User quota'; $string['usesitenameforsitepages'] = 'Use site name for site pages'; $string['usetags'] = 'Enable tags functionality'; +$string['validateemptylineerror'] = 'Empty lines are not valid'; $string['validateerror'] = 'This value is not valid'; +$string['validateerrorlist'] = 'These entries are invalid: {$a}'; $string['validateiperror'] = 'These IP addresses are invalid: {$a}'; $string['verifychangedemail'] = 'Restrict domains when changing email'; $string['warningcurrentsetting'] = 'Invalid current value: {$a}'; diff --git a/lib/adminlib.php b/lib/adminlib.php index da825e150bfa6..a59d4817ee74c 100644 --- a/lib/adminlib.php +++ b/lib/adminlib.php @@ -3583,6 +3583,179 @@ public function validate($data) { } } +/** + * Used to validate a textarea used for domain names, wildcard domain names and IP addresses/ranges (both IPv4 and IPv6 format). + * + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @copyright 2016 Jake Dallimore (jrhdallimore@gmail.com) + */ +class admin_setting_configmixedhostiplist extends admin_setting_configtextarea { + + /** + * Validate the contents of the textarea as either IP addresses, domain name or wildcard domain name (RFC 4592). + * Used to validate a new line separated list of entries collected from a textarea control. + * + * This setting provides support for internationalised domain names (IDNs), however, such UTF-8 names will be converted to + * their ascii-compatible encoding (punycode) on save, and converted back to their UTF-8 representation when fetched + * via the get_setting() method, which has been overriden. + * + * @param string $data A list of FQDNs, DNS wildcard format domains, and IP addresses, separated by new lines. + * @return mixed bool true for success or string:error on failure + */ + public function validate($data) { + if (empty($data)) { + return true; + } + $entries = explode("\n", $data); + $badentries = []; + + foreach ($entries as $key => $entry) { + $entry = trim($entry); + if (empty($entry)) { + return get_string('validateemptylineerror', 'admin'); + } + + // Validate each string entry against the supported formats. + if (\core\ip_utils::is_ip_address($entry) || \core\ip_utils::is_ipv6_range($entry) + || \core\ip_utils::is_ipv4_range($entry) || \core\ip_utils::is_domain_name($entry) + || \core\ip_utils::is_domain_matching_pattern($entry)) { + continue; + } + + // Otherwise, the entry is invalid. + $badentries[] = $entry; + } + + if ($badentries) { + return get_string('validateerrorlist', 'admin', join(', ', $badentries)); + } + return true; + } + + /** + * Convert any lines containing international domain names (IDNs) to their ascii-compatible encoding (ACE). + * + * @param string $data the setting data, as sent from the web form. + * @return string $data the setting data, with all IDNs converted (using punycode) to their ascii encoded version. + */ + protected function ace_encode($data) { + if (empty($data)) { + return $data; + } + $entries = explode("\n", $data); + foreach ($entries as $key => $entry) { + $entry = trim($entry); + // This regex matches any string which: + // a) contains at least one non-ascii unicode character AND + // b) starts with a-zA-Z0-9 or any non-ascii unicode character AND + // c) ends with a-zA-Z0-9 or any non-ascii unicode character + // d) contains a-zA-Z0-9, hyphen, dot or any non-ascii unicode characters in the middle. + if (preg_match('/^(?=[^\x00-\x7f])([^\x00-\x7f]|[a-zA-Z0-9])([^\x00-\x7f]|[a-zA-Z0-9-.])*([^\x00-\x7f]|[a-zA-Z0-9])$/', + $entry)) { + // If we can convert the unicode string to an idn, do so. + // Otherwise, leave the original unicode string alone and let the validation function handle it (it will fail). + $val = idn_to_ascii($entry); + $entries[$key] = $val ? $val : $entry; + } + } + return implode("\n", $entries); + } + + /** + * Decode any ascii-encoded domain names back to their utf-8 representation for display. + * + * @param string $data the setting data, as found in the database. + * @return string $data the setting data, with all ascii-encoded IDNs decoded back to their utf-8 representation. + */ + protected function ace_decode($data) { + $entries = explode("\n", $data); + foreach ($entries as $key => $entry) { + $entry = trim($entry); + if (strpos($entry, 'xn--') !== false) { + $entries[$key] = idn_to_utf8($entry); + } + } + return implode("\n", $entries); + } + + /** + * Override, providing utf8-decoding for ascii-encoded IDN strings. + * + * @return string the setting string, with any punycode strings converted back to their utf-8 string representation. + */ + public function get_setting() { + // Here, we need to decode any ascii-encoded IDNs back to their native, utf-8 representation. + $data = $this->config_read($this->name); + if (function_exists('idn_to_utf8')) { + $data = $this->ace_decode($data); + } + return $data; + } + + /** + * Override, providing ascii-encoding for utf8 (native) IDN strings. + * + * @param string $data + * @return string + */ + public function write_setting($data) { + if ($this->paramtype === PARAM_INT and $data === '') { + // Do not complain if '' used instead of 0. + $data = 0; + } + + // Try to convert any non-ascii domains to ACE prior to validation - we can't modify anything in validate! + if (function_exists('idn_to_ascii')) { + $data = $this->ace_encode($data); + } + + $validated = $this->validate($data); + if ($validated !== true) { + return $validated; + } + return ($this->config_write($this->name, $data) ? '' : get_string('errorsetting', 'admin')); + } +} + +/** + * Used to validate a textarea used for port numbers. + * + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @copyright 2016 Jake Dallimore (jrhdallimore@gmail.com) + */ +class admin_setting_configportlist extends admin_setting_configtextarea { + + /** + * Validate the contents of the textarea as port numbers. + * Used to validate a new line separated list of ports collected from a textarea control. + * + * @param string $data A list of ports separated by new lines + * @return mixed bool true for success or string:error on failure + */ + public function validate($data) { + if (empty($data)) { + return true; + } + $ports = explode("\n", $data); + $badentries = []; + foreach ($ports as $port) { + $port = trim($port); + if (empty($port)) { + return get_string('validateemptylineerror', 'admin'); + } + + // Is the string a valid integer number? + if (strval(intval($port)) !== $port || intval($port) <= 0) { + $badentries[] = $port; + } + } + if ($badentries) { + return get_string('validateerrorlist', 'admin', $badentries); + } + return true; + } +} + /** * An admin setting for selecting one or more users who have a capability diff --git a/version.php b/version.php index 16626a61057c9..6d599e870c1a6 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2016110600.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2016110600.01; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes. From f6d9efefaac9ab4b658830b384adebbdf26e4e67 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 8 Nov 2016 14:40:38 +0800 Subject: [PATCH 2/3] MDL-48498 core_files: curl_security_helper_base and implementation Base class and core implementation providing a means to check URLs against the curl security admin settings entries. --- lib/classes/files/curl_security_helper.php | 254 ++++++++++++++++ .../files/curl_security_helper_base.php | 63 ++++ lib/phpunit/classes/util.php | 23 ++ lib/tests/curl_security_helper_test.php | 271 ++++++++++++++++++ 4 files changed, 611 insertions(+) create mode 100644 lib/classes/files/curl_security_helper.php create mode 100644 lib/classes/files/curl_security_helper_base.php create mode 100644 lib/tests/curl_security_helper_test.php diff --git a/lib/classes/files/curl_security_helper.php b/lib/classes/files/curl_security_helper.php new file mode 100644 index 0000000000000..6c0639f8f8178 --- /dev/null +++ b/lib/classes/files/curl_security_helper.php @@ -0,0 +1,254 @@ +. + +/** + * Contains a class providing functions used to check the host/port black/whitelists for curl. + * + * @package core + * @copyright 2016 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @author Jake Dallimore + */ + +namespace core\files; +use core\ip_utils; + +defined('MOODLE_INTERNAL') || exit(); + +/** + * Host and port checking for curl. + * + * This class provides a means to check URL/host/port against the system-level cURL security entries. + * It does not provide a means to add URLs, hosts or ports to the black/white lists; this is configured manually + * via the site admin section of Moodle (See: 'Site admin' > 'Security' > 'HTTP Security'). + * + * This class is currently used by the 'curl' wrapper class in lib/filelib.php. + * Depends on: + * core\ip_utils (several functions) + * moodlelib (clean_param) + * + * @package core + * @copyright 2016 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @author Jake Dallimore + */ +class curl_security_helper extends curl_security_helper_base { + /** + * @var array of supported transport schemes and their respective default ports. + */ + protected $transportschemes = [ + 'http' => 80, + 'https' => 443 + ]; + + /** + * Checks whether the given URL is blacklisted by checking its address and port number against the black/white lists. + * The behaviour of this function can be classified as strict, as it returns true for URLs which are invalid or + * could not be parsed, as well as those valid URLs which were found in the blacklist. + * + * @param string $urlstring the URL to check. + * @return bool true if the URL is blacklisted or invalid and false if the URL is not blacklisted. + */ + public function url_is_blocked($urlstring) { + // If no config data is present, then all hosts/ports are allowed. + if (!$this->is_enabled()) { + return false; + } + + // Try to parse the URL to get the 'host' and 'port' components. + try { + $url = new \moodle_url($urlstring); + $parsed['scheme'] = $url->get_scheme(); + $parsed['host'] = $url->get_host(); + $parsed['port'] = $url->get_port(); + } catch (\moodle_exception $e) { + // Moodle exception is thrown if the $urlstring is invalid. Treat as blocked. + return true; + } + + // The port will be empty unless explicitly set in the $url (uncommon), so try to infer it from the supported schemes. + if (!$parsed['port'] && $parsed['scheme'] && isset($this->transportschemes[$parsed['scheme']])) { + $parsed['port'] = $this->transportschemes[$parsed['scheme']]; + } + + if ($parsed['port'] && $parsed['host']) { + // Check the host and port against the blacklist/whitelist entries. + return $this->host_is_blocked($parsed['host']) || $this->port_is_blocked($parsed['port']); + } + return true; + } + + /** + * Returns a string message describing a blocked URL. E.g. 'This URL is blocked'. + * + * @return string the string error. + */ + public function get_blocked_url_string() { + return get_string('curlsecurityurlblocked', 'admin'); + } + + /** + * Checks whether the host portion of a url is blocked. + * The host portion may be a FQDN, IPv4 address or a IPv6 address wrapped in square brackets, as per standard URL notation. + * E.g. + * images.example.com + * 127.0.0.1 + * [0.0.0.0.0.0.0.1] + * The method logic is as follows: + * 1. Check the host component against the list of IPv4/IPv6 addresses and ranges. + * - This will perform a DNS forward lookup if required. + * 2. Check the host component against the list of domain names and wildcard domain names. + * - This will perform a DNS reverse lookup if required. + * + * @param string $host the host component of the URL to check against the blacklist. + * @return bool true if the host is both valid and blocked, false otherwise. + */ + protected function host_is_blocked($host) { + if (!$this->is_enabled() || empty($host) || !is_string($host)) { + return false; + } + + // Fix for square brackets in the 'host' portion of the URL (only occurs if an IPv6 address is specified). + $host = str_replace(array('[', ']'), '', $host); // RFC3986, section 3.2.2. + $blacklistedhosts = $this->get_blacklisted_hosts_by_category(); + + if (ip_utils::is_ip_address($host)) { + if ($this->address_explicitly_blocked($host)) { + return true; + } + + // Only perform a reverse lookup if there is a point to it (i.e. we have rules to check against). + if ($blacklistedhosts['domain'] || $blacklistedhosts['domainwildcard']) { + $hostname = gethostbyaddr($host); // DNS reverse lookup - supports both IPv4 and IPv6 address formats. + if ($hostname !== $host && $this->host_explicitly_blocked($hostname)) { + return true; + } + } + } else if (ip_utils::is_domain_name($host)) { + if ($this->host_explicitly_blocked($host)) { + return true; + } + + // Only perform a forward lookup if there are IP rules to check against. + if ($blacklistedhosts['ipv4'] || $blacklistedhosts['ipv6']) { + $hostip = gethostbyname($host); // DNS forward lookup - only returns IPv4 addresses! + if ($hostip !== $host && $this->address_explicitly_blocked($hostip)) { + return true; + } + } + } + return false; + } + + /** + * Checks whether the given port is blocked, as determined by its absence on the ports whitelist. + * Ports are assumed to be blocked unless found in the whitelist. + * + * @param integer|string $port the port to check against the ports whitelist. + * @return bool true if the port is blocked, false otherwise. + */ + protected function port_is_blocked($port) { + $portnum = intval($port); + // Intentionally block port 0 and below and check the int cast was valid. + if (empty($port) || (string)$portnum !== (string)$port || $port < 0) { + return true; + } + $allowedports = $this->get_whitelisted_ports(); + return !empty($allowedports) && !in_array($portnum, $allowedports); + } + + /** + * Convenience method to check whether we have any entries in the host blacklist or ports whitelist admin settings. + * If no entries are found at all, the assumption is that the blacklist is disabled entirely. + * + * @return bool true if one or more entries exist, false otherwise. + */ + public function is_enabled() { + return (!empty($this->get_whitelisted_ports()) || !empty($this->get_blacklisted_hosts())); + } + + /** + * Checks whether the input address is blocked by at any of the IPv4 or IPv6 address rules. + * + * @param string $addr the ip address to check. + * @return bool true if the address is covered by an entry in the blacklist, false otherwise. + */ + protected function address_explicitly_blocked($addr) { + $blockedhosts = $this->get_blacklisted_hosts_by_category(); + $iphostsblocked = array_merge($blockedhosts['ipv4'], $blockedhosts['ipv6']); + return address_in_subnet($addr, implode(',', $iphostsblocked)); + } + + /** + * Checks whether the input hostname is blocked by any of the domain/wildcard rules. + * + * @param string $host the hostname to check + * @return bool true if the host is covered by an entry in the blacklist, false otherwise. + */ + protected function host_explicitly_blocked($host) { + $blockedhosts = $this->get_blacklisted_hosts_by_category(); + $domainhostsblocked = array_merge($blockedhosts['domain'], $blockedhosts['domainwildcard']); + return ip_utils::is_domain_in_allowed_list($host, $domainhostsblocked); + } + + /** + * Helper to get all entries from the admin setting, as an array, sorted by classification. + * Classifications include 'ipv4', 'ipv6', 'domain', 'domainwildcard'. + * + * @return array of host/domain/ip entries from the 'curlsecurityblockedhosts' config. + */ + protected function get_blacklisted_hosts_by_category() { + // For each of the admin setting entries, check and place in the correct section of the config array. + $config = ['ipv6' => [], 'ipv4' => [], 'domain' => [], 'domainwildcard' => []]; + $entries = $this->get_blacklisted_hosts(); + foreach ($entries as $entry) { + if (ip_utils::is_ipv6_address($entry) || ip_utils::is_ipv6_range($entry)) { + $config['ipv6'][] = $entry; + } else if (ip_utils::is_ipv4_address($entry) || ip_utils::is_ipv4_range($entry)) { + $config['ipv4'][] = $entry; + } else if (ip_utils::is_domain_name($entry)) { + $config['domain'][] = $entry; + } else if (ip_utils::is_domain_matching_pattern($entry)) { + $config['domainwildcard'][] = $entry; + } + } + return $config; + } + + /** + * Helper that returns the whitelisted ports, as defined in the 'curlsecurityallowedport' setting. + * + * @return array the array of whitelisted ports. + */ + protected function get_whitelisted_ports() { + global $CFG; + return array_filter(explode("\n", $CFG->curlsecurityallowedport), function($entry) { + return !empty($entry); + }); + } + + /** + * Helper that returns the blacklisted hosts, as defined in the 'curlsecurityblockedhosts' setting. + * + * @return array the array of blacklisted host entries. + */ + protected function get_blacklisted_hosts() { + global $CFG; + return array_filter(array_map('trim', explode("\n", $CFG->curlsecurityblockedhosts)), function($entry) { + return !empty($entry); + }); + } +} diff --git a/lib/classes/files/curl_security_helper_base.php b/lib/classes/files/curl_security_helper_base.php new file mode 100644 index 0000000000000..e7f440ccdc8d7 --- /dev/null +++ b/lib/classes/files/curl_security_helper_base.php @@ -0,0 +1,63 @@ +. + +/** + * Contains an abstract base class definition for curl security helpers. + * + * @package core + * @copyright 2016 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @author Jake Dallimore + */ + +namespace core\files; + +defined('MOODLE_INTERNAL') || exit(); + +/** + * Security helper for the curl class. + * + * This class is intended as a base class for all curl security helpers. A curl security helper should provide a means to check + * a URL to determine whether curl should be allowed to request its content. It must also be able to return a simple string to + * explain that the URL is blocked, e.g. 'This URL is blocked'. + * + * Curl security helpers are currently used by the 'curl' wrapper class in lib/filelib.php. + * + * This class depends on: + * - nothing. + * + * @package core + * @copyright 2016 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @author Jake Dallimore + */ +abstract class curl_security_helper_base { + + /** + * Check whether the input url should be blocked or not. + * + * @param string $url the url to check. + * @return bool true if the url is deemed to be blocked, false otherwise. + */ + abstract public function url_is_blocked($url); + + /** + * Returns a string, explaining that a URL is blocked. + * + * @return string the lang string indicating that the url has been blocked. + */ + abstract public function get_blocked_url_string(); +} \ No newline at end of file diff --git a/lib/phpunit/classes/util.php b/lib/phpunit/classes/util.php index a3d2b34cef9da..6152374cb8533 100644 --- a/lib/phpunit/classes/util.php +++ b/lib/phpunit/classes/util.php @@ -839,4 +839,27 @@ public static function run_all_adhoc_tasks() { } } } + + /** + * Helper function to call a protected/private method of an object using reflection. + * + * Example 1. Calling a protected object method: + * $result = call_internal_method($myobject, 'method_name', [$param1, $param2], '\my\namespace\myobjectclassname'); + * + * Example 2. Calling a protected static method: + * $result = call_internal_method(null, 'method_name', [$param1, $param2], '\my\namespace\myclassname'); + * + * @param object|null $object the object on which to call the method, or null if calling a static method. + * @param string $methodname the name of the protected/private method. + * @param array $params the array of function params to pass to the method. + * @param string $classname the fully namespaced name of the class the object was created from (base in the case of mocks), + * or the name of the static class when calling a static method. + * @return mixed the respective return value of the method. + */ + public static function call_internal_method($object, $methodname, array $params = array(), $classname) { + $reflection = new \ReflectionClass($classname); + $method = $reflection->getMethod($methodname); + $method->setAccessible(true); + return $method->invokeArgs($object, $params); + } } diff --git a/lib/tests/curl_security_helper_test.php b/lib/tests/curl_security_helper_test.php new file mode 100644 index 0000000000000..83b1cc47e65fe --- /dev/null +++ b/lib/tests/curl_security_helper_test.php @@ -0,0 +1,271 @@ +. + +/** + * Unit tests for /lib/classes/curl/curl_security_helper.php. + * + * @package core + * @copyright 2016 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * cURL security test suite. + * + * Note: The curl_security_helper class performs forward and reverse DNS look-ups in some cases. This class will not attempt to test + * this functionality as look-ups can vary from machine to machine. Instead, human testing with known inputs/outputs is recommended. + * + * @package core + * @copyright 2016 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class core_curl_security_helper_testcase extends advanced_testcase { + /** + * Test for \core\files\curl_security_helper::url_is_blocked(). + * + * @param string $url the url to validate. + * @param string $blockedhosts the list of blocked hosts. + * @param string $allowedports the list of allowed ports. + * @param bool $expected the expected result. + * @dataProvider curl_security_url_data_provider + */ + public function test_curl_security_helper_url_is_blocked($url, $blockedhosts, $allowedports, $expected) { + $this->resetAfterTest(true); + $helper = new \core\files\curl_security_helper(); + set_config('curlsecurityblockedhosts', $blockedhosts); + set_config('curlsecurityallowedport', $allowedports); + $this->assertEquals($expected, $helper->url_is_blocked($url)); + } + + /** + * Data provider for test_curl_security_helper_url_is_blocked(). + * + * @return array + */ + public function curl_security_url_data_provider() { + // Format: url, blocked hosts, allowed ports, expected result. + return [ + // Base set without the blacklist enabled - no checking takes place. + ["http://localhost/x.png", "", "", false], // IP=127.0.0.1, Port=80 (port inferred from http). + ["http://localhost:80/x.png", "", "", false], // IP=127.0.0.1, Port=80 (specific port overrides http scheme). + ["https://localhost/x.png", "", "", false], // IP=127.0.0.1, Port=443 (port inferred from https). + ["http://localhost:443/x.png", "", "", false], // IP=127.0.0.1, Port=443 (specific port overrides http scheme). + ["localhost/x.png", "", "", false], // IP=127.0.0.1, Port=80 (port inferred from http fallback). + ["localhost:443/x.png", "", "", false], // IP=127.0.0.1, Port=443 (port hard specified, despite http fallback). + ["http://127.0.0.1/x.png", "", "", false], // IP=127.0.0.1, Port=80 (port inferred from http). + ["127.0.0.1/x.png", "", "", false], // IP=127.0.0.1, Port=80 (port inferred from http fallback). + ["http://localhost:8080/x.png", "", "", false], // IP=127.0.0.1, Port=8080 (port hard specified). + ["http://192.168.1.10/x.png", "", "", false], // IP=192.168.1.10, Port=80 (port inferred from http). + ["https://192.168.1.10/x.png", "", "", false], // IP=192.168.1.10, Port=443 (port inferred from https). + ["http://sub.example.com/x.png", "", "", false], // IP=::1, Port = 80 (port inferred from http). + ["http://s-1.d-1.com/x.png", "", "", false], // IP=::1, Port = 80 (port inferred from http). + + // Test set using domain name filters but with all ports allowed (empty). + ["http://localhost/x.png", "localhost", "", true], + ["localhost/x.png", "localhost", "", true], + ["localhost:0/x.png", "localhost", "", true], + ["ftp://localhost/x.png", "localhost", "", true], + ["http://sub.example.com/x.png", "localhost", "", false], + ["http://example.com/x.png", "example.com", "", true], + ["http://sub.example.com/x.png", "example.com", "", false], + + // Test set using wildcard domain name filters but with all ports allowed (empty). + ["http://sub.example.com/x.png", "*.com", "", true], + ["http://example.com/x.png", "*.example.com", "", false], + ["http://sub.example.com/x.png", "*.example.com", "", true], + ["http://sub.example.com/x.png", "*.sub.example.com", "", false], + ["http://sub.example.com/x.png", "*.example", "", false], + + // Test set using IP address filters but with all ports allowed (empty). + ["http://localhost/x.png", "127.0.0.1", "", true], + ["http://127.0.0.1/x.png", "127.0.0.1", "", true], + ["http://sub.example.com", "127.0.0.1", "", false], + + // Test set using CIDR IP range filters but with all ports allowed (empty). + ["http://localhost/x.png", "127.0.0.0/24", "", true], + ["http://127.0.0.1/x.png", "127.0.0.0/24", "", true], + ["http://sub.example.com", "127.0.0.0/24", "", false], + + // Test set using last-group range filters but with all ports allowed (empty). + ["http://localhost/x.png", "127.0.0.0-30", "", true], + ["http://127.0.0.1/x.png", "127.0.0.0-30", "", true], + ["http://sub.example.com", "127.0.0.0/24", "", false], + + // Test set using port filters but with all hosts allowed (empty). + ["http://localhost/x.png", "", "80\n443", false], + ["http://localhost:80/x.png", "", "80\n443", false], + ["https://localhost/x.png", "", "80\n443", false], + ["http://localhost:443/x.png", "", "80\n443", false], + ["http://sub.example.com:8080/x.png", "", "80\n443", true], + ["http://sub.example.com:-80/x.png", "", "80\n443", true], + ["http://sub.example.com:aaa/x.png", "", "80\n443", true], + + // Test set using port filters and hosts filters. + ["http://localhost/x.png", "127.0.0.1", "80\n443", true], + ["http://127.0.0.1/x.png", "127.0.0.1", "80\n443", true], + ["http://sub.example.com", "127.0.0.1", "80\n443", false], + + // Note on testing URLs using IPv6 notation: + // At present, the curl_security_helper class doesn't support IPv6 url notation. + // E.g. http://[ad34::dddd]:port/resource + // This is because it uses clean_param(x, PARAM_URL) as part of parsing, which won't validate urls having IPv6 notation. + // The underlying IPv6 address and range support is in place, however, so if clean_param is changed in future, + // please add the following test sets. + // 1. ["http://[::1]/x.png", "", "", false] + // 2. ["http://[::1]/x.png", "::1", "", true] + // 3. ["http://[::1]/x.png", "::1/64", "", true] + // 4. ["http://[fe80::dddd]/x.png", "fe80::cccc-eeee", "", true] + // 5. ["http://[fe80::dddd]/x.png", "fe80::dddd/128", "", true]. + ]; + } + + /** + * Test for \core\files\curl_security_helper->is_enabled(). + * + * @param string $blockedhosts the list of blocked hosts. + * @param string $allowedports the list of allowed ports. + * @param bool $expected the expected result. + * @dataProvider curl_security_settings_data_provider + */ + public function test_curl_security_helper_is_enabled($blockedhosts, $allowedports, $expected) { + $this->resetAfterTest(true); + $helper = new \core\files\curl_security_helper(); + set_config('curlsecurityblockedhosts', $blockedhosts); + set_config('curlsecurityallowedport', $allowedports); + $this->assertEquals($expected, $helper->is_enabled()); + } + + /** + * Data provider for test_curl_security_helper_is_enabled(). + * + * @return array + */ + public function curl_security_settings_data_provider() { + // Format: blocked hosts, allowed ports, expected result. + return [ + ["", "", false], + ["127.0.0.1", "", true], + ["localhost", "", true], + ["127.0.0.0/24\n192.0.0.0/24", "", true], + ["", "80\n443", true], + ]; + } + + /** + * Test for \core\files\curl_security_helper::host_is_blocked(). + * + * @param string $host the host to validate. + * @param string $blockedhosts the list of blocked hosts. + * @param bool $expected the expected result. + * @dataProvider curl_security_host_data_provider + */ + public function test_curl_security_helper_host_is_blocked($host, $blockedhosts, $expected) { + $this->resetAfterTest(true); + $helper = new \core\files\curl_security_helper(); + set_config('curlsecurityblockedhosts', $blockedhosts); + $this->assertEquals($expected, phpunit_util::call_internal_method($helper, 'host_is_blocked', [$host], + '\core\files\curl_security_helper')); + } + + /** + * Data provider for test_curl_security_helper_host_is_blocked(). + * + * @return array + */ + public function curl_security_host_data_provider() { + return [ + // IPv4 hosts. + ["127.0.0.1", "127.0.0.1", true], + ["127.0.0.1", "127.0.0.0/24", true], + ["127.0.0.1", "127.0.0.0-40", true], + ["", "127.0.0.0/24", false], + + // IPv6 hosts. + // Note: ["::", "::", true], - should match but 'address_in_subnet()' has trouble with fully collapsed IPv6 addresses. + ["::1", "::1", true], + ["::1", "::0-cccc", true], + ["::1", "::0/64", true], + ["FE80:0000:0000:0000:0000:0000:0000:0000", "fe80::/128", true], + ["fe80::eeee", "fe80::ddde/64", true], + ["fe80::dddd", "fe80::cccc-eeee", true], + ["fe80::dddd", "fe80::ddde-eeee", false], + + // Domain name hosts. + ["example.com", "example.com", true], + ["sub.example.com", "example.com", false], + ["example.com", "*.com", true], + ["example.com", "*.example.com", false], + ["sub.example.com", "*.example.com", true], + ["sub.sub.example.com", "*.example.com", true], + ["sub.example.com", "*example.com", false], + ["sub.example.com", "*.example", false], + + // International domain name hosts. + ["xn--nw2a.xn--j6w193g", "xn--nw2a.xn--j6w193g", true], // The domain 見.香港 is ace-encoded to xn--nw2a.xn--j6w193g. + ]; + } + + /** + * Test for \core\files\curl_security_helper->port_is_blocked(). + * + * @param int|string $port the port to validate. + * @param string $allowedports the list of allowed ports. + * @param bool $expected the expected result. + * @dataProvider curl_security_port_data_provider + */ + public function test_curl_security_helper_port_is_blocked($port, $allowedports, $expected) { + $this->resetAfterTest(true); + $helper = new \core\files\curl_security_helper(); + set_config('curlsecurityallowedport', $allowedports); + $this->assertEquals($expected, phpunit_util::call_internal_method($helper, 'port_is_blocked', [$port], + '\core\files\curl_security_helper')); + } + + /** + * Data provider for test_curl_security_helper_port_is_blocked(). + * + * @return array + */ + public function curl_security_port_data_provider() { + return [ + ["", "80\n443", true], + [" ", "80\n443", true], + ["-1", "80\n443", true], + [-1, "80\n443", true], + ["n", "80\n443", true], + [0, "80\n443", true], + ["0", "80\n443", true], + [8080, "80\n443", true], + ["8080", "80\n443", true], + ["80", "80\n443", false], + [80, "80\n443", false], + [443, "80\n443", false], + [0, "", true], // Port 0 and below are always invalid, even when the admin hasn't set whitelist entries. + [-1, "", true], // Port 0 and below are always invalid, even when the admin hasn't set whitelist entries. + [null, "", true], // Non-string, non-int values are invalid. + ]; + } + + /** + * Test for \core\files\curl_security_helper::get_blocked_url_string(). + */ + public function test_curl_security_helper_get_blocked_url_string() { + $helper = new \core\files\curl_security_helper(); + $this->assertEquals(get_string('curlsecurityurlblocked', 'admin'), $helper->get_blocked_url_string()); + } +} From 605bab84ce47ddb198647055bd48ed81476711ef Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 8 Nov 2016 14:41:18 +0800 Subject: [PATCH 3/3] MDL-48498 core_files: Add security helper to curl class Allow injection of security_helper objects into curl, but default to a core helper tied to http security settings values. --- lib/filelib.php | 78 ++++++++++++++++++++++++++++++++++---- lib/tests/filelib_test.php | 41 ++++++++++++++++++++ 2 files changed, 111 insertions(+), 8 deletions(-) diff --git a/lib/filelib.php b/lib/filelib.php index e2f19c94fe073..9b1e02c443ca4 100644 --- a/lib/filelib.php +++ b/lib/filelib.php @@ -2789,6 +2789,10 @@ class curl { private $cookie = false; /** @var bool tracks multiple headers in response - redirect detection */ private $responsefinished = false; + /** @var security helper class, responsible for checking host/ports against blacklist/whitelist entries.*/ + private $securityhelper; + /** @var bool ignoresecurity a flag which can be supplied to the constructor, allowing security to be bypassed. */ + private $ignoresecurity; /** * Curl constructor. @@ -2799,6 +2803,8 @@ class curl { * cookie: (string) path to cookie file, false if none * cache: (bool) use cache * module_cache: (string) type of cache + * securityhelper: (\core\files\curl_security_helper_base) helper object providing URL checking for requests. + * ignoresecurity: (bool) set true to override and ignore the security helper when making requests. * * @param array $settings */ @@ -2863,6 +2869,14 @@ public function __construct($settings = array()) { if (!isset($this->emulateredirects)) { $this->emulateredirects = ini_get('open_basedir'); } + + // Curl security setup. Allow injection of a security helper, but if not found, default to the core helper. + if (isset($settings['securityhelper']) && $settings['securityhelper'] instanceof \core\files\curl_security_helper_base) { + $this->set_security($settings['securityhelper']); + } else { + $this->set_security(new \core\files\curl_security_helper()); + } + $this->ignoresecurity = isset($settings['ignoresecurity']) ? $settings['ignoresecurity'] : false; } /** @@ -3215,6 +3229,29 @@ public function download($requests, $options = array()) { return $this->multi($requests, $options); } + /** + * Returns the current curl security helper. + * + * @return \core\files\curl_security_helper instance. + */ + public function get_security() { + return $this->securityhelper; + } + + /** + * Sets the curl security helper. + * + * @param \core\files\curl_security_helper $securityobject instance/subclass of the base curl_security_helper class. + * @return bool true if the security helper could be set, false otherwise. + */ + public function set_security($securityobject) { + if ($securityobject instanceof \core\files\curl_security_helper) { + $this->securityhelper = $securityobject; + return true; + } + return false; + } + /** * Multi HTTP Requests * This function could run multi-requests in parallel. @@ -3264,6 +3301,20 @@ protected function multi($requests, $options = array()) { return $results; } + /** + * Helper function to reset the request state vars. + * + * @return void. + */ + protected function reset_request_state_vars() { + $this->info = array(); + $this->error = ''; + $this->errno = 0; + $this->response = array(); + $this->rawresponse = array(); + $this->responsefinished = false; + } + /** * Single HTTP Request * @@ -3272,20 +3323,22 @@ protected function multi($requests, $options = array()) { * @return bool */ protected function request($url, $options = array()) { + // Reset here so that the data is valid when result returned from cache, or if we return due to a blacklist hit. + $this->reset_request_state_vars(); + + // If curl security is enabled, check the URL against the blacklist before calling curl_exec. + // Note: This will only check the base url. In the case of redirects, the blacklist is also after the curl_exec. + if (!$this->ignoresecurity && $this->securityhelper->url_is_blocked($url)) { + $this->error = $this->securityhelper->get_blocked_url_string(); + return $this->error; + } + // Set the URL as a curl option. $this->setopt(array('CURLOPT_URL' => $url)); // Create curl instance. $curl = curl_init(); - // Reset here so that the data is valid when result returned from cache. - $this->info = array(); - $this->error = ''; - $this->errno = 0; - $this->response = array(); - $this->rawresponse = array(); - $this->responsefinished = false; - $this->apply_opt($curl, $options); if ($this->cache && $ret = $this->cache->get($this->options)) { return $ret; @@ -3297,6 +3350,15 @@ protected function request($url, $options = array()) { $this->errno = curl_errno($curl); // Note: $this->response and $this->rawresponse are filled by $hits->formatHeader callback. + // In the case of redirects (which curl blindly follows), check the post-redirect URL against the blacklist entries too. + if (intval($this->info['redirect_count']) > 0 && !$this->ignoresecurity + && $this->securityhelper->url_is_blocked($this->info['url'])) { + $this->reset_request_state_vars(); + $this->error = $this->securityhelper->get_blocked_url_string(); + curl_close($curl); + return $this->error; + } + if ($this->emulateredirects and $this->options['CURLOPT_FOLLOWLOCATION'] and $this->info['http_code'] != 200) { $redirects = 0; diff --git a/lib/tests/filelib_test.php b/lib/tests/filelib_test.php index 4b9a2bf1882e6..1c144535855bf 100644 --- a/lib/tests/filelib_test.php +++ b/lib/tests/filelib_test.php @@ -218,6 +218,47 @@ public function test_curl_basics() { $this->assertSame(0, $curl->get_errno()); } + /** + * Test a curl basic request with security enabled. + */ + public function test_curl_basics_with_security_helper() { + $this->resetAfterTest(); + + // Test a request with a basic hostname filter applied. + $testhtml = $this->getExternalTestFileUrl('/test.html'); + $url = new moodle_url($testhtml); + $host = $url->get_host(); + set_config('curlsecurityblockedhosts', $host); // Blocks $host. + + // Create curl with the default security enabled. We expect this to be blocked. + $curl = new curl(); + $contents = $curl->get($testhtml); + $expected = $curl->get_security()->get_blocked_url_string(); + $this->assertSame($expected, $contents); + $this->assertSame(0, $curl->get_errno()); + + // Now, create a curl using the 'ignoresecurity' override. + // We expect this request to pass, despite the admin setting having been set earlier. + $curl = new curl(['ignoresecurity' => true]); + $contents = $curl->get($testhtml); + $this->assertSame('47250a973d1b88d9445f94db4ef2c97a', md5($contents)); + $this->assertSame(0, $curl->get_errno()); + + // Now, try injecting a mock security helper into curl. This will override the default helper. + $mockhelper = $this->getMockBuilder('\core\files\curl_security_helper')->getMock(); + + // Make the mock return a different string. + $mockhelper->expects($this->any())->method('get_blocked_url_string')->will($this->returnValue('You shall not pass')); + + // And make the mock security helper block all URLs. This helper instance doesn't care about config. + $mockhelper->expects($this->any())->method('url_is_blocked')->will($this->returnValue(true)); + + $curl = new curl(['securityhelper' => $mockhelper]); + $contents = $curl->get($testhtml); + $this->assertSame('You shall not pass', $curl->get_security()->get_blocked_url_string()); + $this->assertSame($curl->get_security()->get_blocked_url_string(), $contents); + } + public function test_curl_redirects() { global $CFG;