Skip to content

Commit

Permalink
Merge pull request phpbb#6639 from marc1706/ticket/17301
Browse files Browse the repository at this point in the history 
[ticket/17301] Ensure reading invalid cache files is aborted
  • Loading branch information
@marc1706
marc1706 committed Jun 22, 2024
2 parents 99b07b8 + 9e15802 commit 15960a7
Show file tree
Hide file tree
Showing 2 changed files with 175 additions and 10 deletions.
40 changes: 31 additions & 9 deletions phpBB/phpbb/cache/driver/file.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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;
}
Expand All @@ -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
Expand Down
145 changes: 144 additions & 1 deletion tests/cache/file_driver_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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
Expand All @@ -49,6 +53,145 @@ protected function tearDown(): void
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);
Expand Down

0 comments on commit 15960a7

Please sign in to comment.