Merge branch 'MDL-72203-master-2' of git://github.com/mickhawkins/moodle

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.
+            // Check the host and port against the allow/block entries.
-            if ($this->host_is_blocked($parsed['host']) || $this->port_is_blocked($parsed['port']) || $maxredirects < 1) {
+            return $this->host_is_blocked($parsed['host']) || $this->port_is_blocked($parsed['port']);
-                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);
         }
-
         return true;
     }
 

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)) {
+        // All redirects are performed at PHP level now and each one is checked against blocked URLs rules. We do not
-            $this->emulateredirects = ini_get('open_basedir');
+        // 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) {
+            if ($name === 'CURLOPT_FOLLOWLOCATION') {
-                // The redirects are emulated elsewhere.
+                // 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']);
+            // For security reasons we do not allow the cURL handle to follow redirects on its own.
-            if (!is_null($urlisblocked)) {
+            // See setting CURLOPT_FOLLOWLOCATION in {@see self::apply_opt()} method.
-                $this->reset_request_state_vars();
+            throw new coding_exception('Internal cURL handle should never follow redirects on its own!',
-                curl_close($curl);
+                'Reported number of redirects: ' . $this->info['redirect_count']);
-                return $urlisblocked;
-            }
         }
 
-        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);
 

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() {

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: