From 0669abc5f0e11da16a40d9bbb48869aff91e22e5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Oct 2011 13:50:10 -0700 Subject: [PATCH] Use a proper entropy source to generate file keys Summary: See T549. Under configurations where files are served from an alternate domain which does not have cookie credentials, we use random keys to prevent browsing, similar to how Facebook relies on pseudorandom information in image URIs (we could some day go farther than this and generate file sessions on the alternate domain or something, I guess). Currently, we generate these random keys in a roundabout manner. Instead, use a real entropy source and store the key on the object. This reduces the number of sha1() calls in the codebase as per T547. Test Plan: Ran upgrade scripts, verified database was populated correctly. Configured alternate file domain, uploaded file, verified secret generated and worked properly. Changed secret, was given 404. Reviewers: jungejason, benmathews, nh, tuomaspelkonen, aran Reviewed By: aran CC: aran, epriestley Differential Revision: 1036 --- conf/default.conf.php | 6 ---- resources/sql/patches/080.filekeys.sql | 2 ++ resources/sql/patches/081.filekeys.php | 31 +++++++++++++++++++ .../PhabricatorFileAltViewController.php | 1 + .../files/storage/file/PhabricatorFile.php | 22 +++++++++---- 5 files changed, 50 insertions(+), 12 deletions(-) create mode 100644 resources/sql/patches/080.filekeys.sql create mode 100644 resources/sql/patches/081.filekeys.php diff --git a/conf/default.conf.php b/conf/default.conf.php index c287485432..e719e03ab1 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -406,12 +406,6 @@ return array( // addresses. 'phabricator.mail-key' => '5ce3e7e8787f6e40dfae861da315a5cdf1018f12', - - // This is hashed with other inputs to generate file secret keys. Changing - // it will invalidate all file URIs if you have an alternate file domain - // configured (see 'security.alternate-file-domain'). - 'phabricator.file-key' => 'ade8dadc8b4382067069a4d4798112191af8a190', - // Version string displayed in the footer. You probably should leave this // alone. 'phabricator.version' => 'UNSTABLE', diff --git a/resources/sql/patches/080.filekeys.sql b/resources/sql/patches/080.filekeys.sql new file mode 100644 index 0000000000..1100f5adc1 --- /dev/null +++ b/resources/sql/patches/080.filekeys.sql @@ -0,0 +1,2 @@ +ALTER TABLE phabricator_file.file + ADD secretKey VARCHAR(20) BINARY; \ No newline at end of file diff --git a/resources/sql/patches/081.filekeys.php b/resources/sql/patches/081.filekeys.php new file mode 100644 index 0000000000..5feb300d3c --- /dev/null +++ b/resources/sql/patches/081.filekeys.php @@ -0,0 +1,31 @@ +loadAllWhere('secretKey IS NULL'); +echo count($files).' files to generate keys for'; +foreach ($files as $file) { + queryfx( + $file->establishConnection('r'), + 'UPDATE %T SET secretKey = %s WHERE id = %d', + $file->getTableName(), + $file->generateSecretKey(), + $file->getID()); + echo '.'; +} +echo "\nDone.\n"; diff --git a/src/applications/files/controller/altview/PhabricatorFileAltViewController.php b/src/applications/files/controller/altview/PhabricatorFileAltViewController.php index aaa55a6bae..01b042cdb2 100644 --- a/src/applications/files/controller/altview/PhabricatorFileAltViewController.php +++ b/src/applications/files/controller/altview/PhabricatorFileAltViewController.php @@ -61,6 +61,7 @@ class PhabricatorFileAltViewController extends PhabricatorFileController { $data = $file->loadFileData(); $response = new AphrontFileResponse(); $response->setContent($data); + $response->setMimeType($file->getMimeType()); $response->setCacheDurationInSeconds(60 * 60 * 24 * 30); return $response; diff --git a/src/applications/files/storage/file/PhabricatorFile.php b/src/applications/files/storage/file/PhabricatorFile.php index 9f8199de18..8feb56cbfc 100644 --- a/src/applications/files/storage/file/PhabricatorFile.php +++ b/src/applications/files/storage/file/PhabricatorFile.php @@ -25,6 +25,7 @@ class PhabricatorFile extends PhabricatorFileDAO { protected $mimeType; protected $byteSize; protected $authorPHID; + protected $secretKey; protected $storageEngine; protected $storageFormat; @@ -222,9 +223,14 @@ class PhabricatorFile extends PhabricatorFileDAO { } public function getViewURI() { + if (!$this->getPHID()) { + throw new Exception( + "You must save a file before you can generate a view URI."); + } + $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain'); if ($alt) { - $path = '/file/alt/'.$this->generateSecretKey().'/'.$this->getPHID().'/'; + $path = '/file/alt/'.$this->getSecretKey().'/'.$this->getPHID().'/'; $uri = new PhutilURI($alt); $uri->setPath($path); @@ -318,13 +324,17 @@ class PhabricatorFile extends PhabricatorFileDAO { } public function validateSecretKey($key) { - return ($key == $this->generateSecretKey()); + return ($key == $this->getSecretKey()); } - private function generateSecretKey() { - $file_key = PhabricatorEnv::getEnvConfig('phabricator.file-key'); - $hash = sha1($this->phid.$this->storageHandle.$file_key); - return substr($hash, 0, 20); + public function save() { + if (!$this->getSecretKey()) { + $this->setSecretKey($this->generateSecretKey()); + } + return parent::save(); } + public function generateSecretKey() { + return Filesystem::readRandomCharacters(20); + } }