From 63828f580689b7799d8d38ed4604e91385f33505 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Apr 2017 09:04:44 -0700 Subject: [PATCH] Store and verify content integrity checksums for files Summary: Ref T12470. This helps defuse attacks where an adversary can directly take control of whatever storage engine files are being stored in and change data there. These attacks would require a significant level of access. Such attackers could potentially attack ranges of AES-256-CBC encrypted files by using Phabricator as a decryption oracle if they were also able to compromise a Phabricator account with read access to the files. By storing a hash of the data (and, in the case of AES-256-CBC files, the IV) when we write files, and verifying it before we decrypt or read them, we can detect and prevent this kind of tampering. This also helps detect mundane corruption and integrity issues. Test Plan: - Added unit tests. - Uploaded new files, saw them get integrity hashes. - Manually corrupted file data, saw it fail. Used `bin/files cat --salvage` to read it anyway. - Tampered with IVs, saw integrity failures. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12470 Differential Revision: https://secure.phabricator.com/D17625 --- src/__phutil_library_map__.php | 2 ++ .../engine/PhabricatorFileStorageEngine.php | 24 ++++++++++++++ .../engine/PhabricatorTestStorageEngine.php | 4 +++ .../PhabricatorFileIntegrityException.php | 4 +++ .../PhabricatorFileAES256StorageFormat.php | 14 ++++++++ .../format/PhabricatorFileStorageFormat.php | 4 +++ .../PhabricatorFileStorageFormatTestCase.php | 33 +++++++++++++++++++ .../PhabricatorFilesManagementCatWorkflow.php | 32 ++++++++++++++++-- .../files/storage/PhabricatorFile.php | 24 ++++++++++++-- 9 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 src/applications/files/exception/PhabricatorFileIntegrityException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f082c2fe22..3fa9b2e25e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2762,6 +2762,7 @@ phutil_register_library_map(array( 'PhabricatorFileImageProxyController' => 'applications/files/controller/PhabricatorFileImageProxyController.php', 'PhabricatorFileImageTransform' => 'applications/files/transform/PhabricatorFileImageTransform.php', 'PhabricatorFileInfoController' => 'applications/files/controller/PhabricatorFileInfoController.php', + 'PhabricatorFileIntegrityException' => 'applications/files/exception/PhabricatorFileIntegrityException.php', 'PhabricatorFileLightboxController' => 'applications/files/controller/PhabricatorFileLightboxController.php', 'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php', 'PhabricatorFileListController' => 'applications/files/controller/PhabricatorFileListController.php', @@ -7891,6 +7892,7 @@ phutil_register_library_map(array( 'PhabricatorFileImageProxyController' => 'PhabricatorFileController', 'PhabricatorFileImageTransform' => 'PhabricatorFileTransform', 'PhabricatorFileInfoController' => 'PhabricatorFileController', + 'PhabricatorFileIntegrityException' => 'Exception', 'PhabricatorFileLightboxController' => 'PhabricatorFileController', 'PhabricatorFileLinkView' => 'AphrontTagView', 'PhabricatorFileListController' => 'PhabricatorFileController', diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php index 39660f847d..9a15460ec8 100644 --- a/src/applications/files/engine/PhabricatorFileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php @@ -332,6 +332,18 @@ abstract class PhabricatorFileStorageEngine extends Phobject { PhabricatorFileStorageFormat $format) { $formatted_data = $this->readFile($file->getStorageHandle()); + + $known_integrity = $file->getIntegrityHash(); + if ($known_integrity !== null) { + $new_integrity = $this->newIntegrityHash($formatted_data, $format); + if ($known_integrity !== $new_integrity) { + throw new PhabricatorFileIntegrityException( + pht( + 'File data integrity check failed. Dark forces have corrupted '. + 'or tampered with this file. The file data can not be read.')); + } + } + $formatted_data = array($formatted_data); $data = ''; @@ -351,4 +363,16 @@ abstract class PhabricatorFileStorageEngine extends Phobject { return array($data); } + public function newIntegrityHash( + $data, + PhabricatorFileStorageFormat $format) { + + $data_hash = PhabricatorHash::digest($data); + $format_hash = $format->newFormatIntegrityHash(); + + $full_hash = "{$data_hash}/{$format_hash}"; + + return PhabricatorHash::digest($full_hash); + } + } diff --git a/src/applications/files/engine/PhabricatorTestStorageEngine.php b/src/applications/files/engine/PhabricatorTestStorageEngine.php index 72e68dc7c7..952fba6075 100644 --- a/src/applications/files/engine/PhabricatorTestStorageEngine.php +++ b/src/applications/files/engine/PhabricatorTestStorageEngine.php @@ -47,4 +47,8 @@ final class PhabricatorTestStorageEngine unset(self::$storage[$handle]); } + public function tamperWithFile($handle, $data) { + self::$storage[$handle] = $data; + } + } diff --git a/src/applications/files/exception/PhabricatorFileIntegrityException.php b/src/applications/files/exception/PhabricatorFileIntegrityException.php new file mode 100644 index 0000000000..f1d1989da8 --- /dev/null +++ b/src/applications/files/exception/PhabricatorFileIntegrityException.php @@ -0,0 +1,4 @@ +getFile(); + list($key_envelope, $iv_envelope) = $this->extractKeyAndIV($file); + + // NOTE: We include the IV in the format integrity hash. If we do not, + // attackers can potentially forge the first block of decrypted data + // in CBC mode if they are able to substitute a chosen IV and predict + // the plaintext. (Normally, they can not tamper with the IV.) + + $input = self::FORMATKEY.'/iv:'.$iv_envelope->openEnvelope(); + + return PhabricatorHash::digest($input); + } + public function newStorageProperties() { // Generate a unique key and IV for this block of data. $key_envelope = self::newAES256Key(); diff --git a/src/applications/files/format/PhabricatorFileStorageFormat.php b/src/applications/files/format/PhabricatorFileStorageFormat.php index 0d286ecd64..2d24aa2207 100644 --- a/src/applications/files/format/PhabricatorFileStorageFormat.php +++ b/src/applications/files/format/PhabricatorFileStorageFormat.php @@ -22,6 +22,10 @@ abstract class PhabricatorFileStorageFormat abstract public function newReadIterator($raw_iterator); abstract public function newWriteIterator($raw_iterator); + public function newFormatIntegrityHash() { + return null; + } + public function newStorageProperties() { return array(); } diff --git a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php index 77d161045f..db10b87964 100644 --- a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php +++ b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php @@ -92,4 +92,37 @@ final class PhabricatorFileStorageFormatTestCase extends PhabricatorTestCase { } $this->assertEqual('cow jumped', $raw_data); } + + public function testStorageTampering() { + $engine = new PhabricatorTestStorageEngine(); + + $good = 'The cow jumped over the full moon.'; + $evil = 'The cow slept quietly, honoring the glorious dictator.'; + + $params = array( + 'name' => 'message.txt', + 'storageEngines' => array( + $engine, + ), + ); + + // First, write the file normally. + $file = PhabricatorFile::newFromFileData($good, $params); + $this->assertEqual($good, $file->loadFileData()); + + // As an adversary, tamper with the file. + $engine->tamperWithFile($file->getStorageHandle(), $evil); + + // Attempts to read the file data should now fail the integrity check. + $caught = null; + try { + $file->loadFileData(); + } catch (PhabricatorFileIntegrityException $ex) { + $caught = $ex; + } + + $this->assertTrue($caught instanceof PhabricatorFileIntegrityException); + } + + } diff --git a/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php index 98efd2dee9..70d5eca5a2 100644 --- a/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php +++ b/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php @@ -19,6 +19,13 @@ final class PhabricatorFilesManagementCatWorkflow 'param' => 'bytes', 'help' => pht('End printing at a specific offset.'), ), + array( + 'name' => 'salvage', + 'help' => pht( + 'DANGEROUS. Attempt to salvage file content even if the '. + 'integrity check fails. If an adversary has tampered with '. + 'the file, the conent may be unsafe.'), + ), array( 'name' => 'names', 'wildcard' => true, @@ -43,9 +50,28 @@ final class PhabricatorFilesManagementCatWorkflow $begin = $args->getArg('begin'); $end = $args->getArg('end'); - $iterator = $file->getFileDataIterator($begin, $end); - foreach ($iterator as $data) { - echo $data; + $file->makeEphemeral(); + + // If we're running in "salvage" mode, wipe out any integrity hash which + // may be present. This makes us read file data without performing an + // integrity check. + $salvage = $args->getArg('salvage'); + if ($salvage) { + $file->setIntegrityHash(null); + } + + try { + $iterator = $file->getFileDataIterator($begin, $end); + foreach ($iterator as $data) { + echo $data; + } + } catch (PhabricatorFileIntegrityException $ex) { + throw new PhutilArgumentUsageException( + pht( + 'File data integrity check failed. Use "--salvage" to bypass '. + 'integrity checks. This flag is dangerous, use it at your own '. + 'risk. Underlying error: %s', + $ex->getMessage())); } return 0; diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index da65477202..dd474d9aa6 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -37,6 +37,7 @@ final class PhabricatorFile extends PhabricatorFileDAO const METADATA_PARTIAL = 'partial'; const METADATA_PROFILE = 'profile'; const METADATA_STORAGE = 'storage'; + const METADATA_INTEGRITY = 'integrity'; protected $name; protected $mimeType; @@ -324,15 +325,18 @@ final class PhabricatorFile extends PhabricatorFileDAO $data_handle = null; $engine_identifier = null; + $integrity_hash = null; $exceptions = array(); foreach ($engines as $engine) { $engine_class = get_class($engine); try { - list($engine_identifier, $data_handle) = $file->writeToEngine( + $result = $file->writeToEngine( $engine, $data, $params); + list($engine_identifier, $data_handle, $integrity_hash) = $result; + // We stored the file somewhere so stop trying to write it to other // places. break; @@ -363,6 +367,8 @@ final class PhabricatorFile extends PhabricatorFileDAO $file->setStorageEngine($engine_identifier); $file->setStorageHandle($data_handle); + $file->setIntegrityHash($integrity_hash); + $file->readPropertiesFromParameters($params); if (!$file->getMimeType()) { @@ -409,7 +415,7 @@ final class PhabricatorFile extends PhabricatorFileDAO 'name' => $this->getName(), ); - list($new_identifier, $new_handle) = $this->writeToEngine( + list($new_identifier, $new_handle, $integrity_hash) = $this->writeToEngine( $engine, $data, $params); @@ -420,6 +426,7 @@ final class PhabricatorFile extends PhabricatorFileDAO $this->setStorageEngine($new_identifier); $this->setStorageHandle($new_handle); + $this->setIntegrityHash($integrity_hash); $this->save(); if (!$make_copy) { @@ -494,6 +501,8 @@ final class PhabricatorFile extends PhabricatorFileDAO $formatted_iterator = $format->newWriteIterator($data_iterator); $formatted_data = $this->loadDataFromIterator($formatted_iterator); + $integrity_hash = $engine->newIntegrityHash($formatted_data, $format); + $data_handle = $engine->writeFile($formatted_data, $params); if (!$data_handle || strlen($data_handle) > 255) { @@ -518,7 +527,7 @@ final class PhabricatorFile extends PhabricatorFileDAO $engine_identifier)); } - return array($engine_identifier, $data_handle); + return array($engine_identifier, $data_handle, $integrity_hash); } @@ -1220,6 +1229,15 @@ final class PhabricatorFile extends PhabricatorFileDAO return $this; } + public function setIntegrityHash($integrity_hash) { + $this->metadata[self::METADATA_INTEGRITY] = $integrity_hash; + return $this; + } + + public function getIntegrityHash() { + return idx($this->metadata, self::METADATA_INTEGRITY); + } + /** * Write the policy edge between this file and some object. *