mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-26 16:52:41 +01:00
Upgrade an old "weakDigest()" inside TOTP synchronization code
Summary: Ref T13222. Ref T12509. When you add a new MFA TOTP authenticator, we generate a temporary token to make sure you're actually adding the key we generated and not picking your own key. That is, if we just put inputs in the form like `key=123, response=456`, users could pick their own keys by changing the value of `key` and then generating the correct `response`. That's probably fine, but maybe attackers could somehow force users to pick known keys in combination with other unknown vulnerabilities that might exist in the future. Instead, we generate a random key and keep track of it to make sure nothing funny is afoot. As an additional barrier, we do the standard "store the digest, not the real key" sort of thing so you can't force a known value even if you can read the database (although this is mostly pointless since you can just read TOTP secrets directly if you can read the database). But it's pretty standard and doesn't hurt anything. Update this from SHA1 to SHA256. This will break any TOTP factors which someone was in the middle of adding during a Phabricator upgrade, but that seems reasonable. They'll get a sensible failure mode. Test Plan: Added a new TOTP factor. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222, T12509 Differential Revision: https://secure.phabricator.com/D19884
This commit is contained in:
parent
1d34238dc9
commit
080fb1985f
1 changed files with 12 additions and 2 deletions
|
@ -2,6 +2,8 @@
|
|||
|
||||
final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
|
||||
|
||||
const DIGEST_TEMPORARY_KEY = 'mfa.totp.sync';
|
||||
|
||||
public function getFactorKey() {
|
||||
return 'totp';
|
||||
}
|
||||
|
@ -34,12 +36,16 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
|
|||
// (We store and verify the hash of the key, not the key itself, to limit
|
||||
// how useful the data in the table is to an attacker.)
|
||||
|
||||
$token_code = PhabricatorHash::digestWithNamedKey(
|
||||
$key,
|
||||
self::DIGEST_TEMPORARY_KEY);
|
||||
|
||||
$temporary_token = id(new PhabricatorAuthTemporaryTokenQuery())
|
||||
->setViewer($user)
|
||||
->withTokenResources(array($user->getPHID()))
|
||||
->withTokenTypes(array($totp_token_type))
|
||||
->withExpired(false)
|
||||
->withTokenCodes(array(PhabricatorHash::weakDigest($key)))
|
||||
->withTokenCodes(array($token_code))
|
||||
->executeOne();
|
||||
if (!$temporary_token) {
|
||||
// If we don't have a matching token, regenerate the key below.
|
||||
|
@ -53,12 +59,16 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
|
|||
// Mark this key as one we generated, so the user is allowed to submit
|
||||
// a response for it.
|
||||
|
||||
$token_code = PhabricatorHash::digestWithNamedKey(
|
||||
$key,
|
||||
self::DIGEST_TEMPORARY_KEY);
|
||||
|
||||
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
|
||||
id(new PhabricatorAuthTemporaryToken())
|
||||
->setTokenResource($user->getPHID())
|
||||
->setTokenType($totp_token_type)
|
||||
->setTokenExpires(time() + phutil_units('1 hour in seconds'))
|
||||
->setTokenCode(PhabricatorHash::weakDigest($key))
|
||||
->setTokenCode($token_code)
|
||||
->save();
|
||||
unset($unguarded);
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue