From c619cd1425b2db410b86ee0eaf228dd66522074a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Thu, 22 Jul 2021 23:10:11 +0200 Subject: [PATCH 1/5] MDL-72203 curl: Revert original fix of redirects to blocked URLs This reverts the original fix introduced in MDL-71916. It introduced an extra native cURL call inside curl_security_helper to check if the given URL triggers a redirect to a blocked URL or not. Shortly after the release, a couple of regressions were reported as a result of the integrated solution. It was agreed to revert the fix and progress with implementing an alternative approach. --- lib/classes/files/curl_security_helper.php | 28 +++------------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/lib/classes/files/curl_security_helper.php b/lib/classes/files/curl_security_helper.php index a7b59f5f9b6..8388a1317ee 100644 --- a/lib/classes/files/curl_security_helper.php +++ b/lib/classes/files/curl_security_helper.php @@ -60,10 +60,9 @@ 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. * @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) { // If no config data is present, then all hosts/ports are allowed. if (!$this->is_enabled()) { return false; @@ -86,30 +85,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; } From 6e5454780d2f5f5ad03a419e12a48d091b161321 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Thu, 22 Jul 2021 23:20:14 +0200 Subject: [PATCH 2/5] MDL-72203 curl: Warn if someone actually started to use $maxredirects The new parameter of curl_security_helper::url_is_blocked() introduced in MDL-71916 became part of the API. Even if we reverted it quickly, someone can use a released Moodle version that has that parameter in place. For that reason and also to avoid potential troubles in the future (e.g. when yet another argument would be added to this method), we need to make it clear that the second parameter of this method should never be used again. Poor $maxredirects, you did not live long with us. Oh well. --- lib/classes/files/curl_security_helper.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/classes/files/curl_security_helper.php b/lib/classes/files/curl_security_helper.php index 8388a1317ee..276b7aa447a 100644 --- a/lib/classes/files/curl_security_helper.php +++ b/lib/classes/files/curl_security_helper.php @@ -60,9 +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 $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) { + 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; From 92b066bdafac71d874f34be1777868807e4832f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Thu, 22 Jul 2021 23:28:23 +0200 Subject: [PATCH 3/5] MDL-72203 curl: Check each URL in redirect chain to see if it is blocked The security problem here was that only the first and the last URL in the redirect chain was checked by the security helper. This patch forces the curl wrapper to always emulate cURL redirects and check every redirect URL in the chain before actually visiting it. --- lib/filelib.php | 48 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/lib/filelib.php b/lib/filelib.php index 18e8eee3589..cd050b1cd12 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); From 6b558e9be827e7bbf2d796c79a6e65870ad528f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 23 Jul 2021 01:04:06 +0200 Subject: [PATCH 4/5] MDL-72203 curl: Remove duplicate unit tests for emulated redirects Before, we had each redirect test duplicated: one for the native redirects via native cURL, second for our emulated implementation. Now all redirects are always emulated so there is no need to have them tested twice. --- lib/tests/filelib_test.php | 105 +++++-------------------------------- 1 file changed, 12 insertions(+), 93 deletions(-) diff --git a/lib/tests/filelib_test.php b/lib/tests/filelib_test.php index fd7a7b98b9b..23270ea0a6a 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)); @@ -375,15 +349,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 +361,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 +377,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 +391,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() { From 1d7c563f79e4a91fefb8855e4c14ba327b029596 Mon Sep 17 00:00:00 2001 From: Michael Hawkins Date: Tue, 27 Jul 2021 22:39:44 +0800 Subject: [PATCH 5/5] MDL-72203 curl: Improve redirect unit testing and update upgrade.txt lib/upgrade.txt was updated to reflect the fact that all cURL redirects will be emulated. --- lib/tests/filelib_test.php | 33 +++++++++++++++++++++++++++++++++ lib/upgrade.txt | 4 ++++ 2 files changed, 37 insertions(+) diff --git a/lib/tests/filelib_test.php b/lib/tests/filelib_test.php index 23270ea0a6a..86e958a0449 100644 --- a/lib/tests/filelib_test.php +++ b/lib/tests/filelib_test.php @@ -337,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'); diff --git a/lib/upgrade.txt b/lib/upgrade.txt index fe23f87e111..0bd56bc577a 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -57,6 +57,10 @@ information provided here is intended especially for developers. which is needed to store id of completion record on successful update which is later beeing used by completion_info::internal_set_data() to reaggregate completions that have been marked for instant course completion. +=== 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: