From 5068469f6fb8d4d99b24b55d169a8f5da45d66d3 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 6527cbc49262a014b1db9f753d39b00d1c056944 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 cbf9dfbd8dcfc60a6e677b475affb38598beec31 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 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); From 986a6f6f404ef17c60571c99bfd4a0b46be072ec 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 dc19dd61342ddcf928a5159fe90df21c4910f9e5 Mon Sep 17 00:00:00 2001 From: Michael Hawkins Date: Tue, 27 Jul 2021 23:09:09 +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 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: