mirror of
https://github.com/projectsend/projectsend.git
synced 2026-09-17 09:05:08 +00:00
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.
177 lines
7.0 KiB
PHP
177 lines
7.0 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
namespace App\Modules\Identity\Social;
|
|
|
|
use App\Models\User;
|
|
use App\Modules\Identity\AccountLookup;
|
|
|
|
/**
|
|
* Which local account, if any, a provider identity signs in as.
|
|
*
|
|
* This class is the feature. Everything else moves bytes; this decides
|
|
* whether an assertion made by a third party is allowed to become access
|
|
* to somebody's files, and it is the exact thing v1 got wrong:
|
|
*
|
|
* SELECT * FROM users WHERE user = :email OR email = :email
|
|
*
|
|
* — whatever address the provider returned, matched against two columns,
|
|
* with no check that the provider had ever verified it and no restriction
|
|
* on the account type it landed on. An identity provider permitting self
|
|
* registration was therefore enough to become an administrator here.
|
|
*
|
|
* The order below is what replaces it. Read it as: the *subject* is the
|
|
* identity; the email is only ever a one-time introduction, and only from
|
|
* a provider willing to vouch for it.
|
|
*/
|
|
class SocialAuthenticator
|
|
{
|
|
public function __construct(
|
|
private readonly SocialProvisioner $provisioner,
|
|
private readonly AccountLookup $accounts,
|
|
) {}
|
|
|
|
public function resolve(SocialSettings $settings, SocialIdentity $identity): SocialResolution
|
|
{
|
|
// 1. A provider that is off, or half-configured, behaves exactly
|
|
// as one that was never added.
|
|
if (! $settings->usable()) {
|
|
return SocialResolution::refuse(__('That sign-in method is not available.'));
|
|
}
|
|
|
|
// 2. Nothing here can be keyed on an identity with no address.
|
|
if ($identity->email === null) {
|
|
return SocialResolution::refuse(
|
|
__(':provider did not provide an email address, which is required to sign in.', [
|
|
'provider' => $settings->provider->label(),
|
|
])
|
|
);
|
|
}
|
|
|
|
if (! $settings->allowsDomain($identity->email)) {
|
|
return SocialResolution::refuse(
|
|
__('Accounts at :domain cannot sign in with :provider here.', [
|
|
'domain' => substr($identity->email, (int) strrpos($identity->email, '@') + 1),
|
|
'provider' => $settings->provider->label(),
|
|
])
|
|
);
|
|
}
|
|
|
|
// 3. An existing link. The subject, not the address — so somebody
|
|
// who changes their email at the provider still lands on their
|
|
// own account, and somebody who changes it *to yours* does not
|
|
// land on yours.
|
|
$linked = SocialAccount::resolve($identity);
|
|
|
|
if ($linked !== null) {
|
|
$this->refreshLink($identity);
|
|
|
|
return SocialResolution::existing($linked);
|
|
}
|
|
|
|
// Whether this provider's word on the address is good enough to
|
|
// act on. `require_verified_email` off is an administrator
|
|
// explicitly accepting that it is not — the escape hatch for a
|
|
// directory that omits the claim.
|
|
$trusted = $identity->emailVerified || ! $settings->require_verified_email;
|
|
|
|
// Exactly this address, not whatever the database's collation
|
|
// calls equal. utf8mb4_unicode_ci folds accents, so a verified
|
|
// sign-in as administrator@éxample.com — a domain somebody else
|
|
// can own — selected administrator@example.com and this method
|
|
// then linked the attacker's subject to it (GHSA-wgxf-v8cr-37mj).
|
|
$existing = $this->accounts->byEmail($identity->email);
|
|
|
|
if ($existing !== null) {
|
|
// 4/5. The takeover, refused. An unverified address may not
|
|
// reach an account that already exists — of any type, but
|
|
// note that an administrator's is the interesting case.
|
|
if (! $trusted) {
|
|
return SocialResolution::refuse(
|
|
__('An account already uses this email address, and :provider did not confirm that you own it. Sign in with your password and connect :provider from your settings instead.', [
|
|
'provider' => $settings->provider->label(),
|
|
])
|
|
);
|
|
}
|
|
|
|
return $this->link($existing, $identity) === null
|
|
? SocialResolution::refuse(__('That sign-in method is not available.'))
|
|
: SocialResolution::linked($existing);
|
|
}
|
|
|
|
// 6. Nobody here at all.
|
|
$provisioned = $this->provisioner->provision(
|
|
$settings,
|
|
$identity,
|
|
// An address nobody vouched for can still become a *new*
|
|
// account — it takes nothing over — but it goes to the
|
|
// approval queue whatever auto_approve says, so a person sees
|
|
// it before it becomes access.
|
|
autoApprove: $trusted && $settings->auto_approve,
|
|
);
|
|
|
|
if ($provisioned === null) {
|
|
return SocialResolution::refuse(__('There is no account here for that address.'));
|
|
}
|
|
|
|
$this->link($provisioned, $identity);
|
|
|
|
return SocialResolution::provisioned($provisioned);
|
|
}
|
|
|
|
/**
|
|
* Bind an identity to an account, deliberately and once.
|
|
*
|
|
* Also reachable from the Connected accounts screen, where the person
|
|
* is already signed in and the address plays no part at all — which
|
|
* is the safest way to connect a provider that cannot verify one.
|
|
*
|
|
* Returns null when this identity already belongs to somebody else.
|
|
* Moving it would not hand over their account, since whoever holds
|
|
* the identity can already use it, but silently detaching another
|
|
* person's sign-in method is not something a login flow should do
|
|
* without saying so.
|
|
*/
|
|
public function link(User $user, SocialIdentity $identity): ?SocialAccount
|
|
{
|
|
$boundElsewhere = SocialAccount::query()
|
|
->where('provider', $identity->provider->value)
|
|
->where('provider_user_id', $identity->subject)
|
|
->where('user_id', '!=', $user->getKey())
|
|
->exists();
|
|
|
|
if ($boundElsewhere) {
|
|
return null;
|
|
}
|
|
|
|
// Keyed on the account and provider, so reconnecting a different
|
|
// Google account replaces the old row rather than colliding with
|
|
// the one-link-per-provider constraint.
|
|
/** @var SocialAccount */
|
|
return SocialAccount::query()->updateOrCreate(
|
|
[
|
|
'user_id' => $user->getKey(),
|
|
'provider' => $identity->provider->value,
|
|
],
|
|
[
|
|
'provider_user_id' => $identity->subject,
|
|
'email' => $identity->email,
|
|
],
|
|
);
|
|
}
|
|
|
|
/**
|
|
* Keep the displayed address current without letting it mean
|
|
* anything: this is what the Connected accounts screen shows, not
|
|
* what any decision is made on.
|
|
*/
|
|
private function refreshLink(SocialIdentity $identity): void
|
|
{
|
|
SocialAccount::query()
|
|
->where('provider', $identity->provider->value)
|
|
->where('provider_user_id', $identity->subject)
|
|
->update(['email' => $identity->email, 'updated_at' => now()]);
|
|
}
|
|
}
|