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

@@ -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',

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

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;
+  }
+
 }

src/applications/files/exception/PhabricatorFileIntegrityException.php

@@ -0,0 +1,4 @@
+<?php
+
+final class PhabricatorFileIntegrityException
+  extends Exception {}

src/applications/files/format/PhabricatorFileAES256StorageFormat.php

@@ -56,6 +56,20 @@ final class PhabricatorFileAES256StorageFormat
     return array($data);
   }
 
+  public function newFormatIntegrityHash() {
+    $file = $this->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();

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

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

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,10 +50,29 @@ final class PhabricatorFilesManagementCatWorkflow
     $begin = $args->getArg('begin');
     $end = $args->getArg('end');
 
+    $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;
   }

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.
    *