Page MenuHomePhabricator
Files F14061201

D19898.diff
No OneTemporary
Actions

D19898.diff
View Options

diff --git a/src/applications/auth/action/PhabricatorAuthTryFactorAction.php b/src/applications/auth/action/PhabricatorAuthTryFactorAction.php
--- a/src/applications/auth/action/PhabricatorAuthTryFactorAction.php
+++ b/src/applications/auth/action/PhabricatorAuthTryFactorAction.php
@@ -9,7 +9,7 @@
}
public function getScoreThreshold() {
- return 100 / phutil_units('1 hour in seconds');
+ return 10 / phutil_units('1 hour in seconds');
}
public function getLimitExplanation() {
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
@@ -567,10 +567,21 @@
if ($request->getExists(AphrontRequest::TYPE_HISEC)) {
// Limit factor verification rates to prevent brute force attacks.
- PhabricatorSystemActionEngine::willTakeAction(
- array($viewer->getPHID()),
- new PhabricatorAuthTryFactorAction(),
- 1);
+ $any_attempt = false;
+ foreach ($factors as $factor) {
+ $impl = $factor->requireImplementation();
+ if ($impl->getRequestHasChallengeResponse($factor, $request)) {
+ $any_attempt = true;
+ break;
+ }
+ }
+
+ if ($any_attempt) {
+ PhabricatorSystemActionEngine::willTakeAction(
+ array($viewer->getPHID()),
+ new PhabricatorAuthTryFactorAction(),
+ 1);
+ }
foreach ($factors as $factor) {
$factor_phid = $factor->getPHID();
@@ -610,10 +621,12 @@
}
// Give the user a credit back for a successful factor verification.
- PhabricatorSystemActionEngine::willTakeAction(
- array($viewer->getPHID()),
- new PhabricatorAuthTryFactorAction(),
- -1);
+ if ($any_attempt) {
+ PhabricatorSystemActionEngine::willTakeAction(
+ array($viewer->getPHID()),
+ new PhabricatorAuthTryFactorAction(),
+ -1);
+ }
if ($session->getIsPartial() && !$jump_into_hisec) {
// If we have a partial session and are not jumping directly into
diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php
--- a/src/applications/auth/factor/PhabricatorAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorAuthFactor.php
@@ -52,6 +52,10 @@
->setWorkflowKey($engine->getWorkflowKey());
}
+ abstract public function getRequestHasChallengeResponse(
+ PhabricatorAuthFactorConfig $config,
+ AphrontRequest $response);
+
final public function getNewIssuedChallenges(
PhabricatorAuthFactorConfig $config,
PhabricatorUser $viewer,
diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
--- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
@@ -80,7 +80,7 @@
$okay = (bool)$this->getTimestepAtWhichResponseIsValid(
$this->getAllowedTimesteps($this->getCurrentTimestep()),
new PhutilOpaqueEnvelope($key),
- (string)$code);
+ $code);
if ($okay) {
$config = $this->newConfigForUser($user)
@@ -206,9 +206,10 @@
if (!$control) {
$value = $result->getValue();
$error = $result->getErrorMessage();
+ $name = $this->getChallengeResponseParameterName($config);
$control = id(new PHUIFormNumberControl())
- ->setName($this->getParameterName($config, 'totpcode'))
+ ->setName($name)
->setDisableAutocomplete(true)
->setValue($value)
->setError($error);
@@ -221,6 +222,15 @@
$form->appendChild($control);
}
+ public function getRequestHasChallengeResponse(
+ PhabricatorAuthFactorConfig $config,
+ AphrontRequest $request) {
+
+ $value = $this->getChallengeResponseFromRequest($config, $request);
+ return (bool)strlen($value);
+ }
+
+
protected function newResultFromIssuedChallenges(
PhabricatorAuthFactorConfig $config,
PhabricatorUser $viewer,
@@ -301,7 +311,9 @@
AphrontRequest $request,
array $challenges) {
- $code = $request->getStr($this->getParameterName($config, 'totpcode'));
+ $code = $this->getChallengeResponseFromRequest(
+ $config,
+ $request);
$result = $this->newResult()
->setValue($code);
@@ -335,13 +347,10 @@
$valid_timestep = $this->getTimestepAtWhichResponseIsValid(
array_intersect_key($challenge_timesteps, $current_timesteps),
new PhutilOpaqueEnvelope($config->getFactorSecret()),
- (string)$code);
+ $code);
if ($valid_timestep) {
- $now = PhabricatorTime::getNow();
- $step_duration = $this->getTimestepDuration();
- $step_window = $this->getTimestepWindowSize();
- $ttl = $now + ($step_duration * $step_window);
+ $ttl = PhabricatorTime::getNow() + 60;
$challenge
->setProperty('totp.timestep', $valid_timestep)
@@ -475,7 +484,7 @@
// The user is allowed to provide a code from the recent past or the
// near future to account for minor clock skew between the client
// and server, and the time it takes to actually enter a code.
- return 2;
+ return 1;
}
private function getTimestepAtWhichResponseIsValid(
@@ -493,6 +502,21 @@
return null;
}
+ private function getChallengeResponseParameterName(
+ PhabricatorAuthFactorConfig $config) {
+ return $this->getParameterName($config, 'totpcode');
+ }
+
+ private function getChallengeResponseFromRequest(
+ PhabricatorAuthFactorConfig $config,
+ AphrontRequest $request) {
+ $name = $this->getChallengeResponseParameterName($config);
+ $value = $request->getStr($name);
+ $value = (string)$value;
+ $value = trim($value);
+
+ return $value;
+ }
}

File Metadata

Mime Type
text/plain
Expires
Tue, Nov 19, 5:34 AM (1 h, 45 m ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6761263
Default Alt Text
D19898.diff (5 KB)

Event Timeline

Phabricator · Built by Phacility