mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-16 16:45:07 +00:00
ignacionelson
b96d060ad8
Reported by @choewonwoo1817 as GHSA-wgxf-v8cr-37mj, with a working
end-to-end reproducer against Keycloak.
`where('email', $address)` is not an exact match. It is whatever the
database says equality means, and the collation INSTALL.md tells people to
create — utf8mb4_unicode_ci — folds accents:
administrator@example.com = administrator@éxample.com -> 1
Those are two different domains. The second is xn--xample-9ua.com, which
somebody else can register and honestly verify at an OIDC provider. So an
attacker with no account here could sign in as themselves and be handed
the first account: SocialAuthenticator found it, linked their subject to
it permanently, and started a session. No password, no interaction from
the owner, an administrator session where that account was one.
Comparison now happens in PHP, in one place, on every driver. Case is
still folded because that is a real requirement — addresses are stored
lowercased and a provider may send any case — and mb_strtolower folds case
without folding accents, which is exactly the line to draw.
Three call sites move to it and two deliberately do not. Loose matching is
right when *refusing* and wrong when *selecting*: AvailableEmailRule and
ClientProvisioning ask "is this address free", where a collation that says
no to a near-miss refuses more registrations, which is the safe direction.
The three that ask "which account is this" are the social path, the login
form (where a password still gated it, so it was confusion rather than
takeover) and the erasure command (irreversible, and the wrong row is the
wrong person).
The test story is the part worth reading. The suite runs on SQLite, whose
`=` is byte-exact, so this defect does not exist there and never did —
which is how it survived six releases with everything green. A test
written the obvious way passes on unfixed code. So the comparison is
pinned by driver-independent tests that always run, and the chain is
proved by AccountLookupCollationTest, which skips unless the connection is
MySQL and carries the command to run it. Run against real MySQL with the
real collation: it fails on the old code and passes on the new.
82 lines
3.2 KiB
PHP
82 lines
3.2 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
namespace App\Modules\Identity;
|
|
|
|
use App\Models\User;
|
|
|
|
/**
|
|
* Finding the account that holds an address, exactly.
|
|
*
|
|
* `where('email', $address)` is not an exact match. It is whatever the
|
|
* database's collation says equality means, and the documented one here —
|
|
* `utf8mb4_unicode_ci`, in INSTALL.md and in config/database.php — folds
|
|
* accents:
|
|
*
|
|
* administrator@example.com = administrator@éxample.com -> 1
|
|
*
|
|
* Those are two different domains. `éxample.com` is `xn--xample-9ua.com`,
|
|
* a name somebody else can own and prove they own. So an attacker could
|
|
* register the second at an OIDC provider, verify it honestly, sign in,
|
|
* and be handed the first account — no password, no interaction from its
|
|
* owner, and an administrator session if that account was one
|
|
* (GHSA-wgxf-v8cr-37mj).
|
|
*
|
|
* ### Loose is right when refusing and wrong when selecting
|
|
*
|
|
* The same looseness protects elsewhere and is deliberately left alone.
|
|
* `AvailableEmailRule` and `ClientProvisioning::emailIsAvailable()` ask
|
|
* "is this address free?", and a collation that answers "no" to a
|
|
* near-miss refuses *more* registrations, which is the safe direction.
|
|
* This class is for the other question — "which account is this?" — where
|
|
* matching more than you meant hands somebody an account.
|
|
*
|
|
* ### Why the filtering is in PHP
|
|
*
|
|
* A `COLLATE utf8mb4_bin` in the query would work on MySQL and break
|
|
* everywhere else, and the test suite runs on SQLite, which is byte-exact
|
|
* and would never have shown the bug in the first place. Comparing here
|
|
* gives one answer on every driver, and it is the answer that does not
|
|
* depend on how somebody created their database.
|
|
*
|
|
* Case is still folded, because that is a real requirement rather than an
|
|
* accident: addresses are stored lowercased and a provider may send any
|
|
* case. `mb_strtolower` folds case without folding accents, which is
|
|
* exactly the line to draw.
|
|
*/
|
|
class AccountLookup
|
|
{
|
|
/**
|
|
* The account whose address is exactly this one, or null.
|
|
*
|
|
* @param bool $withTrashed include soft-deleted accounts — a
|
|
* deleted account still holds its address
|
|
* until erasure
|
|
*/
|
|
public function byEmail(string $email, bool $withTrashed = false): ?User
|
|
{
|
|
$query = $withTrashed ? User::withTrashed() : User::query();
|
|
|
|
// The database narrows, this decides. A collation that matches too
|
|
// much returns extra rows here and they are dropped; one that
|
|
// matches too little was never going to return the right row at
|
|
// all, which is a different bug and not one anybody has.
|
|
return $query->where('email', $email)->get()
|
|
->first(fn (User $user): bool => $this->isSameAddress($user->email, $email));
|
|
}
|
|
|
|
/**
|
|
* Whether two strings name the same mailbox: case-insensitively, and
|
|
* byte-exact about everything else.
|
|
*/
|
|
public function isSameAddress(?string $stored, ?string $given): bool
|
|
{
|
|
if ($stored === null || $given === null) {
|
|
return false;
|
|
}
|
|
|
|
return mb_strtolower(trim($stored), 'UTF-8') === mb_strtolower(trim($given), 'UTF-8');
|
|
}
|
|
}
|