From f775da3931864d486bd34d198a4b86b0bc9fc0ba Mon Sep 17 00:00:00 2001 From: El RIDO Date: Thu, 27 Aug 2015 21:41:21 +0200 Subject: [PATCH] fixing nasty deletion bug from #15, included unit tests to trigger it and reworked persistence classes to through exceptions rather to fail silently --- doc/README.md | 2 +- lib/persistence.php | 23 +++-- lib/serversalt.php | 39 ++++++-- lib/trafficlimiter.php | 1 + lib/zerobin.php | 5 +- lib/zerobin/data.php | 4 +- lib/zerobin/db.php | 18 ++-- tst/bootstrap.php | 40 ++++---- tst/serversalt.php | 100 +++++++++++++++++++ tst/vizhash16x16.php | 23 ++--- tst/zerobin/db.php | 213 ++++++++++++++++++++++++++++------------- 11 files changed, 334 insertions(+), 134 deletions(-) create mode 100644 tst/serversalt.php diff --git a/doc/README.md b/doc/README.md index a3baf68..6566958 100644 --- a/doc/README.md +++ b/doc/README.md @@ -10,7 +10,7 @@ Details about [installing phpDocumentor](http://phpdoc.org/docs/latest/getting-s can be found in its own documentation. Example for Debian and Ubuntu: - $ sudo aptitude install pear graphviz + $ sudo aptitude install php-pear graphviz $ sudo pear channel-discover pear.phpdoc.org $ sudo pear install phpdoc/phpDocumentor diff --git a/lib/persistence.php b/lib/persistence.php index 33840cf..ae0f2dd 100644 --- a/lib/persistence.php +++ b/lib/persistence.php @@ -67,7 +67,7 @@ abstract class persistence protected static function _exists($filename) { self::_initialize(); - return is_file(self::$_path . '/' . $filename); + return is_file(self::$_path . DIRECTORY_SEPARATOR . $filename); } /** @@ -75,23 +75,29 @@ abstract class persistence * * @access protected * @static + * @throws Exception * @return void */ protected static function _initialize() { // Create storage directory if it does not exist. - if (!is_dir(self::$_path)) mkdir(self::$_path, 0705); + if (!is_dir(self::$_path)) + if (!@mkdir(self::$_path)) + throw new Exception('unable to create directory ' . self::$_path, 10); // Create .htaccess file if it does not exist. - $file = self::$_path . '/.htaccess'; + $file = self::$_path . DIRECTORY_SEPARATOR . '.htaccess'; if (!is_file($file)) { - file_put_contents( + $writtenBytes = @file_put_contents( $file, 'Allow from none' . PHP_EOL . 'Deny from all'. PHP_EOL, LOCK_EX ); + if ($writtenBytes === false || $writtenBytes < 30) { + throw new Exception('unable to write to file ' . $file, 11); + } } } @@ -102,14 +108,17 @@ abstract class persistence * @static * @param string $filename * @param string $data + * @throws Exception * @return string */ protected static function _store($filename, $data) { self::_initialize(); - $file = self::$_path . '/' . $filename; - file_put_contents($file, $data, LOCK_EX); - chmod($file, 0705); + $file = self::$_path . DIRECTORY_SEPARATOR . $filename; + $writtenBytes = @file_put_contents($file, $data, LOCK_EX); + if ($writtenBytes === false || $writtenBytes < strlen($data)) { + throw new Exception('unable to write to file ' . $file, 13); + } return $file; } } diff --git a/lib/serversalt.php b/lib/serversalt.php index 312705c..c441bdb 100644 --- a/lib/serversalt.php +++ b/lib/serversalt.php @@ -47,12 +47,11 @@ class serversalt extends persistence } else // fallback to mt_rand() { - for($i = 0; $i < 16; ++$i) { + for($i = 0; $i < 256; ++$i) { $randomSalt .= base_convert(mt_rand(), 10, 16); } } - self::$_salt = $randomSalt; - return self::$_salt; + return $randomSalt; } /** @@ -60,21 +59,41 @@ class serversalt extends persistence * * @access public * @static + * @throws Exception * @return string */ public static function get() { - if (strlen(self::$_salt)) return self::$_salt; + if (strlen(self::$_salt)) return self::$_salt; $file = 'salt.php'; - if (!self::_exists($file)) { + if (self::_exists($file)) { + $items = explode('|', @file_get_contents(self::getPath($file))); + if (!is_array($items) || count($items) != 3) { + throw new Exception('unable to read file ' . self::getPath($file), 20); + } + self::$_salt = $items[1]; + } else { + self::$_salt = self::generate(); self::_store( - $file, - '' + $file, + '' ); } - $items = explode('|', file_get_contents(self::getPath($file))); - self::$_salt = $items[1]; - return $items[1]; + return self::$_salt; + } + + /** + * set the path + * + * @access public + * @static + * @param string $path + * @return void + */ + public static function setPath($path) + { + self::$_salt = ''; + parent::setPath($path); } } diff --git a/lib/trafficlimiter.php b/lib/trafficlimiter.php index e066eef..6d2b20c 100644 --- a/lib/trafficlimiter.php +++ b/lib/trafficlimiter.php @@ -47,6 +47,7 @@ class trafficlimiter extends persistence * @access public * @static * @param string $ip + * @throws Exception * @return bool */ public static function canPass($ip) diff --git a/lib/zerobin.php b/lib/zerobin.php index a1617f5..eebc32c 100644 --- a/lib/zerobin.php +++ b/lib/zerobin.php @@ -339,7 +339,7 @@ class zerobin // Generate the "delete" token. // The token is the hmac of the pasteid signed with the server salt. // The paste can be delete by calling http://myserver.com/zerobin/?pasteid=&deletetoken= - $deletetoken = hash_hmac('sha1', $dataid , serversalt::get()); + $deletetoken = hash_hmac('sha1', $dataid, serversalt::get()); // 0 = no error $this->_return_message(0, $dataid, array('deletetoken' => $deletetoken)); @@ -373,7 +373,8 @@ class zerobin } // Make sure token is valid. - if (filter::slow_equals($deletetoken, hash_hmac('sha1', $dataid , serversalt::get()))) + serversalt::setPath($this->_conf['traffic']['dir']); + if (!filter::slow_equals($deletetoken, hash_hmac('sha1', $dataid, serversalt::get()))) { $this->_error = 'Wrong deletion token. Paste was not deleted.'; return; diff --git a/lib/zerobin/data.php b/lib/zerobin/data.php index fa5cbe6..1cf043b 100644 --- a/lib/zerobin/data.php +++ b/lib/zerobin/data.php @@ -38,8 +38,8 @@ class zerobin_data extends zerobin_abstract { // if given update the data directory if ( - is_array($options) && - array_key_exists('dir', $options) + is_array($options) && + array_key_exists('dir', $options) ) self::$_dir = $options['dir'] . DIRECTORY_SEPARATOR; // if needed initialize the singleton if(!(self::$_instance instanceof zerobin_data)) { diff --git a/lib/zerobin/db.php b/lib/zerobin/db.php index 1382e8f..69d48f9 100644 --- a/lib/zerobin/db.php +++ b/lib/zerobin/db.php @@ -80,13 +80,6 @@ class zerobin_db extends zerobin_abstract array_key_exists('opt', $options) ) { - self::$_db = new PDO( - $options['dsn'], - $options['usr'], - $options['pwd'], - $options['opt'] - ); - // check if the database contains the required tables self::$_type = strtolower( substr($options['dsn'], 0, strpos($options['dsn'], ':')) @@ -132,9 +125,16 @@ class zerobin_db extends zerobin_abstract throw new Exception( 'PDO type ' . self::$_type . - ' is currently not supported.' + ' is currently not supported.', + 5 ); } + self::$_db = new PDO( + $options['dsn'], + $options['usr'], + $options['pwd'], + $options['opt'] + ); $statement = self::$_db->query($sql); $tables = $statement->fetchAll(PDO::FETCH_COLUMN, 0); @@ -266,7 +266,7 @@ class zerobin_db extends zerobin_abstract array($pasteid) ); if ( - array_key_exists($pasteid, self::$_cache) + array_key_exists($pasteid, self::$_cache) ) unset(self::$_cache[$pasteid]); } diff --git a/tst/bootstrap.php b/tst/bootstrap.php index 117a0aa..d2974c4 100644 --- a/tst/bootstrap.php +++ b/tst/bootstrap.php @@ -8,24 +8,24 @@ require PATH . 'lib/auto.php'; class helper { - public static function rmdir($path) - { - $path .= DIRECTORY_SEPARATOR; - $dir = dir($path); - while(false !== ($file = $dir->read())) { - if($file != '.' && $file != '..') { - if(is_dir($path . $file)) { - self::rmdir($path . $file); - } elseif(is_file($path . $file)) { - if(!@unlink($path . $file)) { - throw new Exception('Error deleting file "' . $path . $file . '".'); - } - } - } - } - $dir->close(); - if(!@rmdir($path)) { - throw new Exception('Error deleting directory "' . $path . '".'); - } - } + public static function rmdir($path) + { + $path .= DIRECTORY_SEPARATOR; + $dir = dir($path); + while(false !== ($file = $dir->read())) { + if($file != '.' && $file != '..') { + if(is_dir($path . $file)) { + self::rmdir($path . $file); + } elseif(is_file($path . $file)) { + if(!@unlink($path . $file)) { + throw new Exception('Error deleting file "' . $path . $file . '".'); + } + } + } + } + $dir->close(); + if(!@rmdir($path)) { + throw new Exception('Error deleting directory "' . $path . '".'); + } + } } \ No newline at end of file diff --git a/tst/serversalt.php b/tst/serversalt.php new file mode 100644 index 0000000..b9dd3a9 --- /dev/null +++ b/tst/serversalt.php @@ -0,0 +1,100 @@ +_path = PATH . 'data'; + if(!is_dir($this->_path)) mkdir($this->_path); + serversalt::setPath($this->_path); + + $this->_otherPath = $this->_path . DIRECTORY_SEPARATOR . 'foo'; + + $this->_invalidPath = $this->_path . DIRECTORY_SEPARATOR . 'bar'; + if(!is_dir($this->_invalidPath)) mkdir($this->_invalidPath); + $this->_invalidFile = $this->_invalidPath . DIRECTORY_SEPARATOR . 'salt.php'; + } + + public function tearDown() + { + /* Tear Down Routine */ + chmod($this->_invalidPath, 0700); + helper::rmdir($this->_path); + } + + public function testGeneration() + { + // generating new salt + serversalt::setPath($this->_path); + $salt = serversalt::get(); + require 'mcrypt_mock.php'; + $this->assertNotEquals($salt, serversalt::generate()); + + // try setting a different path and resetting it + serversalt::setPath($this->_otherPath); + $this->assertNotEquals($salt, serversalt::get()); + serversalt::setPath($this->_path); + $this->assertEquals($salt, serversalt::get()); + } + + /** + * @expectedException Exception + * @expectedExceptionCode 11 + */ + public function testPathShenanigans() + { + // try setting an invalid path + chmod($this->_invalidPath, 0000); + serversalt::setPath($this->_invalidPath); + serversalt::get(); + } + + /** + * @expectedException Exception + * @expectedExceptionCode 20 + */ + public function testFileRead() + { + // try setting an invalid file + chmod($this->_invalidPath, 0700); + file_put_contents($this->_invalidFile, ''); + chmod($this->_invalidFile, 0000); + serversalt::setPath($this->_invalidPath); + serversalt::get(); + } + + /** + * @expectedException Exception + * @expectedExceptionCode 13 + */ + public function testFileWrite() + { + // try setting an invalid file + chmod($this->_invalidPath, 0700); + @unlink($this->_invalidFile); + file_put_contents($this->_invalidPath . DIRECTORY_SEPARATOR . '.htaccess', ''); + chmod($this->_invalidPath, 0500); + serversalt::setPath($this->_invalidPath); + serversalt::get(); + } + + /** + * @expectedException Exception + * @expectedExceptionCode 10 + */ + public function testPermissionShenanigans() + { + // try creating an invalid path + chmod($this->_invalidPath, 0000); + serversalt::setPath($this->_invalidPath . DIRECTORY_SEPARATOR . 'baz'); + serversalt::get(); + } +} diff --git a/tst/vizhash16x16.php b/tst/vizhash16x16.php index b33d48d..c810c54 100644 --- a/tst/vizhash16x16.php +++ b/tst/vizhash16x16.php @@ -10,22 +10,20 @@ class vizhash16x16Test extends PHPUnit_Framework_TestCase public function setUp() { /* Setup Routine */ - $this->_path = PATH . 'data' . DIRECTORY_SEPARATOR; - $this->_dataDirCreated = !is_dir($this->_path); - if($this->_dataDirCreated) mkdir($this->_path); - $this->_file = $this->_path . 'vizhash.png'; + $this->_path = PATH . 'data'; + if(!is_dir($this->_path)) mkdir($this->_path); + $this->_file = $this->_path . DIRECTORY_SEPARATOR . 'vizhash.png'; + serversalt::setPath($this->_path); } public function tearDown() { /* Tear Down Routine */ - if($this->_dataDirCreated) { - helper::rmdir($this->_path); - } else { - if(!@unlink($this->_file)) { - throw new Exception('Error deleting file "' . $this->_file . '".'); - } + chmod($this->_path, 0700); + if(!@unlink($this->_file)) { + throw new Exception('Error deleting file "' . $this->_file . '".'); } + helper::rmdir($this->_path); } public function testVizhashGeneratesUniquePngsPerIp() @@ -37,10 +35,5 @@ class vizhash16x16Test extends PHPUnit_Framework_TestCase $this->assertEquals('image/png', $finfo->file($this->_file)); $this->assertNotEquals($pngdata, $vz->generate('2001:1620:2057:dead:beef::cafe:babe')); $this->assertEquals($pngdata, $vz->generate('127.0.0.1')); - - // generating new salt - $salt = serversalt::get(); - require 'mcrypt_mock.php'; - $this->assertNotEquals($salt, serversalt::generate()); } } diff --git a/tst/zerobin/db.php b/tst/zerobin/db.php index 1bb5eb0..94176da 100644 --- a/tst/zerobin/db.php +++ b/tst/zerobin/db.php @@ -1,68 +1,145 @@ - '{"iv":"EN39/wd5Nk8HAiSG2K5AsQ","v":1,"iter":1000,"ks":128,"ts":64,"mode":"ccm","adata":"","cipher":"aes","salt":"QKN1DBXe5PI","ct":"8hA83xDdXjD7K2qfmw5NdA"}', - 'meta' => array( - 'postdate' => 1344803344, - 'expire_date' => 1344803644, - 'opendiscussion' => true, - ), - ); - - private static $commentid = 'c47efb4741195f42'; - - private static $comment = array( - 'data' => '{"iv":"Pd4pOKWkmDTT9uPwVwd5Ag","v":1,"iter":1000,"ks":128,"ts":64,"mode":"ccm","adata":"","cipher":"aes","salt":"ZIUhFTliVz4","ct":"6nOCU3peNDclDDpFtJEBKA"}', - 'meta' => array( - 'nickname' => '{"iv":"76MkAtOGC4oFogX/aSMxRA","v":1,"iter":1000,"ks":128,"ts":64,"mode":"ccm","adata":"","cipher":"aes","salt":"ZIUhFTliVz4","ct":"b6Ae/U1xJdsX/+lATud4sQ"}', - 'vizhash' => 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAABGUlEQVQokWOsl5/94983CNKQMjnxaOePf98MeKwPfNjkLZ3AgARab6b9+PeNEVnDj3/ff/z7ZiHnzsDA8Pv7H2TVPJw8EAYLAwb48OaVgIgYKycLsrYv378wMDB8//qdCVMDRA9EKSsnCwRBxNsepaLboMFlyMDAICAi9uHNK24GITQ/MDAwoNhgIGMLtwGrzegaLjw5jMz9+vUdnN17uwDCQDhJgk0O07yvX9+teDX1x79v6DYIsIjgcgMaYGFgYOBg4kJx2JejkAiBxAw+PzAwMNz4dp6wDXDw4MdNNOl0rWYsNkD89OLXI/xmo9sgzatJjAYmBgYGDiauD3/ePP18nVgb4MF89+M5ZX6js293wUMpnr8KTQMAxsCJnJ30apMAAAAASUVORK5CYII=', - 'postdate' => 1344803528, - ), - ); - - private $_model; - - public function setUp() - { - /* Setup Routine */ - $this->_model = zerobin_db::getInstance( - array( - 'dsn' => 'sqlite::memory:', - 'usr' => null, - 'pwd' => null, - 'opt' => array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION), - ) - ); - } - - public function testDatabaseBasedDataStoreWorks() - { - // storing pastes - $this->assertFalse($this->_model->exists(self::$pasteid), 'paste does not yet exist'); - $this->assertTrue($this->_model->create(self::$pasteid, self::$paste), 'store new paste'); - $this->assertTrue($this->_model->exists(self::$pasteid), 'paste exists after storing it'); - $this->assertFalse($this->_model->create(self::$pasteid, self::$paste), 'unable to store the same paste twice'); - $this->assertEquals(json_decode(json_encode(self::$paste)), $this->_model->read(self::$pasteid)); - - // storing comments - $this->assertFalse($this->_model->existsComment(self::$pasteid, self::$pasteid, self::$commentid), 'comment does not yet exist'); - $this->assertTrue($this->_model->createComment(self::$pasteid, self::$pasteid, self::$commentid, self::$comment) !== false, 'store comment'); - $this->assertTrue($this->_model->existsComment(self::$pasteid, self::$pasteid, self::$commentid), 'comment exists after storing it'); - $comment = json_decode(json_encode(self::$comment)); - $comment->meta->commentid = self::$commentid; - $comment->meta->parentid = self::$pasteid; - $this->assertEquals( - array($comment->meta->postdate => $comment), - $this->_model->readComments(self::$pasteid) - ); - - // deleting pastes - $this->_model->delete(self::$pasteid); - $this->assertFalse($this->_model->exists(self::$pasteid), 'paste successfully deleted'); - $this->assertFalse($this->_model->existsComment(self::$pasteid, self::$pasteid, self::$commentid), 'comment was deleted with paste'); - $this->assertFalse($this->_model->read(self::$pasteid), 'paste can no longer be found'); - } -} + '{"iv":"EN39/wd5Nk8HAiSG2K5AsQ","v":1,"iter":1000,"ks":128,"ts":64,"mode":"ccm","adata":"","cipher":"aes","salt":"QKN1DBXe5PI","ct":"8hA83xDdXjD7K2qfmw5NdA"}', + 'meta' => array( + 'postdate' => 1344803344, + 'expire_date' => 1344803644, + 'opendiscussion' => true, + ), + ); + + private static $commentid = 'c47efb4741195f42'; + + private static $comment = array( + 'data' => '{"iv":"Pd4pOKWkmDTT9uPwVwd5Ag","v":1,"iter":1000,"ks":128,"ts":64,"mode":"ccm","adata":"","cipher":"aes","salt":"ZIUhFTliVz4","ct":"6nOCU3peNDclDDpFtJEBKA"}', + 'meta' => array( + 'nickname' => '{"iv":"76MkAtOGC4oFogX/aSMxRA","v":1,"iter":1000,"ks":128,"ts":64,"mode":"ccm","adata":"","cipher":"aes","salt":"ZIUhFTliVz4","ct":"b6Ae/U1xJdsX/+lATud4sQ"}', + 'vizhash' => 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAABGUlEQVQokWOsl5/94983CNKQMjnxaOePf98MeKwPfNjkLZ3AgARab6b9+PeNEVnDj3/ff/z7ZiHnzsDA8Pv7H2TVPJw8EAYLAwb48OaVgIgYKycLsrYv378wMDB8//qdCVMDRA9EKSsnCwRBxNsepaLboMFlyMDAICAi9uHNK24GITQ/MDAwoNhgIGMLtwGrzegaLjw5jMz9+vUdnN17uwDCQDhJgk0O07yvX9+teDX1x79v6DYIsIjgcgMaYGFgYOBg4kJx2JejkAiBxAw+PzAwMNz4dp6wDXDw4MdNNOl0rWYsNkD89OLXI/xmo9sgzatJjAYmBgYGDiauD3/ePP18nVgb4MF89+M5ZX6js293wUMpnr8KTQMAxsCJnJ30apMAAAAASUVORK5CYII=', + 'postdate' => 1344803528, + ), + ); + + private $_model; + + public function setUp() + { + /* Setup Routine */ + $this->_model = zerobin_db::getInstance( + array( + 'dsn' => 'sqlite::memory:', + 'usr' => null, + 'pwd' => null, + 'opt' => array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION), + ) + ); + } + + public function testDatabaseBasedDataStoreWorks() + { + // storing pastes + $this->assertFalse($this->_model->exists(self::$pasteid), 'paste does not yet exist'); + $this->assertTrue($this->_model->create(self::$pasteid, self::$paste), 'store new paste'); + $this->assertTrue($this->_model->exists(self::$pasteid), 'paste exists after storing it'); + $this->assertFalse($this->_model->create(self::$pasteid, self::$paste), 'unable to store the same paste twice'); + $this->assertEquals(json_decode(json_encode(self::$paste)), $this->_model->read(self::$pasteid)); + + // storing comments + $this->assertFalse($this->_model->existsComment(self::$pasteid, self::$pasteid, self::$commentid), 'comment does not yet exist'); + $this->assertTrue($this->_model->createComment(self::$pasteid, self::$pasteid, self::$commentid, self::$comment) !== false, 'store comment'); + $this->assertTrue($this->_model->existsComment(self::$pasteid, self::$pasteid, self::$commentid), 'comment exists after storing it'); + $comment = json_decode(json_encode(self::$comment)); + $comment->meta->commentid = self::$commentid; + $comment->meta->parentid = self::$pasteid; + $this->assertEquals( + array($comment->meta->postdate => $comment), + $this->_model->readComments(self::$pasteid) + ); + + // deleting pastes + $this->_model->delete(self::$pasteid); + $this->assertFalse($this->_model->exists(self::$pasteid), 'paste successfully deleted'); + $this->assertFalse($this->_model->existsComment(self::$pasteid, self::$pasteid, self::$commentid), 'comment was deleted with paste'); + $this->assertFalse($this->_model->read(self::$pasteid), 'paste can no longer be found'); + } + + /** + * @expectedException PDOException + */ + public function testGetIbmInstance() + { + zerobin_db::getInstance(array( + 'dsn' => 'ibm:', 'usr' => null, 'pwd' => null, + 'opt' => array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION) + )); + } + + /** + * @expectedException PDOException + */ + public function testGetInformixInstance() + { + zerobin_db::getInstance(array( + 'dsn' => 'informix:', 'usr' => null, 'pwd' => null, + 'opt' => array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION) + )); + } + + /** + * @expectedException PDOException + */ + public function testGetMssqlInstance() + { + zerobin_db::getInstance(array( + 'dsn' => 'mssql:', 'usr' => null, 'pwd' => null, + 'opt' => array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION) + )); + } + + /** + * @expectedException PDOException + */ + public function testGetMysqlInstance() + { + zerobin_db::getInstance(array( + 'dsn' => 'mysql:', 'usr' => null, 'pwd' => null, + 'opt' => array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION) + )); + } + + /** + * @expectedException PDOException + */ + public function testGetOciInstance() + { + zerobin_db::getInstance(array( + 'dsn' => 'oci:', 'usr' => null, 'pwd' => null, + 'opt' => array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION) + )); + } + + /** + * @expectedException PDOException + */ + public function testGetPgsqlInstance() + { + zerobin_db::getInstance(array( + 'dsn' => 'pgsql:', 'usr' => null, 'pwd' => null, + 'opt' => array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION) + )); + } + + /** + * @expectedException Exception + * @expectedExceptionCode 5 + */ + public function testGetFooInstance() + { + zerobin_db::getInstance(array( + 'dsn' => 'foo:', 'usr' => null, 'pwd' => null, 'opt' => null + )); + } +}