Skip to content

Commit

Permalink
Hide errors in production during rate limit check
Browse files Browse the repository at this point in the history
The rate limit error is treated like an invalid user, which allows Kirby to trigger the artificial `usleep()` delay even if the rate limit is reached.

GHSA-c27j-76xg-6x4f

# Conflicts:
#	src/Cms/Auth.php
  • Loading branch information
lukasbestle committed Oct 17, 2022
1 parent 501fd97 commit cf0b8c7
Showing 1 changed file with 21 additions and 21 deletions.
42 changes: 21 additions & 21 deletions src/Cms/Auth.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,6 @@ public function __construct(App $kirby)
public function createChallenge(string $email, bool $long = false, string $mode = 'login')
{
$email = Idn::decodeEmail($email);
$this->checkRateLimit($email);

// rate-limit the number of challenges for DoS/DDoS protection
$this->track($email, false);

$session = $this->kirby->session([
'createMode' => 'cookie',
Expand All @@ -109,6 +105,11 @@ public function createChallenge(string $email, bool $long = false, string $mode
// catch every exception to hide them from attackers
// unless auth debugging is enabled
try {
$this->checkRateLimit($email);

// rate-limit the number of challenges for DoS/DDoS protection
$this->track($email, false);

$timeout = $this->kirby->option('auth.challenge.timeout', 10 * 60);

// try to find the provided user
Expand Down Expand Up @@ -508,14 +509,10 @@ protected function checkRateLimit(string $email): void
if ($this->isBlocked($email) === true) {
$this->kirby->trigger('user.login:failed', compact('email'));

if ($this->kirby->option('debug') === true) {
$message = 'Rate limit exceeded';
} else {
// avoid leaking security-relevant information
$message = ['key' => 'access.login'];
}

throw new PermissionException($message);
throw new PermissionException([
'details' => ['reason' => 'rate-limited'],
'fallback' => 'Rate limit exceeded'
]);
}
}

Expand All @@ -534,10 +531,11 @@ protected function checkRateLimit(string $email): void
public function validatePassword(string $email, string $password)
{
$email = Idn::decodeEmail($email);
$this->checkRateLimit($email);

// validate the user
try {
$this->checkRateLimit($email);

// validate the user and its password
if ($user = $this->kirby->users()->find($email)) {
if ($user->validatePassword($password) === true) {
return $user;
Expand All @@ -551,8 +549,10 @@ public function validatePassword(string $email, string $password)
]
]);
} catch (Throwable $e) {
// log invalid login trial
$this->track($email);
// log invalid login trial unless the rate limit is already active
if (($e->getDetails()['reason'] ?? null) !== 'rate-limited') {
$this->track($email);
}

// sleep for a random amount of milliseconds
// to make automated attacks harder
Expand Down Expand Up @@ -808,10 +808,7 @@ public function verifyChallenge(string $code)
}

// rate-limiting
if ($this->isBlocked($email) === true) {
$this->kirby->trigger('user.login:failed', compact('email'));
throw new PermissionException('Rate limit exceeded');
}
$this->checkRateLimit($email);

// time-limiting
$timeout = $session->get('kirby.challenge.timeout');
Expand Down Expand Up @@ -840,7 +837,10 @@ class_exists(static::$challenges[$challenge]) === true &&

throw new LogicException('Invalid authentication challenge: ' . $challenge);
} catch (Throwable $e) {
if (empty($email) === false && $e->getMessage() !== 'Rate limit exceeded') {
if (
empty($email) === false &&
($e->getDetails()['reason'] ?? null) !== 'rate-limited'
) {
$this->track($email);
}

Expand Down

0 comments on commit cf0b8c7

Please sign in to comment.