Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F13196724
D17625.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
10 KB
Referenced Files
None
Subscribers
None
D17625.diff
View Options
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 @@
+<?php
+
+final class PhabricatorFileIntegrityException
+ extends Exception {}
diff --git a/src/applications/files/format/PhabricatorFileAES256StorageFormat.php b/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
--- a/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
+++ b/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
@@ -56,6 +56,20 @@
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();
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.
*
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Mon, May 13, 11:30 PM (2 w, 5 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6285779
Default Alt Text
D17625.diff (10 KB)
Attached To
Mode
D17625: Store and verify content integrity checksums for files
Attached
Detach File
Event Timeline
Log In to Comment