Page MenuHomePhabricator
Files F15388392

D8911.id21146.diff
No OneTemporary
Actions

D8911.id21146.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1257,6 +1257,7 @@
'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php',
'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php',
'PhabricatorAuthTerminateSessionController' => 'applications/auth/controller/PhabricatorAuthTerminateSessionController.php',
+ 'PhabricatorAuthTryFactorAction' => 'applications/auth/action/PhabricatorAuthTryFactorAction.php',
'PhabricatorAuthUnlinkController' => 'applications/auth/controller/PhabricatorAuthUnlinkController.php',
'PhabricatorAuthValidateController' => 'applications/auth/controller/PhabricatorAuthValidateController.php',
'PhabricatorAuthenticationConfigOptions' => 'applications/config/option/PhabricatorAuthenticationConfigOptions.php',
@@ -4020,6 +4021,7 @@
'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
'PhabricatorAuthStartController' => 'PhabricatorAuthController',
'PhabricatorAuthTerminateSessionController' => 'PhabricatorAuthController',
+ 'PhabricatorAuthTryFactorAction' => 'PhabricatorSystemAction',
'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController',
'PhabricatorAuthValidateController' => 'PhabricatorAuthController',
'PhabricatorAuthenticationConfigOptions' => 'PhabricatorApplicationConfigOptions',
diff --git a/src/applications/auth/action/PhabricatorAuthTryFactorAction.php b/src/applications/auth/action/PhabricatorAuthTryFactorAction.php
new file mode 100644
--- /dev/null
+++ b/src/applications/auth/action/PhabricatorAuthTryFactorAction.php
@@ -0,0 +1,21 @@
+<?php
+
+final class PhabricatorAuthTryFactorAction extends PhabricatorSystemAction {
+
+ const TYPECONST = 'auth.factor';
+
+ public function getActionConstant() {
+ return self::TYPECONST;
+ }
+
+ public function getScoreThreshold() {
+ return 10 / phutil_units('1 hour in seconds');
+ }
+
+ public function getLimitExplanation() {
+ return pht(
+ 'You have failed to verify multi-factor authentication too often in '.
+ 'a short period of time.');
+ }
+
+}
diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -247,11 +247,24 @@
return $this->issueHighSecurityToken($session, true);
}
+ // Check for a rate limit without awarding points, so the user doesn't
+ // get partway through the workflow only to get blocked.
+ PhabricatorSystemActionEngine::willTakeAction(
+ array($viewer->getPHID()),
+ new PhabricatorAuthTryFactorAction(),
+ 0);
+
$validation_results = array();
if ($request->isHTTPPost()) {
$request->validateCSRF();
if ($request->getExists(AphrontRequest::TYPE_HISEC)) {
+ // Limit factor verification rates to prevent brute force attacks.
+ PhabricatorSystemActionEngine::willTakeAction(
+ array($viewer->getPHID()),
+ new PhabricatorAuthTryFactorAction(),
+ 1);
+
$ok = true;
foreach ($factors as $factor) {
$id = $factor->getID();
@@ -268,6 +281,12 @@
}
if ($ok) {
+ // Give the user a credit back for a successful factor verification.
+ PhabricatorSystemActionEngine::willTakeAction(
+ array($viewer->getPHID()),
+ new PhabricatorAuthTryFactorAction(),
+ -1);
+
$until = time() + phutil_units('15 minutes in seconds');
$session->setHighSecurityUntil($until);
@@ -284,6 +303,9 @@
PhabricatorUserLog::ACTION_ENTER_HISEC);
$log->save();
} else {
+
+
+
$log = PhabricatorUserLog::initializeNewLog(
$viewer,
$viewer->getPHID(),
diff --git a/src/applications/system/engine/PhabricatorSystemActionEngine.php b/src/applications/system/engine/PhabricatorSystemActionEngine.php
--- a/src/applications/system/engine/PhabricatorSystemActionEngine.php
+++ b/src/applications/system/engine/PhabricatorSystemActionEngine.php
@@ -2,6 +2,42 @@
final class PhabricatorSystemActionEngine extends Phobject {
+ /**
+ * Prepare to take an action, throwing an exception if the user has exceeded
+ * the rate limit.
+ *
+ * The `$actors` are a list of strings. Normally this will be a list of
+ * user PHIDs, but some systems use other identifiers (like email
+ * addresses). Each actor's score threshold is tracked independently. If
+ * any actor exceeds the rate limit for the action, this method throws.
+ *
+ * The `$action` defines the actual thing being rate limited, and sets the
+ * limit.
+ *
+ * You can pass either a positive, zero, or negative `$score` to this method:
+ *
+ * - If the score is positive, the user is given that many points toward
+ * the rate limit after the limit is checked. Over time, this will cause
+ * them to hit the rate limit and be prevented from taking further
+ * actions.
+ * - If the score is zero, the rate limit is checked but no score changes
+ * are made. This allows you to check for a rate limit before beginning
+ * a workflow, so the user doesn't fill in a form only to get rate limited
+ * at the end.
+ * - If the score is negative, the user is credited points, allowing them
+ * to take more actions than the limit normally permits. By awarding
+ * points for failed actions and credits for successful actions, a
+ * system can be sensitive to failure without overly restricting
+ * legitimate uses.
+ *
+ * If any actor is exceeding their rate limit, this method throws a
+ * @{class:PhabricatorSystemActionRateLimitException}.
+ *
+ * @param list<string> List of actors.
+ * @param PhabricatorSystemAction Action being taken.
+ * @param float Score or credit, see above.
+ * @return void
+ */
public static function willTakeAction(
array $actors,
PhabricatorSystemAction $action,
@@ -20,9 +56,11 @@
}
}
- $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
- self::recordAction($actors, $action, $score);
- unset($unguarded);
+ if ($score != 0) {
+ $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+ self::recordAction($actors, $action, $score);
+ unset($unguarded);
+ }
}
public static function loadBlockedActors(
@@ -35,9 +73,17 @@
$blocked = array();
foreach ($scores as $actor => $actor_score) {
- $actor_score = $actor_score + ($score / $window);
+ // For the purposes of checking for a block, we just use the raw
+ // persistent score and do not include the score for this action. This
+ // allows callers to test for a block without adding any points and get
+ // the same result they would if they were adding points: we only
+ // trigger a rate limit when the persistent score exceeds the threshold.
if ($action->shouldBlockActor($actor, $actor_score)) {
- $blocked[$actor] = $actor_score;
+ // When reporting the results, we do include the points for this
+ // action. This makes the error messages more clear, since they
+ // more accurately report the number of actions the user has really
+ // tried to take.
+ $blocked[$actor] = $actor_score + ($score / $window);
}
}

File Metadata

Mime Type
text/plain
Expires
Sun, Mar 16, 3:46 AM (2 w, 6 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7705564
Default Alt Text
D8911.id21146.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility