From ab5fa2da8b90fda9cd061b53d2a017a8e9e6bbf0 Mon Sep 17 00:00:00 2001 From: ignacionelson Date: Tue, 8 Sep 2026 19:00:46 -0300 Subject: [PATCH] Make Entra prove the address, not just the directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by Dickson Massawe as GHSA-2rfh-v3j2-2jg7. Pinning the tenant was half an answer. It defeats the classic cross-tenant nOAuth, where a stranger's own directory asserts your address, because a foreign tenant carries a different tid. It does nothing about the same attack from inside the pinned tenant: Entra's email claim is user-mutable — a B2B guest's otherMails among its sources — so a colleague or an invited guest could present an administrator's address and have their subject bound to that account. Tenant-pinning answers "which directory said this". It never answered "does this person own that address". xms_edov is Microsoft's own answer to the second, and their guidance says to require it wherever email identifies an account. Absent counts as unverified, which is the only safe reading given it is absent by default. Nobody is locked out by this, which is worth saying because it looked like a breaking change until I read SocialAuthenticator::resolve in order. An account already linked resolves by subject at step 3, before trust is consulted at all — those keep working untouched. A first-time link to an existing account is refused with the message that already exists for exactly this case, which names the way through: sign in with your password and connect the provider from your settings. A brand-new account is still created; it goes to the approval queue rather than auto-approving. The settings screen and docs/testing-social-login.md now tell an operator to add the claim, and there is an upgrade note. The tests exercise fromSocialite() on raw claims, which nothing did before: tests/Feature/Auth/SocialLoginTest.php builds a SocialIdentity by hand and so never reaches this mapping. That is how the branch could trust a tenant match alone with a full suite passing. --- CHANGELOG.md | 20 ++++ .../Identity/Social/SocialIdentity.php | 37 ++++++- .../js/pages/system/settings/social-login.tsx | 5 + .../Feature/Identity/SocialEmailTrustTest.php | 104 ++++++++++++++++++ 4 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/Identity/SocialEmailTrustTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index d2a3bbdb..23e5bf45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,17 @@ when a version is cut. **Fixed** +- **Microsoft sign-in now needs one more claim before it will trust an address.** Naming your tenant + says which directory vouched for a sign-in; it never said the person owns the address they + presented. Inside your own tenant a member or an invited guest could present a colleague's — an + administrator's — and have their Microsoft account attached to it. ProjectSend now also requires + the `xms_edov` claim, which is Microsoft's own answer to that question. + + *Who this affected:* only installations with Microsoft sign-in enabled. **There is something to do + — see the upgrade notes.** Nobody is locked out: accounts already linked to Microsoft keep working + untouched, because they are matched on the account itself rather than on the address. + + Reported by Dickson Massawe. - **Changing your own email address now asks for your password.** It did not, and that address is where a password reset is sent — so anybody who got hold of a signed-in session could point the account at their own inbox, request a reset, and keep the account for good. Deleting your account @@ -48,6 +59,15 @@ when a version is cut. or `1` — including `no`, `off`, and a misspelling — used to switch the CAPTCHA off on the login and registration forms. Only an explicit `true` or `1` does now; everything else leaves it on. +### Upgrade notes + +- **If you use Microsoft sign-in, add the `xms_edov` optional claim to your app registration.** In + the Entra portal: your app registration → Token configuration → Add optional claim → ID → + `xms_edov`. Until you do, Microsoft sign-in keeps working and keeps creating new accounts, but it + will no longer attach itself to an account that already exists — the person is told to sign in + with their password and connect Microsoft from their settings instead. People already signed in + with Microsoft are not affected. + ## 2.4.0 — 8 September 2026 diff --git a/app/Modules/Identity/Social/SocialIdentity.php b/app/Modules/Identity/Social/SocialIdentity.php index 631765bc..80b99e16 100644 --- a/app/Modules/Identity/Social/SocialIdentity.php +++ b/app/Modules/Identity/Social/SocialIdentity.php @@ -39,8 +39,40 @@ final readonly class SocialIdentity * | LinkedIn | `email_verified` claim | * | OpenID Connect | `email_verified` claim, from the ID token | * | GitHub | An address at all — Socialite's GithubProvider replaces `email` with the result of `getEmailByToken()`, which only ever returns one that is **primary and verified** | - * | Microsoft | The token's `tid` matching the configured tenant. Entra does not emit a usable `email_verified`, and its `email` claim is user-mutable — pinning the tenant is what makes it mean anything (this is the *nOAuth* class of bug) | + * | Microsoft | The token's `tid` matching the configured tenant **and** `xms_edov` — see below | * | Facebook | Nothing. The Graph API has no equivalent claim, so an address from Facebook is never treated as verified | + * + * ### Microsoft takes two claims, not one + * + * Entra emits no usable `email_verified`, and its `email` claim is + * user-mutable — populated from `otherMails`/proxyAddresses for a B2B + * guest, among other places. Pinning the tenant was the first answer + * and it is half of one: it defeats the classic cross-tenant *nOAuth*, + * where a stranger's own tenant asserts your address, because a + * foreign tenant carries a different `tid`. + * + * It does nothing about the same attack from *inside* the pinned + * tenant. A colleague, or a guest somebody invited, could shape their + * `email` claim to an administrator's address and have their subject + * bound to that account (GHSA-2rfh-v3j2-2jg7). Tenant-pinning answers + * "which directory said this", never "does this person own that + * address". + * + * `xms_edov` is Microsoft's own answer to the second question — the + * optional claim meaning the tenant has verified it owns the email's + * domain — and their guidance says to require it wherever `email` + * identifies an account. Absent is treated as unverified, which is the + * only safe reading: it is absent by default, so anything else would + * be no check at all. + * + * **What an installation has to do.** The claim must be added to the + * app registration (Token configuration → optional claims → `xms_edov` + * on the ID token). Until it is, Microsoft sign-in still works and + * still creates new accounts — it simply stops silently attaching + * itself to accounts that already exist, and says so, pointing the + * person at signing in with a password and connecting the provider + * from their settings. Accounts already linked are unaffected: they + * resolve by subject, before this is consulted at all. */ public static function fromSocialite( SocialProvider $provider, @@ -72,7 +104,8 @@ final readonly class SocialIdentity || ($raw['email_verified'] ?? null) === 'true', SocialProvider::Github => true, SocialProvider::Microsoft => is_string($settings->tenant_id) - && ($raw['tid'] ?? null) === $settings->tenant_id, + && ($raw['tid'] ?? null) === $settings->tenant_id + && (($raw['xms_edov'] ?? null) === true || ($raw['xms_edov'] ?? null) === 'true'), SocialProvider::Facebook => false, }, name: is_string($user->getName()) && trim($user->getName()) !== '' diff --git a/resources/js/pages/system/settings/social-login.tsx b/resources/js/pages/system/settings/social-login.tsx index 79c42bf0..ec9b183f 100644 --- a/resources/js/pages/system/settings/social-login.tsx +++ b/resources/js/pages/system/settings/social-login.tsx @@ -197,6 +197,11 @@ function ProviderCard({ provider, open, onToggle }: { provider: ProviderSettings 'Your own tenant, not "common". Microsoft lets a user change the email address on their account, so a sign-in is only trustworthy when the token came from the tenant you named here.', )}

