From f98f2c5896fb4594e20d4f0ee77988fddfd6319c Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 11 Jun 2024 20:12:01 +0200 Subject: [PATCH 1/2] [ticket/17301] Ensure reading invalid cache files is aborted PHPBB-17301 --- phpBB/phpbb/cache/driver/file.php | 40 +++++++-- tests/cache/file_driver_test.php | 140 +++++++++++++++++++++++++++++- 2 files changed, 170 insertions(+), 10 deletions(-) diff --git a/phpBB/phpbb/cache/driver/file.php b/phpBB/phpbb/cache/driver/file.php index a07b9a97010..768e314b182 100644 --- a/phpBB/phpbb/cache/driver/file.php +++ b/phpBB/phpbb/cache/driver/file.php @@ -330,6 +330,27 @@ function sql_save(\phpbb\db\driver\driver_interface $db, $query, $query_result, 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 @@ function _read($filename) if (!is_numeric($bytes) || ($bytes = (int) $bytes) === 0) { - // We cannot process the file without a valid number of bytes - // so we discard it - fclose($handle); - - $this->vars = $this->var_expires = array(); - $this->is_modified = false; - - $this->remove_file($file); + $this->cleanup_invalid_data_global($file, $handle); return false; } @@ -392,9 +406,17 @@ function _read($filename) } $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 diff --git a/tests/cache/file_driver_test.php b/tests/cache/file_driver_test.php index 10c9aec1824..cf36cda2bba 100644 --- a/tests/cache/file_driver_test.php +++ b/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 @@ protected function setUp(): void } $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,140 @@ protected function tearDown(): void parent::tearDown(); } + public function test_read_not_readable() + { + 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); From 9e15802805d356b02834348d74e7f65b121663b7 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Tue, 11 Jun 2024 20:29:23 +0200 Subject: [PATCH 2/2] [ticket/17301] Do not test unreadable files on windows PHPBB-17301 --- tests/cache/file_driver_test.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/cache/file_driver_test.php b/tests/cache/file_driver_test.php index cf36cda2bba..56a1b28073e 100644 --- a/tests/cache/file_driver_test.php +++ b/tests/cache/file_driver_test.php @@ -55,6 +55,11 @@ protected function tearDown(): void 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