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.
69 lines
2.9 KiB
PHP
69 lines
2.9 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Identity\AccountLookup;
|
|
use Illuminate\Support\Facades\DB;
|
|
|
|
/**
|
|
* The one test that can see GHSA-wgxf-v8cr-37mj.
|
|
*
|
|
* The defect was never in PHP: `where('email', $address)` asked the
|
|
* database what equality means, and the documented collation —
|
|
* `utf8mb4_unicode_ci`, in INSTALL.md and in config/database.php — folds
|
|
* accents. `administrator@example.com` and `administrator@éxample.com`
|
|
* compare equal, and those are two different domains: the second is
|
|
* `xn--xample-9ua.com`, which somebody else can own and honestly verify at
|
|
* an OIDC provider.
|
|
*
|
|
* **The suite runs on SQLite, whose `=` is byte-exact, so none of this
|
|
* exists there.** That is why the vulnerability lived through six releases
|
|
* with a green suite, and why this file skips rather than passing: a test
|
|
* that silently proves nothing is worse than one that says it did not run.
|
|
*
|
|
* To run it:
|
|
*
|
|
* docker compose exec -T -e DB_CONNECTION=mysql -e DB_HOST=db \
|
|
* -e DB_DATABASE=collation_check -e DB_USERNAME=root -e DB_PASSWORD=root \
|
|
* app vendor/bin/pest tests/Feature/Identity/AccountLookupCollationTest.php
|
|
*/
|
|
beforeEach(function () {
|
|
if (DB::connection()->getDriverName() !== 'mysql') {
|
|
test()->markTestSkipped('Needs MySQL: SQLite compares byte-exactly and cannot show a collation fault.');
|
|
}
|
|
});
|
|
|
|
test('the database really does fold the accent', function () {
|
|
// Asserted rather than assumed, because everything below is only
|
|
// meaningful if this is true of the connection actually in use. An
|
|
// installation on utf8mb4_bin has never had the bug.
|
|
$folds = DB::selectOne("SELECT _utf8mb4'a@example.com' COLLATE utf8mb4_unicode_ci = _utf8mb4'a@éxample.com' COLLATE utf8mb4_unicode_ci AS c")->c;
|
|
|
|
expect((int) $folds)->toBe(1);
|
|
});
|
|
|
|
test('the raw query matches an address it should not', function () {
|
|
// The defect itself, shown rather than described: this is exactly what
|
|
// SocialAuthenticator used to run.
|
|
User::factory()->create(['email' => 'administrator@example.com']);
|
|
|
|
$found = User::query()->where('email', 'administrator@éxample.com')->first();
|
|
|
|
expect($found)->not->toBeNull('the collation no longer folds — the rest of this file is moot');
|
|
});
|
|
|
|
test('the lookup refuses the address the collation would have accepted', function () {
|
|
// The fix. Same database, same collation, same row present.
|
|
User::factory()->create(['email' => 'administrator@example.com']);
|
|
|
|
expect(app(AccountLookup::class)->byEmail('administrator@éxample.com'))->toBeNull();
|
|
});
|
|
|
|
test('and still finds the real one', function () {
|
|
$user = User::factory()->create(['email' => 'administrator@example.com']);
|
|
|
|
expect(app(AccountLookup::class)->byEmail('administrator@example.com')?->id)->toBe($user->id)
|
|
->and(app(AccountLookup::class)->byEmail('ADMINISTRATOR@Example.com')?->id)->toBe($user->id);
|
|
});
|