diff --git a/lib/classes/files/curl_security_helper.php b/lib/classes/files/curl_security_helper.php index a7b59f5f9b6..276b7aa447a 100644 --- a/lib/classes/files/curl_security_helper.php +++ b/lib/classes/files/curl_security_helper.php @@ -60,10 +60,15 @@ class curl_security_helper extends curl_security_helper_base { * could not be parsed, as well as those valid URLs which were found in the blocklist. * * @param string $urlstring the URL to check. - * @param int $maxredirects Optional number of maximum redirects to follow - prevents infinite recursion. + * @param int $notused There used to be an optional parameter $maxredirects for a short while here, not used any more. * @return bool true if the URL is blocked or invalid and false if the URL is not blocked. */ - public function url_is_blocked($urlstring, $maxredirects = 3) { + public function url_is_blocked($urlstring, $notused = null) { + + if ($notused !== null) { + debugging('The $maxredirects parameter of curl_security_helper::url_is_blocked() has been dropped!', DEBUG_DEVELOPER); + } + // If no config data is present, then all hosts/ports are allowed. if (!$this->is_enabled()) { return false; @@ -86,30 +91,9 @@ class curl_security_helper extends curl_security_helper_base { } if ($parsed['port'] && $parsed['host']) { - // Check the host and port against the allow/block entries, and that we have not run out of redirects. - if ($this->host_is_blocked($parsed['host']) || $this->port_is_blocked($parsed['port']) || $maxredirects < 1) { - return true; - } - - // Check if the host has a redirect in place, without following it. - $ch = curl_init(); - curl_setopt($ch, CURLOPT_URL, $urlstring); - curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); - curl_setopt($ch, CURLOPT_FOLLOWLOCATION, false); - - curl_exec($ch); - $curlinfo = curl_getinfo($ch); - $redirecturl = $curlinfo['redirect_url']; - - if (!$redirecturl) { - return false; - } - - // Recursively check redirects, until final URL checked, redirects to a blocked host/port, or has too many redirects. - $maxredirects--; - return $this->url_is_blocked($redirecturl, $maxredirects); + // Check the host and port against the allow/block entries. + return $this->host_is_blocked($parsed['host']) || $this->port_is_blocked($parsed['port']); } - return true; } diff --git a/lib/filelib.php b/lib/filelib.php index a64582be23f..b8e9adea76c 100644 --- a/lib/filelib.php +++ b/lib/filelib.php @@ -3079,7 +3079,7 @@ class curl { public $error; /** @var int error code */ public $errno; - /** @var bool use workaround for open_basedir restrictions, to be changed from unit tests only! */ + /** @var bool Perform redirects at PHP level instead of relying on native cURL functionality. Always true now. */ public $emulateredirects = null; /** @var array cURL options */ @@ -3176,9 +3176,13 @@ class curl { $this->proxy = false; } - if (!isset($this->emulateredirects)) { - $this->emulateredirects = ini_get('open_basedir'); - } + // All redirects are performed at PHP level now and each one is checked against blocked URLs rules. We do not + // want to let cURL naively follow the redirect chain and visit every URL for security reasons. Even when the + // caller explicitly wants to ignore the security checks, we would need to fall back to the original + // implementation and use emulated redirects if open_basedir is in effect to avoid the PHP warning + // "CURLOPT_FOLLOWLOCATION cannot be activated when in safe_mode or an open_basedir". So it is better to simply + // ignore this property and always handle redirects at this PHP wrapper level and not inside the native cURL. + $this->emulateredirects = true; // 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) { @@ -3487,8 +3491,8 @@ class curl { // Set options. foreach($this->options as $name => $val) { - if ($name === 'CURLOPT_FOLLOWLOCATION' and $this->emulateredirects) { - // The redirects are emulated elsewhere. + if ($name === 'CURLOPT_FOLLOWLOCATION') { + // All the redirects are emulated at PHP level. curl_setopt($curl, CURLOPT_FOLLOWLOCATION, 0); continue; } @@ -3705,7 +3709,11 @@ class curl { } } - // This will only check the base url. In the case of redirects, the blocking check is also after the curl_exec. + if (empty($this->emulateredirects)) { + // Just in case someone had tried to explicitly disable emulated redirects in legacy code. + debugging('Attempting to disable emulated redirects has no effect any more!', DEBUG_DEVELOPER); + } + $urlisblocked = $this->check_securityhelper_blocklist($url); if (!is_null($urlisblocked)) { return $urlisblocked; @@ -3728,17 +3736,14 @@ class curl { $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 list of blocked list too. if (intval($this->info['redirect_count']) > 0) { - $urlisblocked = $this->check_securityhelper_blocklist($this->info['url']); - if (!is_null($urlisblocked)) { - $this->reset_request_state_vars(); - curl_close($curl); - return $urlisblocked; - } + // For security reasons we do not allow the cURL handle to follow redirects on its own. + // See setting CURLOPT_FOLLOWLOCATION in {@see self::apply_opt()} method. + throw new coding_exception('Internal cURL handle should never follow redirects on its own!', + 'Reported number of redirects: ' . $this->info['redirect_count']); } - if ($this->emulateredirects and $this->options['CURLOPT_FOLLOWLOCATION'] and $this->info['http_code'] != 200) { + if ($this->options['CURLOPT_FOLLOWLOCATION'] && $this->info['http_code'] != 200) { $redirects = 0; while($redirects <= $this->options['CURLOPT_MAXREDIRS']) { @@ -3772,6 +3777,12 @@ class curl { if (isset($this->info['redirect_url'])) { if (preg_match('|^https?://|i', $this->info['redirect_url'])) { $redirecturl = $this->info['redirect_url']; + } else { + // Emulate CURLOPT_REDIR_PROTOCOLS behaviour which we have set to (CURLPROTO_HTTP | CURLPROTO_HTTPS) only. + $this->errno = CURLE_UNSUPPORTED_PROTOCOL; + $this->error = 'Redirect to a URL with unsuported protocol: ' . $this->info['redirect_url']; + curl_close($curl); + return $this->error; } } if (!$redirecturl) { @@ -3801,6 +3812,13 @@ class curl { } } + $urlisblocked = $this->check_securityhelper_blocklist($redirecturl); + if (!is_null($urlisblocked)) { + $this->reset_request_state_vars(); + curl_close($curl); + return $urlisblocked; + } + curl_setopt($curl, CURLOPT_URL, $redirecturl); $ret = curl_exec($curl); diff --git a/lib/tests/filelib_test.php b/lib/tests/filelib_test.php index fd7a7b98b9b..86e958a0449 100644 --- a/lib/tests/filelib_test.php +++ b/lib/tests/filelib_test.php @@ -262,7 +262,6 @@ class core_filelib_testcase extends advanced_testcase { public function test_curl_redirects() { global $CFG; - // Test full URL redirects. $testurl = $this->getExternalTestFileUrl('/test_redir.php'); $curl = new curl(); @@ -273,6 +272,7 @@ class core_filelib_testcase extends advanced_testcase { $this->assertSame(2, $curl->info['redirect_count']); $this->assertSame('done', $contents); + // All redirects are emulated now. Enabling "emulateredirects" explicitly does not have effect. $curl = new curl(); $curl->emulateredirects = true; $contents = $curl->get("$testurl?redir=2", array(), array('CURLOPT_MAXREDIRS'=>2)); @@ -282,6 +282,17 @@ class core_filelib_testcase extends advanced_testcase { $this->assertSame(2, $curl->info['redirect_count']); $this->assertSame('done', $contents); + // All redirects are emulated now. Attempting to disable "emulateredirects" explicitly causes warning. + $curl = new curl(); + $curl->emulateredirects = false; + $contents = $curl->get("$testurl?redir=2", array(), array('CURLOPT_MAXREDIRS' => 2)); + $response = $curl->getResponse(); + $this->assertDebuggingCalled('Attempting to disable emulated redirects has no effect any more!'); + $this->assertSame('200 OK', reset($response)); + $this->assertSame(0, $curl->get_errno()); + $this->assertSame(2, $curl->info['redirect_count']); + $this->assertSame('done', $contents); + // This test was failing for people behind Squid proxies. Squid does not // fully support HTTP 1.1, so converts things to HTTP 1.0, where the name // of the status code is different. @@ -301,21 +312,6 @@ class core_filelib_testcase extends advanced_testcase { $this->assertSame('', $contents); $curl = new curl(); - $curl->emulateredirects = true; - $contents = $curl->get("$testurl?redir=3", array(), array('CURLOPT_FOLLOWLOCATION'=>0)); - $response = $curl->getResponse(); - $this->assertSame($responsecode302, reset($response)); - $this->assertSame(0, $curl->get_errno()); - $this->assertSame(302, $curl->info['http_code']); - $this->assertSame('', $contents); - - $curl = new curl(); - $contents = $curl->get("$testurl?redir=2", array(), array('CURLOPT_MAXREDIRS'=>1)); - $this->assertSame(CURLE_TOO_MANY_REDIRECTS, $curl->get_errno()); - $this->assertNotEmpty($contents); - - $curl = new curl(); - $curl->emulateredirects = true; $contents = $curl->get("$testurl?redir=2", array(), array('CURLOPT_MAXREDIRS'=>1)); $this->assertSame(CURLE_TOO_MANY_REDIRECTS, $curl->get_errno()); $this->assertNotEmpty($contents); @@ -332,28 +328,6 @@ class core_filelib_testcase extends advanced_testcase { @unlink($tofile); $curl = new curl(); - $curl->emulateredirects = true; - $tofile = "$CFG->tempdir/test.html"; - @unlink($tofile); - $fp = fopen($tofile, 'w'); - $result = $curl->get("$testurl?redir=1", array(), array('CURLOPT_FILE'=>$fp)); - $this->assertTrue($result); - fclose($fp); - $this->assertFileExists($tofile); - $this->assertSame('done', file_get_contents($tofile)); - @unlink($tofile); - - $curl = new curl(); - $tofile = "$CFG->tempdir/test.html"; - @unlink($tofile); - $result = $curl->download_one("$testurl?redir=1", array(), array('filepath'=>$tofile)); - $this->assertTrue($result); - $this->assertFileExists($tofile); - $this->assertSame('done', file_get_contents($tofile)); - @unlink($tofile); - - $curl = new curl(); - $curl->emulateredirects = true; $tofile = "$CFG->tempdir/test.html"; @unlink($tofile); $result = $curl->download_one("$testurl?redir=1", array(), array('filepath'=>$tofile)); @@ -363,6 +337,39 @@ class core_filelib_testcase extends advanced_testcase { @unlink($tofile); } + /** + * Test that redirects to blocked hosts are blocked. + */ + public function test_curl_blocked_redirect() { + $this->resetAfterTest(); + + $testurl = $this->getExternalTestFileUrl('/test_redir.php'); + + // Block a host. + // Note: moodle.com is the URL redirected to when test_redir.php has the param extdest=1 set. + set_config('curlsecurityblockedhosts', 'moodle.com'); + + // Redirecting to a non-blocked host should resolve. + $curl = new curl(); + $contents = $curl->get("{$testurl}?redir=2"); + $response = $curl->getResponse(); + $this->assertSame('200 OK', reset($response)); + $this->assertSame(0, $curl->get_errno()); + + // Redirecting to the blocked host should fail. + $curl = new curl(); + $blockedstring = $curl->get_security()->get_blocked_url_string(); + $contents = $curl->get("{$testurl}?redir=1&extdest=1"); + $this->assertSame($blockedstring, $contents); + $this->assertSame(0, $curl->get_errno()); + + // Redirecting to the blocked host after multiple successful redirects should also fail. + $curl = new curl(); + $contents = $curl->get("{$testurl}?redir=3&extdest=1"); + $this->assertSame($blockedstring, $contents); + $this->assertSame(0, $curl->get_errno()); + } + public function test_curl_relative_redirects() { // Test relative location redirects. $testurl = $this->getExternalTestFileUrl('/test_relative_redir.php'); @@ -375,15 +382,6 @@ class core_filelib_testcase extends advanced_testcase { $this->assertSame(1, $curl->info['redirect_count']); $this->assertSame('done', $contents); - $curl = new curl(); - $curl->emulateredirects = true; - $contents = $curl->get($testurl); - $response = $curl->getResponse(); - $this->assertSame('200 OK', reset($response)); - $this->assertSame(0, $curl->get_errno()); - $this->assertSame(1, $curl->info['redirect_count']); - $this->assertSame('done', $contents); - // Test different redirect types. $testurl = $this->getExternalTestFileUrl('/test_relative_redir.php'); @@ -396,24 +394,6 @@ class core_filelib_testcase extends advanced_testcase { $this->assertSame('done', $contents); $curl = new curl(); - $curl->emulateredirects = true; - $contents = $curl->get("$testurl?type=301"); - $response = $curl->getResponse(); - $this->assertSame('200 OK', reset($response)); - $this->assertSame(0, $curl->get_errno()); - $this->assertSame(1, $curl->info['redirect_count']); - $this->assertSame('done', $contents); - - $curl = new curl(); - $contents = $curl->get("$testurl?type=302"); - $response = $curl->getResponse(); - $this->assertSame('200 OK', reset($response)); - $this->assertSame(0, $curl->get_errno()); - $this->assertSame(1, $curl->info['redirect_count']); - $this->assertSame('done', $contents); - - $curl = new curl(); - $curl->emulateredirects = true; $contents = $curl->get("$testurl?type=302"); $response = $curl->getResponse(); $this->assertSame('200 OK', reset($response)); @@ -430,24 +410,6 @@ class core_filelib_testcase extends advanced_testcase { $this->assertSame('done', $contents); $curl = new curl(); - $curl->emulateredirects = true; - $contents = $curl->get("$testurl?type=303"); - $response = $curl->getResponse(); - $this->assertSame('200 OK', reset($response)); - $this->assertSame(0, $curl->get_errno()); - $this->assertSame(1, $curl->info['redirect_count']); - $this->assertSame('done', $contents); - - $curl = new curl(); - $contents = $curl->get("$testurl?type=307"); - $response = $curl->getResponse(); - $this->assertSame('200 OK', reset($response)); - $this->assertSame(0, $curl->get_errno()); - $this->assertSame(1, $curl->info['redirect_count']); - $this->assertSame('done', $contents); - - $curl = new curl(); - $curl->emulateredirects = true; $contents = $curl->get("$testurl?type=307"); $response = $curl->getResponse(); $this->assertSame('200 OK', reset($response)); @@ -462,16 +424,6 @@ class core_filelib_testcase extends advanced_testcase { $this->assertSame(0, $curl->get_errno()); $this->assertSame(1, $curl->info['redirect_count']); $this->assertSame('done', $contents); - - $curl = new curl(); - $curl->emulateredirects = true; - $contents = $curl->get("$testurl?type=308"); - $response = $curl->getResponse(); - $this->assertSame('200 OK', reset($response)); - $this->assertSame(0, $curl->get_errno()); - $this->assertSame(1, $curl->info['redirect_count']); - $this->assertSame('done', $contents); - } public function test_curl_proxybypass() { diff --git a/lib/upgrade.txt b/lib/upgrade.txt index ecd7d399d91..947d050e383 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -1,6 +1,10 @@ This files describes API changes in core libraries and APIs, information provided here is intended especially for developers. +=== 3.11.2 === +* For security reasons, filelib has been updated so all requests now use emulated redirects. + For this reason, manually disabling emulateredirects will no longer have any effect (and will generate a debugging message). + === 3.11 === * PHPUnit has been upgraded to 9.5 (see MDL-71036 for details). That comes with a few changes: