Merge pull request #6639 from marc1706/ticket/17301

[ticket/17301] Ensure reading invalid cache files is aborted

phpBB/phpbb/cache/driver/file.php

@@ -330,6 +330,27 @@ class file extends \phpbb\cache\driver\base
 		return $query_result;
 	}
 
+	/**
+	 * Cleanup when loading invalid data global file
+	 *
+	 * @param string $file Filename
+	 * @param resource $handle
+	 *
+	 * @return void
+	 */
+	private function cleanup_invalid_data_global(string $file, $handle): void
+	{
+		if (is_resource($handle))
+		{
+			fclose($handle);
+		}
+
+		$this->vars = $this->var_expires = [];
+		$this->is_modified = false;
+
+		$this->remove_file($file);
+	}
+
 	/**
 	* Read cached data from a specified file
 	*
@@ -372,14 +393,7 @@ class file extends \phpbb\cache\driver\base
 
 				if (!is_numeric($bytes) || ($bytes = (int) $bytes) === 0)
 				{
-					// We cannot process the file without a valid number of bytes
+					$this->cleanup_invalid_data_global($file, $handle);
-					// so we discard it
-					fclose($handle);
-
-					$this->vars = $this->var_expires = array();
-					$this->is_modified = false;
-
-					$this->remove_file($file);
 
 					return false;
 				}
@@ -392,9 +406,17 @@ class file extends \phpbb\cache\driver\base
 				}
 
 				$var_name = substr(fgets($handle), 0, -1);
+				$data_length = $bytes - strlen($var_name);
+
+				if ($data_length <= 0)
+				{
+					$this->cleanup_invalid_data_global($file, $handle);
+
+					return false;
+				}
 
 				// Read the length of bytes that consists of data.
-				$data = fread($handle, $bytes - strlen($var_name));
+				$data = fread($handle, $data_length);
 				$data = @unserialize($data);
 
 				// Don't use the data if it was invalid

tests/cache/file_driver_test.php

@@ -17,6 +17,9 @@ class phpbb_cache_file_driver_test extends phpbb_cache_common_test_case
 {
 	private $cache_dir;
 
+	/** @var \phpbb\cache\driver\file */
+	private  $cache_file;
+
 	public function getDataSet()
 	{
 		return $this->createXMLDataSet(__DIR__ . '/fixtures/config.xml');
@@ -36,7 +39,8 @@ class phpbb_cache_file_driver_test extends phpbb_cache_common_test_case
 		}
 		$this->create_cache_dir();
 
-		$this->driver = new \phpbb\cache\driver\file($this->cache_dir);
+		$this->cache_file = new \phpbb\cache\driver\file($this->cache_dir);
+		$this->driver = $this->cache_file;
 	}
 
 	protected function tearDown(): void
@@ -49,6 +53,145 @@ class phpbb_cache_file_driver_test extends phpbb_cache_common_test_case
 		parent::tearDown();
 	}
 
+	public function test_read_not_readable()
+	{
+		if (strtolower(substr(PHP_OS, 0, 3)) === 'win')
+		{
+			$this->markTestSkipped('Unable to test unreadable files on Windows');
+		}
+
+		global $phpEx;
+
+		// Create file that is not readable
+		$this->assertTrue($this->cache_file->_write('unreadable', 'foo', time() + 86400));
+
+		$filename = "{$this->cache_dir}unreadable.$phpEx";
+		@chmod($filename, 0000);
+		$this->assertFalse($this->cache_file->_read('unreadable'));
+		@chmod($filename, 0600);
+		$this->assertNotFalse($this->cache_file->_read('unreadable'));
+	}
+
+	public function test_read_data_global_invalid()
+	{
+		global $phpEx;
+
+		$reflectionCacheVars = new \ReflectionProperty($this->cache_file, 'vars');
+		$reflectionCacheVars->setAccessible(true);
+		$reflectionCacheVars->setValue($this->cache_file, ['foo' => 'bar']);
+
+		$reflectionCacheVarExpires = new \ReflectionProperty($this->cache_file, 'var_expires');
+		$reflectionCacheVarExpires->setAccessible(true);
+		$reflectionCacheVarExpires->setValue($this->cache_file, ['foo' => time() + 86400]);
+
+		// Create file in invalid format
+		$this->assertTrue($this->cache_file->_write('data_global'));
+		$filename = "{$this->cache_dir}data_global.$phpEx";
+		$cache_data = file_get_contents($filename);
+		// Force negative read when retrieving data_global
+		$cache_data = str_replace("\n13\n", "\n1\n", $cache_data);
+		file_put_contents($filename, $cache_data);
+
+		$this->assertFalse($this->cache_file->_read('data_global'));
+	}
+
+	public function test_read_data_global_zero_bytes()
+	{
+		global $phpEx;
+
+		$reflectionCacheVars = new \ReflectionProperty($this->cache_file, 'vars');
+		$reflectionCacheVars->setAccessible(true);
+		$reflectionCacheVars->setValue($this->cache_file, ['foo' => 'bar']);
+
+		$reflectionCacheVarExpires = new \ReflectionProperty($this->cache_file, 'var_expires');
+		$reflectionCacheVarExpires->setAccessible(true);
+		$reflectionCacheVarExpires->setValue($this->cache_file, ['foo' => time() + 86400]);
+
+		// Create file in invalid format
+		$this->assertTrue($this->cache_file->_write('data_global'));
+		$filename = "{$this->cache_dir}data_global.$phpEx";
+		$cache_data = file_get_contents($filename);
+		// Force negative read when retrieving data_global
+		$cache_data = str_replace("\n13\n", "\n0\n", $cache_data);
+		file_put_contents($filename, $cache_data);
+
+		$this->assertFalse($this->cache_file->_read('data_global'));
+	}
+
+	public function test_read_data_global_hex_bytes()
+	{
+		global $phpEx;
+
+		$reflectionCacheVars = new \ReflectionProperty($this->cache_file, 'vars');
+		$reflectionCacheVars->setAccessible(true);
+		$reflectionCacheVars->setValue($this->cache_file, ['foo' => 'bar']);
+
+		$reflectionCacheVarExpires = new \ReflectionProperty($this->cache_file, 'var_expires');
+		$reflectionCacheVarExpires->setAccessible(true);
+		$reflectionCacheVarExpires->setValue($this->cache_file, ['foo' => time() + 86400]);
+
+		// Create file in invalid format
+		$this->assertTrue($this->cache_file->_write('data_global'));
+		$filename = "{$this->cache_dir}data_global.$phpEx";
+		$cache_data = file_get_contents($filename);
+		// Force negative read when retrieving data_global
+		$cache_data = str_replace("\n13\n", "\nA\n", $cache_data);
+		file_put_contents($filename, $cache_data);
+
+		$this->assertFalse($this->cache_file->_read('data_global'));
+	}
+
+	public function test_read_data_global_expired()
+	{
+		$reflectionCacheVars = new \ReflectionProperty($this->cache_file, 'vars');
+		$reflectionCacheVars->setAccessible(true);
+		$reflectionCacheVars->setValue($this->cache_file, ['foo' => 'bar']);
+
+		$reflectionCacheVarExpires = new \ReflectionProperty($this->cache_file, 'var_expires');
+		$reflectionCacheVarExpires->setAccessible(true);
+		$reflectionCacheVarExpires->setValue($this->cache_file, ['foo' => time() - 86400]);
+
+		// Create file in invalid format
+		$this->assertTrue($this->cache_file->_write('data_global'));
+
+		// Clear data
+		$reflectionCacheVars->setValue($this->cache_file, []);
+		$reflectionCacheVarExpires->setValue($this->cache_file, []);
+
+		$this->assertTrue($this->cache_file->_read('data_global'));
+
+		// Check data, should be empty
+		$this->assertEquals([], $reflectionCacheVars->getValue($this->cache_file));
+	}
+
+	public function test_read_data_global()
+	{
+		$reflectionCacheVars = new \ReflectionProperty($this->cache_file, 'vars');
+		$reflectionCacheVars->setAccessible(true);
+		$expectedVars = ['foo' => 'bar'];
+		$reflectionCacheVars->setValue($this->cache_file, $expectedVars);
+
+		$reflectionCacheVarExpires = new \ReflectionProperty($this->cache_file, 'var_expires');
+		$reflectionCacheVarExpires->setAccessible(true);
+		$expectedVarExpires = ['foo' => time() + 86400];
+		$reflectionCacheVarExpires->setValue($this->cache_file, $expectedVarExpires);
+
+		// Create file in invalid format
+		$this->assertTrue($this->cache_file->_write('data_global'));
+
+		// Clear data
+		$reflectionCacheVars->setValue($this->cache_file, []);
+		$reflectionCacheVarExpires->setValue($this->cache_file, []);
+		$this->assertEquals([], $reflectionCacheVars->getValue($this->cache_file));
+		$this->assertEquals([], $reflectionCacheVarExpires->getValue($this->cache_file));
+
+		$this->assertTrue($this->cache_file->_read('data_global'));
+
+		// Check data, should be empty
+		$this->assertEquals($expectedVars, $reflectionCacheVars->getValue($this->cache_file));
+		$this->assertEquals($expectedVarExpires, $reflectionCacheVarExpires->getValue($this->cache_file));
+	}
+
 	private function create_cache_dir()
 	{
 		$this->get_test_case_helpers()->makedirs($this->cache_dir);