+

+ {t( + 'Also add the "xms_edov" optional claim to your app registration, under Token configuration. Naming the tenant says which directory vouched for the sign-in; that claim says the directory checked the person really owns the address. Without it, someone else inside your tenant could sign in with a colleague\'s address, so ProjectSend will create new accounts but never attach a Microsoft sign-in to an account that already exists.', + )} +

)} diff --git a/tests/Feature/Identity/SocialEmailTrustTest.php b/tests/Feature/Identity/SocialEmailTrustTest.php new file mode 100644 index 00000000..01515910 --- /dev/null +++ b/tests/Feature/Identity/SocialEmailTrustTest.php @@ -0,0 +1,104 @@ + $claims */ +function microsoftIdentity(array $claims, ?string $tenantId = TENANT): ?SocialIdentity +{ + $user = new SocialiteUser; + $user->map(['id' => 'attacker-subject', 'email' => $claims['email'] ?? null, 'name' => 'Someone']); + $user->setRaw($claims); + + $settings = new SocialSettings(['provider' => SocialProvider::Microsoft->value, 'tenant_id' => $tenantId]); + + return SocialIdentity::fromSocialite(SocialProvider::Microsoft, $user, $settings); +} + +/* +|-------------------------------------------------------------------------- +| Entra: a tenant match is not a verified address +|-------------------------------------------------------------------------- +| +| Pinning the tenant defeats the classic cross-tenant nOAuth, and that is +| all it defeats. Inside the tenant, `email` is user-mutable and can be +| populated from unverified sources — a B2B guest's otherMails among them +| — so a colleague or an invited guest could present the victim's address. +| Microsoft's own guidance names the control: xms_edov. +*/ + +test('a tenant match alone is not enough', function () { + $identity = microsoftIdentity([ + 'tid' => TENANT, + 'email' => 'admin@example.test', + ]); + + expect($identity?->emailVerified)->toBeFalse(); +}); + +test('the domain-owner claim is what makes it verified', function () { + $identity = microsoftIdentity([ + 'tid' => TENANT, + 'email' => 'admin@example.test', + 'xms_edov' => true, + ]); + + expect($identity?->emailVerified)->toBeTrue(); +}); + +test('the claim is read when Entra sends it as a string', function () { + // Optional claims have been observed arriving as strings rather than + // JSON booleans, and the branch beside this one already allows for the + // same on email_verified. + expect(microsoftIdentity(['tid' => TENANT, 'email' => 'a@example.test', 'xms_edov' => 'true'])?->emailVerified) + ->toBeTrue(); +}); + +test('a false or junk claim is not verified', function (mixed $value) { + expect(microsoftIdentity(['tid' => TENANT, 'email' => 'a@example.test', 'xms_edov' => $value])?->emailVerified) + ->toBeFalse(); +})->with([ + 'false' => [false], + '"false"' => ['false'], + 'null' => [null], + 'empty' => [''], + 'a typo' => ['ture'], + 'zero' => [0], +]); + +test('the domain-owner claim alone, from the wrong tenant, is not enough', function () { + // Both halves. xms_edov says the tenant owns the domain; the tid says + // it is *our* tenant. A foreign tenant that owns its own domain is + // still a stranger. + $identity = microsoftIdentity([ + 'tid' => 'some-other-tenant', + 'email' => 'admin@example.test', + 'xms_edov' => true, + ]); + + expect($identity?->emailVerified)->toBeFalse(); +}); + +test('an installation with no tenant pinned verifies nothing', function () { + expect(microsoftIdentity(['tid' => TENANT, 'email' => 'a@example.test', 'xms_edov' => true], tenantId: null)?->emailVerified) + ->toBeFalse(); +});