From e45ffda55a9aeca0d6429ab9f5d6bfa7adefa3fb Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 18 Dec 2011 11:00:39 -0800 Subject: [PATCH] Move most remaining sha1() calls to HMAC Summary: - For context, see T547. This is the last (maybe?) in a series of diffs that moves us off raw sha1() calls in order to make it easier to audit the codebase for correct use of hash functions. - This breaks CSRF tokens. Any open forms will generate an error when submitted, so maybe upgrade off-peak. - We now generate HMAC mail keys but accept MAC or HMAC. In a few months, we can remove the MAC version. - The only remaining callsite is Conduit. We can't use HMAC since Arcanist would need to know the key. {T550} provides a better solution to this, anyway. Test Plan: - Verified CSRF tokens generate properly. - Manually changed CSRF to an incorrect value and got an error. - Verified mail generates with a new mail hash. - Verified Phabricator accepts both old and new mail hashes. - Verified Phabricator rejects bad mail hashes. - Checked user log, things look OK. Reviewers: btrahan, jungejason, benmathews Reviewed By: btrahan CC: aran, epriestley, btrahan Maniphest Tasks: T547 Differential Revision: 1237 --- conf/default.conf.php | 6 ++++ src/__phutil_library_map__.php | 1 + .../PhabricatorMetaMTAReceivedMail.php | 20 +++++++++++- .../metamta/storage/receivedmail/__init__.php | 1 + .../people/storage/log/PhabricatorUserLog.php | 2 +- .../people/storage/log/__init__.php | 1 + .../people/storage/user/PhabricatorUser.php | 2 +- .../people/storage/user/__init__.php | 1 + src/docs/userguide/diffusion.diviner | 2 +- .../util/hash/PhabricatorHash.php | 31 +++++++++++++++++++ src/infrastructure/util/hash/__init__.php | 12 +++++++ 11 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 src/infrastructure/util/hash/PhabricatorHash.php create mode 100644 src/infrastructure/util/hash/__init__.php diff --git a/conf/default.conf.php b/conf/default.conf.php index 078c4e128d..bbc0a31906 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -65,6 +65,12 @@ return array( // attack difficult, but it is viable unless you isolate the file domain. 'security.alternate-file-domain' => null, + // Default key for HMAC digests where the key is not important (i.e., the + // hash itself is secret). You can change this if you want (to any other + // string), but doing so will break existing sessions and CSRF tokens. + 'security.hmac-key' => '[D\t~Y7eNmnQGJ;rnH6aF;m2!vJ8@v8C=Cs:aQS\.Qw', + + // -- DarkConsole ----------------------------------------------------------- // // DarkConsole is a administrative debugging/profiling tool built into diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bab5bcf691..f6aa7a6666 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -471,6 +471,7 @@ phutil_register_library_map(array( 'PhabricatorGarbageCollectorDaemon' => 'infrastructure/daemon/garbagecollector', 'PhabricatorGoodForNothingWorker' => 'infrastructure/daemon/workers/worker/goodfornothing', 'PhabricatorHandleObjectSelectorDataView' => 'applications/phid/handle/view/selector', + 'PhabricatorHash' => 'infrastructure/util/hash', 'PhabricatorHelpController' => 'applications/help/controller/base', 'PhabricatorHelpKeyboardShortcutController' => 'applications/help/controller/keyboardshortcut', 'PhabricatorIRCBot' => 'infrastructure/daemon/irc/bot', diff --git a/src/applications/metamta/storage/receivedmail/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/receivedmail/PhabricatorMetaMTAReceivedMail.php index 860994c6c6..7f83330bd9 100644 --- a/src/applications/metamta/storage/receivedmail/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/receivedmail/PhabricatorMetaMTAReceivedMail.php @@ -176,7 +176,11 @@ class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { } $expect_hash = self::computeMailHash($receiver->getMailKey(), $check_phid); - if ($expect_hash != $hash) { + + // See note at computeOldMailHash(). + $old_hash = self::computeOldMailHash($receiver->getMailKey(), $check_phid); + + if ($expect_hash != $hash && $old_hash != $hash) { return $this->setMessage("Invalid mail hash!")->save(); } @@ -228,6 +232,20 @@ class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { public static function computeMailHash($mail_key, $phid) { $global_mail_key = PhabricatorEnv::getEnvConfig('phabricator.mail-key'); + $hash = PhabricatorHash::digest($mail_key.$global_mail_key.$phid); + return substr($hash, 0, 16); + } + + public static function computeOldMailHash($mail_key, $phid) { + + // TODO: Remove this method entirely in a couple of months. We've moved from + // plain sha1 to sha1+hmac to make the codebase more auditable for good uses + // of hash functions, but still accept the old hashes on email replies to + // avoid breaking things. Once we've been sending only hmac hashes for a + // while, remove this and start rejecting old hashes. See T547. + + $global_mail_key = PhabricatorEnv::getEnvConfig('phabricator.mail-key'); + $hash = sha1($mail_key.$global_mail_key.$phid); return substr($hash, 0, 16); } diff --git a/src/applications/metamta/storage/receivedmail/__init__.php b/src/applications/metamta/storage/receivedmail/__init__.php index 6254939d3f..37d4e56863 100644 --- a/src/applications/metamta/storage/receivedmail/__init__.php +++ b/src/applications/metamta/storage/receivedmail/__init__.php @@ -14,6 +14,7 @@ phutil_require_module('phabricator', 'applications/metamta/parser'); phutil_require_module('phabricator', 'applications/metamta/storage/base'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'infrastructure/env'); +phutil_require_module('phabricator', 'infrastructure/util/hash'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/people/storage/log/PhabricatorUserLog.php b/src/applications/people/storage/log/PhabricatorUserLog.php index abc0355b1f..339877e4bb 100644 --- a/src/applications/people/storage/log/PhabricatorUserLog.php +++ b/src/applications/people/storage/log/PhabricatorUserLog.php @@ -89,7 +89,7 @@ class PhabricatorUserLog extends PhabricatorUserDAO { // seeing the logs doesn't compromise all the sessions which appear in // them. This just prevents casual leaks, like in a screenshot. if (strlen($session)) { - $this->session = sha1($session); + $this->session = PhabricatorHash::digest($session); } return $this; } diff --git a/src/applications/people/storage/log/__init__.php b/src/applications/people/storage/log/__init__.php index 90f82b5ed8..3392d47eae 100644 --- a/src/applications/people/storage/log/__init__.php +++ b/src/applications/people/storage/log/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('phabricator', 'applications/people/storage/base'); +phutil_require_module('phabricator', 'infrastructure/util/hash'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/people/storage/user/PhabricatorUser.php b/src/applications/people/storage/user/PhabricatorUser.php index ca5eb1b5cd..0cb6990ad1 100644 --- a/src/applications/people/storage/user/PhabricatorUser.php +++ b/src/applications/people/storage/user/PhabricatorUser.php @@ -191,7 +191,7 @@ class PhabricatorUser extends PhabricatorUserDAO { private function generateToken($epoch, $frequency, $key, $len) { $time_block = floor($epoch / $frequency); $vec = $this->getPHID().$this->getPasswordHash().$key.$time_block; - return substr(sha1($vec), 0, $len); + return substr(PhabricatorHash::digest($vec), 0, $len); } /** diff --git a/src/applications/people/storage/user/__init__.php b/src/applications/people/storage/user/__init__.php index c93a02dfda..8cdfe8db6e 100644 --- a/src/applications/people/storage/user/__init__.php +++ b/src/applications/people/storage/user/__init__.php @@ -14,6 +14,7 @@ phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'applications/phid/storage/phid'); phutil_require_module('phabricator', 'applications/search/index/indexer/user'); phutil_require_module('phabricator', 'infrastructure/env'); +phutil_require_module('phabricator', 'infrastructure/util/hash'); phutil_require_module('phabricator', 'storage/qsprintf'); phutil_require_module('phabricator', 'storage/queryfx'); diff --git a/src/docs/userguide/diffusion.diviner b/src/docs/userguide/diffusion.diviner index d59960ced5..e4a33dae7d 100644 --- a/src/docs/userguide/diffusion.diviner +++ b/src/docs/userguide/diffusion.diviner @@ -58,7 +58,7 @@ of 7-character hashes: Because 7-character hashes are likely to collide for even moderately large repositories, Diffusion generally uses either a 16-character prefix (which makes -collisions very unlikely) or the full 40-character SHA1 (which makes collisions +collisions very unlikely) or the full 40-character hash (which makes collisions astronomically unlikely). = Adding Repositories = diff --git a/src/infrastructure/util/hash/PhabricatorHash.php b/src/infrastructure/util/hash/PhabricatorHash.php new file mode 100644 index 0000000000..9a54d29c10 --- /dev/null +++ b/src/infrastructure/util/hash/PhabricatorHash.php @@ -0,0 +1,31 @@ +