Skip to content

Commit 4c59e6b

Browse files
committed
Prevent bypassing 2fa by changing username case
When using postgres db as authentication backend it was possible to bypass 2fa by using e.g. `ADMIN` instead of `admin` as username. This is now prevented by: 1. Ensuring there is only one secret per user no matter the case 2. Lowering all usernames for filtering in the database
1 parent 01edbd1 commit 4c59e6b

4 files changed

Lines changed: 27 additions & 51 deletions

File tree

library/Icinga/Authentication/TwoFactorTotp.php

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,10 @@
88
use Endroid\QrCode\Builder\Builder;
99
use Icinga\Common\Database;
1010
use Icinga\Exception\ConfigurationError;
11-
use Icinga\Model\TwoFactorModel;
1211
use ipl\Sql\Connection;
1312
use ipl\Sql\Delete;
1413
use ipl\Sql\Insert;
15-
use ipl\Stdlib\Filter;
14+
use ipl\Sql\Select;
1615
use OTPHP\TOTP;
1716
use PDOException;
1817
use Throwable;
@@ -75,10 +74,14 @@ public static function createFromSecret(string $secret, string $user): static
7574
*/
7675
public static function loadFromDb(Connection $db, string $user): ?static
7776
{
78-
$dbTwoFactor = TwoFactorModel::on($db)->filter(Filter::equal('username', $user))->first();
77+
$select = (new Select())
78+
->from('icingaweb_2fa')
79+
->columns('secret')
80+
->where(['LOWER(username) = ?' => strtolower($user)]);
7981

80-
/** @var TwoFactorModel|null $dbTwoFactor */
81-
if ($dbTwoFactor === null) {
82+
$dbTwoFactor = $db->select($select)->fetch();
83+
84+
if (! $dbTwoFactor) {
8285
return null;
8386
}
8487

@@ -96,7 +99,12 @@ public static function loadFromDb(Connection $db, string $user): ?static
9699
public static function hasDbSecret(Connection $db, string $user): bool
97100
{
98101
try {
99-
return TwoFactorModel::on($db)->filter(Filter::equal('username', $user))->first() !== null;
102+
$select = (new Select())
103+
->from('icingaweb_2fa')
104+
->columns('username')
105+
->where(['LOWER(username) = ?' => strtolower($user)]);
106+
107+
return ! empty($db->select($select)->fetchAll());
100108
} catch (PDOException) {
101109
return false;
102110
}
@@ -196,7 +204,7 @@ public function removeFromDb(): void
196204
$db->prepexec(
197205
(new Delete())
198206
->from('icingaweb_2fa')
199-
->where(['username = ?' => $this->user])
207+
->where(['LOWER(username) = ?' => strtolower($this->user)])
200208
);
201209
$db->commitTransaction();
202210
} catch (Throwable $e) {

library/Icinga/Model/TwoFactorModel.php

Lines changed: 0 additions & 44 deletions
This file was deleted.

schema/pgsql-upgrades/2.13.0.sql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,9 @@ CREATE TABLE "icingaweb_2fa" (
44
"ctime" bigint,
55
CONSTRAINT pk_icingaweb_2fa PRIMARY KEY ("username")
66
);
7+
8+
CREATE UNIQUE INDEX idx_icingaweb_2fa
9+
ON "icingaweb_2fa"
10+
USING btree (
11+
lower((username)::text)
12+
);

schema/pgsql.schema.sql

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,5 +138,11 @@ CREATE TABLE "icingaweb_2fa" (
138138
CONSTRAINT pk_icingaweb_2fa PRIMARY KEY ("username")
139139
);
140140

141+
CREATE UNIQUE INDEX idx_icingaweb_2fa
142+
ON "icingaweb_2fa"
143+
USING btree (
144+
lower((username)::text)
145+
);
146+
141147
INSERT INTO icingaweb_schema (version, timestamp, success)
142148
VALUES ('2.12.0', extract(epoch from now()) * 1000, 'y');

0 commit comments

Comments
 (0)