diff -u b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFileStorage.php b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFileStorage.php --- b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFileStorage.php +++ b/core/lib/Drupal/Component/PhpStorage/MTimeProtectedFileStorage.php @@ -37,6 +37,13 @@ protected $secret; /** + * Faster but less secure reads. + * + * @var bool + */ + protected $fast; + + /** * Constructs this MTimeProtectedLoader object. * * @param $configuration @@ -44,6 +51,8 @@ * ignored): * - directory: The directory where the files should be stored. * - secret: A cryptographically hard to guess secret string. + * - fast: A boolean. If TRUE, the file mtime is not compared to the + * directory mtime on load, making it faster but less secure. * @param $bin * The storage bin. Multiple storage objects can be instantiated with the * same configuration, but for different bins. @@ -51,6 +60,33 @@ public function __construct(array $configuration, $bin) { parent::__construct($configuration, $bin); $this->secret = $configuration['secret']; + $this->fast = !empty($configuration['fast']); + } + + /** + * Implements Drupal\Component\PhpStorage\PhpStorageInterface::exists(). + */ + public function load($name) { + $filename = $this->getFullPath($name, $directory, $directory_mtime); + // While file_exists() and filemtim() uses the same PHP stat() cache, + // include_once doesn't so this adds one additional filesystem call. It is + // very likely, however, that the filesystem has its own metadata cache and + // so no additional I/O will actually happen. This behaviour can be + // switched off by setting the 'fast' key to TRUE in $configuration passed + // to __construct(). + if ($this->fast || (file_exists($filename) && filemtime($filename) <= $directory_mtime)) { + // Inline parent::load() to avoid an expensive getFullPath() call. + return (@include_once $filename) !== FALSE; + } + return FALSE; + } + + /** + * Implements Drupal\Component\PhpStorage\PhpStorageInterface::exists(). + */ + public function exists($name) { + $filename = $this->getFullPath($name, $directory, $directory_mtime); + return file_exists($filename) && ($this->fast || filemtime($filename) <= $directory_mtime); } /** @@ -177,7 +213,7 @@ * (optional) The mtime of $directory. Can be passed to avoid an extra * filesystem call when the mtime of the directory is already known. */ - protected function getFullPath($name, $directory = NULL, $directory_mtime = NULL) { + protected function getFullPath($name, &$directory = NULL, &$directory_mtime = NULL) { if (!isset($directory)) { $directory = $this->getContainingDirectoryFullPath($name); } diff -u b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/FileStorageTest.php b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/FileStorageTest.php --- b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/FileStorageTest.php +++ b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/FileStorageTest.php @@ -45,25 +45,24 @@ } /** - * Tests writing with one class and readon with another. + * Tests writing with one class and reading with another. */ function testReadOnly() { $php = drupal_php_storage('simpletest'); $name = $this->randomName() . '/' . $this->randomName() . '.php'; - // Find a function name that doesn't exist. + // Find a global that doesn't exist. do { $random = mt_rand(10000, 100000); - $function = 'test' . $random; - } while (function_exists($function)); + } while (isset($GLOBALS[$random])); // Write out a PHP file and ensure it's successfully loaded. - $code = "save($name, $code); $this->assertIdentical($success, TRUE); $php_read = drupal_php_storage('readonly'); $php_read->load($name); - $this->assertIdentical($function(), $random); + $this->assertTrue($GLOBALS[$random]); // If the file was successfully loaded, it must also exist, but ensure the // exists() method returns that correctly. diff -u b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/MTimeProtectedFileStorageTest.php b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/MTimeProtectedFileStorageTest.php --- b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/MTimeProtectedFileStorageTest.php +++ b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/MTimeProtectedFileStorageTest.php @@ -49,7 +49,8 @@ $php->save($name, 'secret . filemtime($expected_directory)) . '.php'; + $directory_mtime = filemtime($expected_directory); + $expected_filename = $expected_directory . '/' . hash_hmac('sha256', $name, $this->secret . $directory_mtime) . '.php'; // Ensure the file exists and that it and the containing directory have // minimal permissions. fileperms() can return high bits unrelated to @@ -73,53 +74,41 @@ // vulnerable to many other attack vectors. sleep(1); - $hacked = FALSE; - $untrusted_code = "assertIdentical(file_get_contents($expected_filename), $untrusted_code); - $php->load($name); - // If the untrusted code was included, $hacked would be re-assigned to FALSE. - $this->assertIdentical($hacked, FALSE); - $this->assertIdentical($php->exists($name), FALSE); - } - - /** - * Tests the vulnerability of the MTimeProtectedFileStorage implementation. It - * does not protet against overwritiing a file. - */ - function testVulnerability() { - $php = drupal_php_storage('simpletest'); - $name = 'simpletest.php'; - $php->save($name, 'secret . filemtime($expected_directory)) . '.php'; - - // Ensure the file exists and that it and the containing directory have - // minimal permissions. fileperms() can return high bits unrelated to - // permissions, so mask with 0777. - $this->assertTrue(file_exists($expected_filename)); - $this->assertIdentical(fileperms($expected_filename) & 0777, 0400); - $this->assertIdentical(fileperms($expected_directory) & 0777, 0100); - - // Ensure that if the file is replaced with an untrusted one (due to a - // direct replacedment), it does get loaded. - sleep(1); - $hacked = FALSE; - $untrusted_code = "assertIdentical(file_get_contents($expected_filename), $untrusted_code); - $php->load($name); - // If the untrusted code was included, $hacked would be re-assigned to FALSE. - $this->assertIdentical($hacked, FALSE); - $this->assertIdentical($php->exists($name), TRUE); + // Run four tests: + // - write the file while keeping the directory mtime; fast = FALSE. + // - write the file while keeping the directory mtime; fast = TRUE. + // - alter the directory mtime; fast = FALSE. + // - alter the directory mtime; fast = TRUE. + // The only one that will actually load the hacked file is the second test. + for ($i = 0; $i < 4; $i++) { + $fast = $i % 2; + $GLOBALS['conf']['php_storage']['simpletest']['fast'] = $fast; + drupal_static_reset('drupal_php_storage'); + $php = drupal_php_storage('simpletest'); + $GLOBALS['hacked'] = FALSE; + $untrusted_code = "= 2) { + // Now try to write the file in such a way that the directory mtime + // changes and invalidates the hash. + file_put_contents($expected_filename . '.tmp', $untrusted_code); + rename($expected_filename . '.tmp', $expected_filename); + } + else { + // On the first two tries do not change the directory mtime but the + // filemtime is now larger than the directory mtime. Unless $fast is + // set, this causes the file not to be found or loaded. + file_put_contents($expected_filename, $untrusted_code); + } + chmod($expected_filename, 0400); + chmod($expected_directory, 0100); + $this->assertIdentical(file_get_contents($expected_filename), $untrusted_code); + $expected = $i == 1; + // The file only exists and loads when $fast is set. + $this->assertIdentical($php->exists($name), $expected); + $this->assertIdentical($php->load($name), $expected); + // If the untrusted code was included, $hacked would be re-assigned to TRUE. + $this->assertIdentical($GLOBALS['hacked'], $expected); + } } } diff -u b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/PhpStorageTestBase.php b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/PhpStorageTestBase.php --- b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/PhpStorageTestBase.php +++ b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/PhpStorageTestBase.php @@ -20,18 +20,17 @@ public function assertCRUD($php) { $name = $this->randomName() . '/' . $this->randomName() . '.php'; - // Find a function name that doesn't exist. + // Find a global that doesn't exist. do { $random = mt_rand(10000, 100000); - $function = 'test' . $random; - } while (function_exists($function)); + } while (isset($GLOBALS[$random])); // Write out a PHP file and ensure it's successfully loaded. - $code = "save($name, $code); $this->assertIdentical($success, TRUE); $php->load($name); - $this->assertIdentical($function(), $random); + $this->assertTrue($GLOBALS[$random]); // If the file was successfully loaded, it must also exist, but ensure the // exists() method returns that correctly.