From 79db7ddafcb4efc13bbfa4bf2e8b1b90a758aa88 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Fri, 28 Jun 2019 21:33:52 +0200 Subject: [PATCH] sending challenge on paste creation, adding logic to store and check it on view requests --- js/pastemeta.jsonld | 3 ++ js/privatebin.js | 9 +++-- js/types.jsonld | 3 ++ lib/Controller.php | 19 ++++++++--- lib/FormatV2.php | 7 ++++ lib/Model/Paste.php | 66 +++++++++++++++++++++++++++++++++++-- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- tst/Bootstrap.php | 2 +- tst/ControllerTest.php | 74 ++++++++++++++++++++++++++++++++++++++++++ tst/FormatV2Test.php | 4 +++ tst/ModelTest.php | 54 +++++++++++++++++++++++++++++- tst/RequestTest.php | 16 +++++++++ 13 files changed, 247 insertions(+), 14 deletions(-) diff --git a/js/pastemeta.jsonld b/js/pastemeta.jsonld index 2ebeca07..3294998a 100644 --- a/js/pastemeta.jsonld +++ b/js/pastemeta.jsonld @@ -26,6 +26,9 @@ }, "time_to_live": { "@type": "pb:RemainingSeconds" + }, + "challenge": { + "@type": "pb:Challenge" } } } \ No newline at end of file diff --git a/js/privatebin.js b/js/privatebin.js index 57fbeda8..672d9322 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -4229,7 +4229,9 @@ jQuery.PrivateBin = (function($, RawDeflate) { // prepare server interaction ServerInteraction.prepare(); - ServerInteraction.setCryptParameters(TopNav.getPassword()); + const key = CryptTool.getSymmetricKey(), + password = TopNav.getPassword(); + ServerInteraction.setCryptParameters(password, key); // set success/fail functions ServerInteraction.setSuccess(showCreatedPaste); @@ -4250,7 +4252,10 @@ jQuery.PrivateBin = (function($, RawDeflate) { TopNav.getOpenDiscussion() ? 1 : 0, TopNav.getBurnAfterReading() ? 1 : 0 ]); - ServerInteraction.setUnencryptedData('meta', {'expire': TopNav.getExpiration()}); + ServerInteraction.setUnencryptedData('meta', { + 'expire': TopNav.getExpiration(), + 'challenge': CryptTool.getCredentials(key, password) + }); // prepare PasteViewer for later preview PasteViewer.setText(plainText); diff --git a/js/types.jsonld b/js/types.jsonld index 005b68b1..d0b1ed0b 100644 --- a/js/types.jsonld +++ b/js/types.jsonld @@ -92,6 +92,9 @@ "@type": "dp:Second", "@minimum": 1 }, + "Challenge": { + "@type": "pb:Base64" + }, "CipherParameters": { "@container": "@list", "@value": [ diff --git a/lib/Controller.php b/lib/Controller.php index 2e0588b7..07b96de3 100644 --- a/lib/Controller.php +++ b/lib/Controller.php @@ -276,9 +276,7 @@ class Controller // accessing this method ensures that the paste would be // deleted if it has already expired $paste->get(); - if ( - Filter::slowEquals($deletetoken, $paste->getDeleteToken()) - ) { + if ($paste->isDeleteTokenCorrect($deletetoken)) { // Paste exists and deletion token is valid: Delete the paste. $paste->delete(); $this->_status = 'Paste was properly deleted.'; @@ -315,9 +313,20 @@ class Controller try { $paste = $this->_model->getPaste($dataid); if ($paste->exists()) { + // handle challenge response + if (!$paste->isTokenCorrect($this->_request->getParam('token'))) { + // we send a generic error to avoid leaking information + // about the existance of a burn after reading pastes + // this avoids an attacker being able to poll, if it has + // been read by the intended recipient or not + $this->_return_message(1, self::GENERIC_ERROR); + return; + } $data = $paste->get(); - if (array_key_exists('salt', $data['meta'])) { - unset($data['meta']['salt']); + foreach (array('salt', 'challenge') as $key) { + if (array_key_exists($key, $data['meta'])) { + unset($data['meta'][$key]); + } } $this->_return_message(0, $dataid, (array) $data); } else { diff --git a/lib/FormatV2.php b/lib/FormatV2.php index 358d834e..ab5ff2a7 100644 --- a/lib/FormatV2.php +++ b/lib/FormatV2.php @@ -67,6 +67,13 @@ class FormatV2 if (!($ct = base64_decode($message['ct'], true))) { return false; } + // - (optional) challenge + if ( + !$isComment && array_key_exists('challenge', $message['meta']) && + !base64_decode($message['meta']['challenge'], true) + ) { + return false; + } // Make sure some fields have a reasonable size: // - initialization vector diff --git a/lib/Model/Paste.php b/lib/Model/Paste.php index 27890840..e8504f2d 100644 --- a/lib/Model/Paste.php +++ b/lib/Model/Paste.php @@ -14,6 +14,7 @@ namespace PrivateBin\Model; use Exception; use PrivateBin\Controller; +use PrivateBin\Filter; use PrivateBin\Persistence\ServerSalt; /** @@ -23,6 +24,14 @@ use PrivateBin\Persistence\ServerSalt; */ class Paste extends AbstractModel { + /** + * Token for challenge/response. + * + * @access protected + * @var string + */ + protected $_token = ''; + /** * Get paste data. * @@ -32,6 +41,11 @@ class Paste extends AbstractModel */ public function get() { + // return cached result if one is found + if (array_key_exists('adata', $this->_data) || array_key_exists('data', $this->_data)) { + return $this->_data; + } + $data = $this->_store->read($this->getId()); if ($data === false) { throw new Exception(Controller::GENERIC_ERROR, 64); @@ -48,10 +62,16 @@ class Paste extends AbstractModel unset($data['meta']['expire_date']); } - // check if non-expired burn after reading paste needs to be deleted + // check if non-expired burn after reading paste needs to be deleted, + // but don't delete it if an incorrect token was sent if ( - (array_key_exists('adata', $data) && $data['adata'][3] === 1) || - (array_key_exists('burnafterreading', $data['meta']) && $data['meta']['burnafterreading']) + ( + (array_key_exists('adata', $data) && $data['adata'][3] === 1) || + (array_key_exists('burnafterreading', $data['meta']) && $data['meta']['burnafterreading']) + ) && ( + !array_key_exists('challenge', $data['meta']) || + $this->_token === $data['meta']['challenge'] + ) ) { $this->delete(); } @@ -94,6 +114,12 @@ class Paste extends AbstractModel $this->_data['meta']['created'] = time(); $this->_data['meta']['salt'] = serversalt::generate(); + // if a challenge was sent, we store the HMAC of paste ID & challenge + if (array_key_exists('challenge', $this->_data['meta'])) { + $this->_data['meta']['challenge'] = hash_hmac( + 'sha256', $this->getId(), base64_decode($this->_data['meta']['challenge']) + ); + } // store paste if ( @@ -201,6 +227,40 @@ class Paste extends AbstractModel (array_key_exists('opendiscussion', $this->_data['meta']) && $this->_data['meta']['opendiscussion']); } + /** + * Check if paste challenge matches provided token. + * + * @access public + * @param string $token + * @throws Exception + * @return bool + */ + public function isTokenCorrect($token) + { + $this->_token = $token; + if (!array_key_exists('challenge', $this->_data['meta'])) { + $this->get(); + } + if (array_key_exists('challenge', $this->_data['meta'])) { + return Filter::slowEquals($token, $this->_data['meta']['challenge']); + } + // paste created without challenge, accept every token sent + return true; + } + + /** + * Check if paste salt based HMAC matches provided delete token. + * + * @access public + * @param string $deletetoken + * @throws Exception + * @return bool + */ + public function isDeleteTokenCorrect($deletetoken) + { + return Filter::slowEquals($deletetoken, $this->getDeleteToken()); + } + /** * Sanitizes data to conform with current configuration. * diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index c73b6738..d0ef8126 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -71,7 +71,7 @@ if ($MARKDOWN): endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index bbf55845..f4e1fde5 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -49,7 +49,7 @@ if ($MARKDOWN): endif; ?> - + diff --git a/tst/Bootstrap.php b/tst/Bootstrap.php index b5393510..540dc231 100644 --- a/tst/Bootstrap.php +++ b/tst/Bootstrap.php @@ -155,7 +155,7 @@ class Helper public static function getPastePost($version = 2, array $meta = array()) { $example = self::getPaste($version, $meta); - $example['meta'] = array('expire' => $example['meta']['expire']); + $example['meta'] = array_merge(array('expire' => $example['meta']['expire']), $meta); return $example; } diff --git a/tst/ControllerTest.php b/tst/ControllerTest.php index b00f2ce6..4c9a19dc 100644 --- a/tst/ControllerTest.php +++ b/tst/ControllerTest.php @@ -366,6 +366,30 @@ class ControllerTest extends PHPUnit_Framework_TestCase $this->assertEquals(1, $paste['adata'][2], 'discussion is enabled'); } + /** + * @runInSeparateProcess + */ + public function testCreateInvalidFormat() + { + $options = parse_ini_file(CONF, true); + $options['traffic']['limit'] = 0; + Helper::createIniFile(CONF, $options); + $paste = Helper::getPasteJson(2, array('challenge' => '$')); + $file = tempnam(sys_get_temp_dir(), 'FOO'); + file_put_contents($file, $paste); + Request::setInputStream($file); + $_SERVER['HTTP_X_REQUESTED_WITH'] = 'JSONHttpRequest'; + $_SERVER['REQUEST_METHOD'] = 'POST'; + $_SERVER['REMOTE_ADDR'] = '::1'; + ob_start(); + new Controller; + $content = ob_get_contents(); + ob_end_clean(); + $response = json_decode($content, true); + $this->assertEquals(1, $response['status'], 'outputs error status'); + $this->assertFalse($this->_data->exists(Helper::getPasteId()), 'paste exists after posting data'); + } + /** * @runInSeparateProcess */ @@ -784,6 +808,56 @@ class ControllerTest extends PHPUnit_Framework_TestCase $this->assertFalse($this->_data->exists(Helper::getPasteId()), 'paste successfully deleted'); } + /** + * @runInSeparateProcess + */ + public function testReadBurnAfterReadingWithToken() + { + $token = base64_encode(hash_hmac( + 'sha256', Helper::getPasteId(), random_bytes(32) + )); + $burnPaste = Helper::getPaste(2, array('challenge' => $token)); + $burnPaste['adata'][3] = 1; + $this->_data->create(Helper::getPasteId(), $burnPaste); + $this->assertTrue($this->_data->exists(Helper::getPasteId()), 'paste exists before deleting data'); + $_SERVER['QUERY_STRING'] = Helper::getPasteId() . '&token=' . $token; + $_GET[Helper::getPasteId()] = ''; + $_GET['token'] = $token; + $_SERVER['HTTP_X_REQUESTED_WITH'] = 'JSONHttpRequest'; + ob_start(); + new Controller; + $content = ob_get_contents(); + ob_end_clean(); + $response = json_decode($content, true); + $this->assertEquals(0, $response['status'], 'outputs status'); + $this->assertFalse($this->_data->exists(Helper::getPasteId()), 'paste successfully deleted'); + } + + /** + * @runInSeparateProcess + */ + public function testReadBurnAfterReadingWithIncorrectToken() + { + $token = base64_encode(hash_hmac( + 'sha256', Helper::getPasteId(), random_bytes(32) + )); + $burnPaste = Helper::getPaste(2, array('challenge' => base64_encode(random_bytes(32)))); + $burnPaste['adata'][3] = 1; + $this->_data->create(Helper::getPasteId(), $burnPaste); + $this->assertTrue($this->_data->exists(Helper::getPasteId()), 'paste exists before deleting data'); + $_SERVER['QUERY_STRING'] = Helper::getPasteId() . '&token=' . $token; + $_GET[Helper::getPasteId()] = ''; + $_GET['token'] = $token; + $_SERVER['HTTP_X_REQUESTED_WITH'] = 'JSONHttpRequest'; + ob_start(); + new Controller; + $content = ob_get_contents(); + ob_end_clean(); + $response = json_decode($content, true); + $this->assertEquals(1, $response['status'], 'outputs status'); + $this->assertTrue($this->_data->exists(Helper::getPasteId()), 'paste not deleted'); + } + /** * @runInSeparateProcess */ diff --git a/tst/FormatV2Test.php b/tst/FormatV2Test.php index 8aa4d3ca..53473457 100644 --- a/tst/FormatV2Test.php +++ b/tst/FormatV2Test.php @@ -21,6 +21,10 @@ class FormatV2Test extends PHPUnit_Framework_TestCase $paste['ct'] = '$'; $this->assertFalse(FormatV2::isValid($paste), 'invalid base64 encoding of ct'); + $paste = Helper::getPastePost(); + $paste['meta']['challenge'] = '$'; + $this->assertFalse(FormatV2::isValid($paste), 'invalid base64 encoding of ct'); + $paste = Helper::getPastePost(); $paste['ct'] = 'bm9kYXRhbm9kYXRhbm9kYXRhbm9kYXRhbm9kYXRhCg=='; $this->assertFalse(FormatV2::isValid($paste), 'low ct entropy'); diff --git a/tst/ModelTest.php b/tst/ModelTest.php index d5c4074f..c7d3fa9d 100644 --- a/tst/ModelTest.php +++ b/tst/ModelTest.php @@ -268,10 +268,48 @@ class ModelTest extends PHPUnit_Framework_TestCase $paste->setData($pasteData); $paste->store(); - $paste = $paste->get(); + $paste = $this->_model->getPaste(Helper::getPasteId())->get(); $this->assertEquals((float) 300, (float) $paste['meta']['time_to_live'], 'remaining time is set correctly', 1.0); } + public function testToken() + { + $pasteData = Helper::getPastePost(); + $pasteData['meta']['challenge'] = base64_encode(random_bytes(32)); + $token = hash_hmac( + 'sha256', Helper::getPasteId(), base64_decode($pasteData['meta']['challenge']) + ); + $this->_model->getPaste(Helper::getPasteId())->delete(); + $paste = $this->_model->getPaste(Helper::getPasteId()); + $this->assertFalse($paste->exists(), 'paste does not yet exist'); + + $paste = $this->_model->getPaste(); + $paste->setData($pasteData); + $paste->store(); + + $paste = $this->_model->getPaste(Helper::getPasteId()); + $this->assertTrue( + $paste->isTokenCorrect($token), + 'token is accepted after store and retrieval' + ); + } + + public function testDeleteToken() + { + $pasteData = Helper::getPastePost(); + $this->_model->getPaste(Helper::getPasteId())->delete(); + $paste = $this->_model->getPaste(Helper::getPasteId()); + $this->assertFalse($paste->exists(), 'paste does not yet exist'); + + $paste = $this->_model->getPaste(); + $paste->setData($pasteData); + $paste->store(); + $deletetoken = $paste->getDeleteToken(); + + $paste = $this->_model->getPaste(Helper::getPasteId()); + $this->assertTrue($paste->isDeleteTokenCorrect($deletetoken), 'delete token is accepted after store and retrieval'); + } + /** * @expectedException Exception * @expectedExceptionCode 64 @@ -287,6 +325,20 @@ class ModelTest extends PHPUnit_Framework_TestCase $paste->getComment(Helper::getPasteId())->delete(); } + /** + * @expectedException Exception + * @expectedExceptionCode 75 + */ + public function testInvalidFormat() + { + $pasteData = Helper::getPastePost(); + $pasteData['adata'][1] = 'foo'; + $this->_model->getPaste(Helper::getPasteId())->delete(); + + $paste = $this->_model->getPaste(); + $paste->setData($pasteData); + } + public function testPurge() { $conf = new Configuration; diff --git a/tst/RequestTest.php b/tst/RequestTest.php index 9b440be0..b92d903d 100644 --- a/tst/RequestTest.php +++ b/tst/RequestTest.php @@ -130,6 +130,22 @@ class RequestTest extends PHPUnit_Framework_TestCase $this->assertEquals('read', $request->getOperation()); } + public function testApiReadWithToken() + { + $this->reset(); + $id = $this->getRandomId(); + $_SERVER['REQUEST_METHOD'] = 'GET'; + $_SERVER['HTTP_ACCEPT'] = 'application/json, text/javascript, */*; q=0.01'; + $_SERVER['QUERY_STRING'] = $id . '&token=foo'; + $_GET[$id] = ''; + $_GET['token'] = 'foo'; + $request = new Request; + $this->assertTrue($request->isJsonApiCall(), 'is JSON Api call'); + $this->assertEquals($id, $request->getParam('pasteid')); + $this->assertEquals('foo', $request->getParam('token')); + $this->assertEquals('read', $request->getOperation()); + } + public function testApiDelete() { $this->reset();