1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-30 02:32:42 +01:00

Add a rate limit for enroll attempts when adding new MFA configurations

Summary:
Depends on D20018. Ref T13222. When you add a new MFA configuration, you can technically (?) guess your way through it with brute force. It's not clear why this would ever really be useful (if an attacker can get here and wants to add TOTP, they can just add TOTP!) but it's probably bad, so don't let users do it.

This limit is fairly generous because I don't think this actually part of any real attack, at least today with factors we're considering.

Test Plan:
  - Added TOTP, guessed wrong a ton of times, got rate limited.
  - Added TOTP, guessed right, got a TOTP factor configuration added to my account.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20019
This commit is contained in:
epriestley 2019-01-23 07:22:10 -08:00
parent e91bc26da6
commit 7c1d1c13f4
3 changed files with 47 additions and 0 deletions

View file

@ -2296,6 +2296,7 @@ phutil_register_library_map(array(
'PhabricatorAuthNeedsApprovalController' => 'applications/auth/controller/PhabricatorAuthNeedsApprovalController.php',
'PhabricatorAuthNeedsMultiFactorController' => 'applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php',
'PhabricatorAuthNewController' => 'applications/auth/controller/config/PhabricatorAuthNewController.php',
'PhabricatorAuthNewFactorAction' => 'applications/auth/action/PhabricatorAuthNewFactorAction.php',
'PhabricatorAuthOldOAuthRedirectController' => 'applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php',
'PhabricatorAuthOneTimeLoginController' => 'applications/auth/controller/PhabricatorAuthOneTimeLoginController.php',
'PhabricatorAuthOneTimeLoginTemporaryTokenType' => 'applications/auth/tokentype/PhabricatorAuthOneTimeLoginTemporaryTokenType.php',
@ -8021,6 +8022,7 @@ phutil_register_library_map(array(
'PhabricatorAuthNeedsApprovalController' => 'PhabricatorAuthController',
'PhabricatorAuthNeedsMultiFactorController' => 'PhabricatorAuthController',
'PhabricatorAuthNewController' => 'PhabricatorAuthProviderConfigController',
'PhabricatorAuthNewFactorAction' => 'PhabricatorSystemAction',
'PhabricatorAuthOldOAuthRedirectController' => 'PhabricatorAuthController',
'PhabricatorAuthOneTimeLoginController' => 'PhabricatorAuthController',
'PhabricatorAuthOneTimeLoginTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',

View file

@ -0,0 +1,21 @@
<?php
final class PhabricatorAuthNewFactorAction extends PhabricatorSystemAction {
const TYPECONST = 'auth.factor.new';
public function getActionConstant() {
return self::TYPECONST;
}
public function getScoreThreshold() {
return 60 / phutil_units('1 hour in seconds');
}
public function getLimitExplanation() {
return pht(
'You have failed too many attempts to synchronize new multi-factor '.
'authentication methods in a short period of time.');
}
}

View file

@ -234,12 +234,36 @@ final class PhabricatorMultiFactorSettingsPanel
$form = id(new AphrontFormView())
->setViewer($viewer);
if ($request->isFormPost()) {
// Subject users to rate limiting so that it's difficult to add factors
// by pure brute force. This is normally not much of an attack, but push
// factor types may have side effects.
PhabricatorSystemActionEngine::willTakeAction(
array($viewer->getPHID()),
new PhabricatorAuthNewFactorAction(),
1);
} else {
// Test the limit before showing the user a form, so we don't give them
// a form which can never possibly work because it will always hit rate
// limiting.
PhabricatorSystemActionEngine::willTakeAction(
array($viewer->getPHID()),
new PhabricatorAuthNewFactorAction(),
0);
}
$config = $selected_provider->processAddFactorForm(
$form,
$request,
$user);
if ($config) {
// If the user added a factor, give them a rate limiting point back.
PhabricatorSystemActionEngine::willTakeAction(
array($viewer->getPHID()),
new PhabricatorAuthNewFactorAction(),
-1);
$config->save();
$log = PhabricatorUserLog::initializeNewLog(