diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2762,6 +2762,7 @@ '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 @@ '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 --- a/src/applications/files/engine/PhabricatorFileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php @@ -332,6 +332,18 @@ 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 @@ 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 --- a/src/applications/files/engine/PhabricatorTestStorageEngine.php +++ b/src/applications/files/engine/PhabricatorTestStorageEngine.php @@ -47,4 +47,8 @@ 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 --- /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 --- a/src/applications/files/format/PhabricatorFileStorageFormat.php +++ b/src/applications/files/format/PhabricatorFileStorageFormat.php @@ -22,6 +22,10 @@ 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 --- a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php +++ b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php @@ -92,4 +92,37 @@ } $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 --- a/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php +++ b/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php @@ -20,6 +20,13 @@ '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 @@ $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 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -37,6 +37,7 @@ const METADATA_PARTIAL = 'partial'; const METADATA_PROFILE = 'profile'; const METADATA_STORAGE = 'storage'; + const METADATA_INTEGRITY = 'integrity'; protected $name; protected $mimeType; @@ -324,15 +325,18 @@ $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 @@ $file->setStorageEngine($engine_identifier); $file->setStorageHandle($data_handle); + $file->setIntegrityHash($integrity_hash); + $file->readPropertiesFromParameters($params); if (!$file->getMimeType()) { @@ -409,7 +415,7 @@ '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 @@ $this->setStorageEngine($new_identifier); $this->setStorageHandle($new_handle); + $this->setIntegrityHash($integrity_hash); $this->save(); if (!$make_copy) { @@ -494,6 +501,8 @@ $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 @@ $engine_identifier)); } - return array($engine_identifier, $data_handle); + return array($engine_identifier, $data_handle, $integrity_hash); } @@ -1220,6 +1229,15 @@ 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. *