From 51a0e793f0ca08a6e0ea0ad6b7181e662946eeef Mon Sep 17 00:00:00 2001
From: Jan-Hendrik Willms <tleilax+studip@gmail.com>
Date: Wed, 13 Nov 2024 14:56:58 +0000
Subject: [PATCH] fix 2fa token waiting (and more cleaning up than should be,
sorry), fixes #4855
Closes #4855
Merge request studip/studip!3640
---
lib/classes/Request.php | 2 ++
lib/classes/TwoFactorAuth.php | 52 +++++++++++++++++++----------------
templates/tfa-validate.php | 16 +++++++++--
3 files changed, 43 insertions(+), 27 deletions(-)
diff --git a/lib/classes/Request.php b/lib/classes/Request.php
index 1bb10798d82..ae294ae87d2 100644
--- a/lib/classes/Request.php
+++ b/lib/classes/Request.php
@@ -77,6 +77,8 @@ class Request implements ArrayAccess, IteratorAggregate
/**
* IteratorAggregate: Create iterator for request parameters.
+ *
+ * @return ArrayIterator
*/
public function getIterator(): Traversable
{
diff --git a/lib/classes/TwoFactorAuth.php b/lib/classes/TwoFactorAuth.php
index 6c04b8e1ad8..b67f4befa26 100644
--- a/lib/classes/TwoFactorAuth.php
+++ b/lib/classes/TwoFactorAuth.php
@@ -11,8 +11,6 @@
final class TwoFactorAuth
{
const SESSION_KEY = 'tfa/confirmed';
- const SESSION_REDIRECT = 'tfa/redirect';
- const SESSION_ENFORCE = 'tfa/enforce';
const SESSION_DATA = 'tfa/data';
const SESSION_CONFIRMATIONS = 'tfa/confirmations';
const SESSION_FAILED = 'tfa/failed';
@@ -26,7 +24,7 @@ final class TwoFactorAuth
* Returns an instance of the authentication
* @return TwoFactorAuth object
*/
- public static function get()
+ public static function get(): TwoFactorAuth
{
if (self::$instance === null) {
self::$instance = new self();
@@ -39,16 +37,16 @@ final class TwoFactorAuth
* user (defaults to current user). The user's permissions decide whether
* the two factor authentication is enabled or not.
*
- * @param User $user User to check (optional, defaults to current user)
+ * @param User|null $user User to check (optional, defaults to current user)
* @return boolean
*/
- public static function isEnabledForUser(User $user = null)
+ public static function isEnabledForUser(User $user = null): bool
{
if ($user === null) {
$user = User::findCurrent();
}
- $valid_perms = array_filter(array_map('trim', explode(',', Config::get()->TFA_PERMS)));
+ $valid_perms = array_filter(array_map('trim', explode(',', Config::get()->getValue('TFA_PERMS'))));
return in_array($user->perms, $valid_perms);
}
@@ -67,11 +65,13 @@ final class TwoFactorAuth
/**
* Private constructor to enforce singleton
+ *
+ * @throws SessionRequiredException
*/
private function __construct()
{
if (session_status() !== PHP_SESSION_ACTIVE) {
- throw new Exception('2FA requires a valid session');
+ throw new SessionRequiredException('2FA requires a valid session');
}
if (!isset($_SESSION[self::SESSION_FAILED])) {
@@ -81,7 +81,7 @@ final class TwoFactorAuth
$_SESSION[self::SESSION_FAILED] = array_filter(
$_SESSION[self::SESSION_FAILED],
function ($timestamp) {
- return $timestamp > time() - Config::get()->TFA_MAX_TRIES_TIMESPAN;
+ return $timestamp > time() - Config::get()->getValue('TFA_MAX_TRIES_TIMESPAN');
}
);
}
@@ -145,7 +145,7 @@ final class TwoFactorAuth
// User has already confirmed this session?
if (isset($_SESSION[self::SESSION_KEY])) {
- list($code, $timeslice) = array_values($_SESSION[self::SESSION_KEY]);
+ [$code, $timeslice] = array_values($_SESSION[self::SESSION_KEY]);
if ($this->secret->validateToken($code, (int) $timeslice, true)) {
return;
}
@@ -155,7 +155,7 @@ final class TwoFactorAuth
// Trusted computer?
$user_cookie_key = self::COOKIE_KEY . '/' . $GLOBALS['user']->id;
if (isset($_COOKIE[$user_cookie_key])) {
- list($code, $timeslice) = explode(':', $_COOKIE[$user_cookie_key]);
+ [$code, $timeslice] = explode(':', $_COOKIE[$user_cookie_key]);
if ($this->secret->validateToken($code, (int) $timeslice, true)) {
$this->registerSecretInSession();
return;
@@ -177,7 +177,7 @@ final class TwoFactorAuth
* @param array $data Optional additional data to pass to the
* confirmation screen (for internal use)
*/
- public function confirm($action, $text, array $data = []): void
+ public function confirm(string $action, string $text, array $data = []): void
{
if (
isset($_SESSION[self::SESSION_CONFIRMATIONS])
@@ -202,7 +202,7 @@ final class TwoFactorAuth
* @param string $text Text to display to the user
* @param array $data Optional additional data (for internal use)
*/
- private function showConfirmationScreen($text = '', array $data = [])
+ private function showConfirmationScreen(string $text = '', array $data = [])
{
$data = array_merge(['global' => false], $data);
@@ -235,8 +235,8 @@ final class TwoFactorAuth
$_SESSION[self::SESSION_DATA] + [
'secret' => $this->secret,
'text' => $text,
- 'blocked' => $this->isBlocked(),
- 'duration' => Config::get()->TFA_TRUST_DURATION,
+ 'waittime' => $this->getWaitingTime(),
+ 'duration' => Config::get()->getValue('TFA_TRUST_DURATION'),
],
'layouts/base.php'
);
@@ -263,7 +263,7 @@ final class TwoFactorAuth
*/
private function registerSecretInCookie()
{
- $lifetime_in_days = Config::get()->TFA_TRUST_DURATION;
+ $lifetime_in_days = Config::get()->getValue('TFA_TRUST_DURATION');
$lifetime = $lifetime_in_days > 0 ? strtotime("+{$lifetime_in_days} days") : 2147483647;
$timeslice = mt_rand(0, PHP_INT_MAX);
@@ -288,7 +288,7 @@ final class TwoFactorAuth
private function validateFromRequest()
{
if (
- $this->isBlocked()
+ $this->getWaitingTime()
|| !Request::isPost()
|| !Request::submitted('tfacode-input')
|| !Request::submitted('tfa-nonce')
@@ -341,16 +341,20 @@ final class TwoFactorAuth
}
/**
- * Returns whether the current session is blocked from any more token
- * inputs. This happens if too many false inputs happen in a short amount
- * of time and should prevent brute force attacks.
+ * Returns the time the user has to wait before entering a token.
*
- * @return boolean
+ * This happens if too many invalid tokens have been input.
*/
- private function isBlocked()
+ private function getWaitingTime(): int
{
- return count($_SESSION[self::SESSION_FAILED]) >= Config::get()->TFA_MAX_TRIES
- ? min($_SESSION[self::SESSION_FAILED])
- : false;
+ if (count($_SESSION[self::SESSION_FAILED]) < Config::get()->getValue('TFA_MAX_TRIES')) {
+ return 0;
+ }
+
+ $min_timestamp = min(array_slice($_SESSION[self::SESSION_FAILED], -Config::get()->getValue('TFA_MAX_TRIES')));
+
+ $wait_time = $min_timestamp + Config::get()->getValue('TFA_MAX_TRIES_TIMESPAN') - time();
+
+ return ceil($wait_time / 60);
}
}
diff --git a/templates/tfa-validate.php b/templates/tfa-validate.php
index f5221bc705f..f3a682155cd 100644
--- a/templates/tfa-validate.php
+++ b/templates/tfa-validate.php
@@ -1,17 +1,27 @@
+<?php
+/**
+ * @var string $__nonce
+ * @var int $waittime
+ * @var string $text
+ * @var TFASecret $secret
+ * @var bool|int $global
+ * @var int $duration
+ */
+?>
<form action="<?= htmlReady(Request::url()) ?>" method="post" class="default">
<input type="hidden" name="tfa-nonce" value="<?= htmlReady($__nonce) ?>">
<fieldset>
<legend><?= _('Zwei-Faktor-Authentifizierung') ?></legend>
-<? if ($blocked): ?>
+<? if ($waittime): ?>
<?= MessageBox::warning(_('Sie haben zu viele ungültige Versuche'), [sprintf(
_('Versuchen Sie es in %u Minute(n) erneut'),
- ceil((time() - $blocked) / 60)
+ $waittime
)])->hideClose() ?>
<? else: ?>
<p><?= htmlReady($text ?: _('Bitte geben Sie ein gültiges Token ein')) ?></p>
<? if ($secret->type === 'app' && !$secret->confirmed): ?>
- <?= formatReady(Config::get()->TFA_TEXT_APP) ?>
+ <?= formatReady(Config::get()->getValue('TFA_TEXT_APP')) ?>
<p>
<?= _('Scannen Sie diesen Code mit Ihrer App ein und geben Sie '
. 'anschliessend ein gültiges Token ein.') ?>
--
GitLab