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

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,
         );

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();
   }

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() {

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(