mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-09 16:32:39 +01:00
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
This commit is contained in:
parent
5f1b3937e5
commit
e45ffda55a
11 changed files with 75 additions and 4 deletions
|
@ -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
|
||||
|
|
|
@ -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',
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
|
|
|
@ -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');
|
||||
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
|
|
@ -7,6 +7,7 @@
|
|||
|
||||
|
||||
phutil_require_module('phabricator', 'applications/people/storage/base');
|
||||
phutil_require_module('phabricator', 'infrastructure/util/hash');
|
||||
|
||||
phutil_require_module('phutil', 'utils');
|
||||
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -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');
|
||||
|
||||
|
|
|
@ -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 =
|
||||
|
|
31
src/infrastructure/util/hash/PhabricatorHash.php
Normal file
31
src/infrastructure/util/hash/PhabricatorHash.php
Normal file
|
@ -0,0 +1,31 @@
|
|||
<?php
|
||||
|
||||
/*
|
||||
* Copyright 2011 Facebook, Inc.
|
||||
*
|
||||
* Licensed under the Apache License, Version 2.0 (the "License");
|
||||
* you may not use this file except in compliance with the License.
|
||||
* You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing, software
|
||||
* distributed under the License is distributed on an "AS IS" BASIS,
|
||||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||
* See the License for the specific language governing permissions and
|
||||
* limitations under the License.
|
||||
*/
|
||||
|
||||
final class PhabricatorHash {
|
||||
|
||||
public static function digest($string) {
|
||||
$key = PhabricatorEnv::getEnvConfig('security.hmac-key');
|
||||
if (!$key) {
|
||||
throw new Exception(
|
||||
"Set a 'security.hmac-key' in your Phabricator configuration!");
|
||||
}
|
||||
|
||||
return hash_hmac('sha1', $string, $key);
|
||||
}
|
||||
|
||||
}
|
12
src/infrastructure/util/hash/__init__.php
Normal file
12
src/infrastructure/util/hash/__init__.php
Normal file
|
@ -0,0 +1,12 @@
|
|||
<?php
|
||||
/**
|
||||
* This file is automatically generated. Lint this module to rebuild it.
|
||||
* @generated
|
||||
*/
|
||||
|
||||
|
||||
|
||||
phutil_require_module('phabricator', 'infrastructure/env');
|
||||
|
||||
|
||||
phutil_require_source('PhabricatorHash.php');
|
Loading…
Reference in a new issue