From 440ef5b7a7e48e2aa2370daa09ffd72f356fedcb Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Apr 2017 14:49:22 -0700 Subject: [PATCH] Remove SHA1 file content hashing and make Files work without any hashing Summary: Ref T12464. We currently use SHA1 to detect when two files have the same content so we don't have to store two copies of the data. Now that a SHA1 collision is known, this is theoretically dangerous. T12464 describes the shape of a possible attack. Before replacing this with something more robust, shore things up so things work correctly if we don't hash at all. This mechanism is entirely optional; it only helps us store less data if some files are duplicates. (This mechanism is also less important now than it once was, before we added temporary files.) Test Plan: Uploaded multiple identical files, saw the uploads work and the files store separate copies of the same data. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12464 Differential Revision: https://secure.phabricator.com/D17619 --- .../conduit/FileAllocateConduitAPIMethod.php | 11 ++- .../conduit/FileUploadConduitAPIMethod.php | 26 ++++--- .../files/storage/PhabricatorFile.php | 74 ++++++++++--------- .../__tests__/PhabricatorFileTestCase.php | 5 ++ 4 files changed, 68 insertions(+), 48 deletions(-) diff --git a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php index ddc5fa14fe..c831eb2afe 100644 --- a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php +++ b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php @@ -36,23 +36,26 @@ final class FileAllocateConduitAPIMethod $properties = array( 'name' => $name, 'authorPHID' => $viewer->getPHID(), - 'viewPolicy' => $view_policy, 'isExplicitUpload' => true, ); + if ($view_policy !== null) { + $properties['viewPolicy'] = $view_policy; + } + $ttl = $request->getValue('deleteAfterEpoch'); if ($ttl) { $properties['ttl.absolute'] = $ttl; } $file = null; - if ($hash) { + if ($hash !== null) { $file = PhabricatorFile::newFileFromContentHash( $hash, $properties); } - if ($hash && !$file) { + if ($hash !== null && !$file) { $chunked_hash = PhabricatorChunkedFileStorageEngine::getChunkedHash( $viewer, $hash); @@ -103,7 +106,7 @@ final class FileAllocateConduitAPIMethod if ($chunk_engines) { $chunk_properties = $properties; - if ($hash) { + if ($hash !== null) { $chunk_properties += array( 'chunkedHash' => $chunked_hash, ); diff --git a/src/applications/files/conduit/FileUploadConduitAPIMethod.php b/src/applications/files/conduit/FileUploadConduitAPIMethod.php index ac93b7d39a..8b3b9faaaa 100644 --- a/src/applications/files/conduit/FileUploadConduitAPIMethod.php +++ b/src/applications/files/conduit/FileUploadConduitAPIMethod.php @@ -27,21 +27,27 @@ final class FileUploadConduitAPIMethod extends FileConduitAPIMethod { $viewer = $request->getUser(); $name = $request->getValue('name'); - $can_cdn = $request->getValue('canCDN'); + $can_cdn = (bool)$request->getValue('canCDN'); $view_policy = $request->getValue('viewPolicy'); $data = $request->getValue('data_base64'); $data = $this->decodeBase64($data); - $file = PhabricatorFile::newFromFileData( - $data, - array( - 'name' => $name, - 'authorPHID' => $viewer->getPHID(), - 'viewPolicy' => $view_policy, - 'canCDN' => $can_cdn, - 'isExplicitUpload' => true, - )); + $params = array( + 'authorPHID' => $viewer->getPHID(), + 'canCDN' => $can_cdn, + 'isExplicitUpload' => true, + ); + + if ($name !== null) { + $params['name'] = $name; + } + + if ($view_policy !== null) { + $params['viewPolicy'] = $view_policy; + } + + $file = PhabricatorFile::newFromFileData($data, $params); return $file->getPHID(); } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index a7dd6b2e97..08160f7cbc 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -198,39 +198,42 @@ final class PhabricatorFile extends PhabricatorFileDAO public static function newFileFromContentHash($hash, array $params) { - // Check to see if a file with same contentHash exist + if ($hash === null) { + return null; + } + + // Check to see if a file with same hash already exists. $file = id(new PhabricatorFile())->loadOneWhere( 'contentHash = %s LIMIT 1', $hash); - - if ($file) { - $copy_of_storage_engine = $file->getStorageEngine(); - $copy_of_storage_handle = $file->getStorageHandle(); - $copy_of_storage_format = $file->getStorageFormat(); - $copy_of_storage_properties = $file->getStorageProperties(); - $copy_of_byte_size = $file->getByteSize(); - $copy_of_mime_type = $file->getMimeType(); - - $new_file = self::initializeNewFile(); - - $new_file->setByteSize($copy_of_byte_size); - - $new_file->setContentHash($hash); - $new_file->setStorageEngine($copy_of_storage_engine); - $new_file->setStorageHandle($copy_of_storage_handle); - $new_file->setStorageFormat($copy_of_storage_format); - $new_file->setStorageProperties($copy_of_storage_properties); - $new_file->setMimeType($copy_of_mime_type); - $new_file->copyDimensions($file); - - $new_file->readPropertiesFromParameters($params); - - $new_file->save(); - - return $new_file; + if (!$file) { + return null; } - return $file; + $copy_of_storage_engine = $file->getStorageEngine(); + $copy_of_storage_handle = $file->getStorageHandle(); + $copy_of_storage_format = $file->getStorageFormat(); + $copy_of_storage_properties = $file->getStorageProperties(); + $copy_of_byte_size = $file->getByteSize(); + $copy_of_mime_type = $file->getMimeType(); + + $new_file = self::initializeNewFile(); + + $new_file->setByteSize($copy_of_byte_size); + + $new_file->setContentHash($hash); + $new_file->setStorageEngine($copy_of_storage_engine); + $new_file->setStorageHandle($copy_of_storage_handle); + $new_file->setStorageFormat($copy_of_storage_format); + $new_file->setStorageProperties($copy_of_storage_properties); + $new_file->setMimeType($copy_of_mime_type); + $new_file->copyDimensions($file); + + $new_file->readPropertiesFromParameters($params); + + $new_file->save(); + + return $new_file; } public static function newChunkedFile( @@ -353,7 +356,9 @@ final class PhabricatorFile extends PhabricatorFileDAO } $file->setByteSize(strlen($data)); - $file->setContentHash(self::hashFileContent($data)); + + $hash = self::hashFileContent($data); + $file->setContentHash($hash); $file->setStorageEngine($engine_identifier); $file->setStorageHandle($data_handle); @@ -379,10 +384,12 @@ final class PhabricatorFile extends PhabricatorFileDAO public static function newFromFileData($data, array $params = array()) { $hash = self::hashFileContent($data); - $file = self::newFileFromContentHash($hash, $params); - if ($file) { - return $file; + if ($hash !== null) { + $file = self::newFileFromContentHash($hash, $params); + if ($file) { + return $file; + } } return self::buildFromFileData($data, $params); @@ -710,9 +717,8 @@ final class PhabricatorFile extends PhabricatorFileDAO } } - public static function hashFileContent($data) { - return sha1($data); + return null; } public function loadFileData() { diff --git a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php index 5b64c9b1ac..936676b902 100644 --- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php +++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php @@ -301,6 +301,11 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase { $data = Filesystem::readRandomCharacters(64); + $hash = PhabricatorFile::hashFileContent($data); + if ($hash === null) { + $this->assertSkipped(pht('File content hashing is not available.')); + } + $params = array( 'name' => 'test.dat', 'storageEngines' => array(