Docs should make clear that setting `oidc_providers.allow_existing_users: true` has security implications

The oidc_providers.allow_existing_users documentation is pretty sparse:

allow_existing_users: set to true to allow a user logging in via OIDC to match a pre-existing account instead of failing. This could be used if switching from password logins to OIDC. Defaults to false.

The way the sample configurations do this makes assumptions contrary to OIDC claim stability guidelines. If a Synapse instance was configured in this way, it could allow account take-overs[^1] by an attacker with an unknown-to-Synapse IdP account.

The documentation should describe the security implications of enabling the option.

While there are some weaknesses in Synapse that make this possible at all, the ability to exploit it is mostly configuration dependant – so I feel this is a "security-related docs bug" rather than "docs-related security bug".

[^1]: If a user has enabled end-to-end encryption and uses it in all channels, I suspect it would prevent an attacker from accessing prior messages or impersonating the user on those channels.

Background

From OpenID Connect Basic Client Implementer's Guide 1.0, Section 2.5.3, Claim Stability and Uniqueness, emphasis added:

The sub (subject) and iss (issuer) Claims, used together, are the only Claims that an RP can rely upon as a stable identifier for the End-User, since the sub Claim MUST be locally unique and never reassigned within the Issuer for a particular End-User, as described in Section 2.2. Therefore, the only guaranteed unique identifier for a given End-User is the combination of the iss Claim and the sub Claim.

All other Claims carry no such guarantees across different issuers in terms of stability over time or uniqueness across users, and Issuers are permitted to apply local restrictions and policies. For instance, an Issuer MAY re-use an email Claim Value across different End-Users at different points in time, and the claimed email address for a given End-User MAY change over time. Therefore, other Claims such as email, phone_number, and preferred_username and MUST NOT be used as unique identifiers for the End-User.

By default, Synapse uses the sub claim to uniquely identify a user, which is in-line with OIDC recommendations.

Detail

Setting oidc_providers.allow_existing_users: true triggers an extra code path in grandfather_existing_users() on login if lookup by external user ID fails.

This maps an OIDC user to a Matrix user based on localpart_template:

https://github.com/element-hq/synapse/blob/2ce7a1edf7291cfda14bb975135eb7c989d910bc/synapse/handlers/oidc.py#L1247-L1255

If there is an existing Matrix user which maps, the new sub is added to the database:

https://github.com/element-hq/synapse/blob/2ce7a1edf7291cfda14bb975135eb7c989d910bc/synapse/handlers/sso.py#L474-L479

As there doesn't seem to be any further authentication (such as requesting a user login with a password, or with another configured external identity provider), my working assumption (from static analysis) is that enabling allow_existing_users allows an unknown-to-Synapse OIDC user to permanently link their OIDC identity to an existing Matrix account that matches by localpart_template.

I couldn't find anywhere that branch checks to see if a matched account already has an user_external_id associated with it (only that an external ID can't be re-used), so it looks like this would allow two OIDC users to "claim" the same Matrix account.

One caveat is that if an instance sets enable_registration: true (the default), an attacker would need a new IdP account for every attempt they made to take over an account.

The `localpart_template`

The impact of the issue has depends on the localpart_template.

There is no default localpart_template, but the example OIDC configs doc have some suggestions, and many of them look insecure:

Entra ID (Azure AD): Using everything before an @ character in preferred_username.

Entra ID sets the preferred_username to the account email address, so this would treat user@example.com and user@example.net as the same user.

Entra ID also has a tenancy ID attribute, but this doesn't seem to be used.
Django OAuth Toolkit: Using everything before an @ character in email.

Similar issue to Entra ID.
Shibboleth: Using everything before an @ character in user.sub, and also sets allow_existing_users: true.

Shibboleth has custom mapping rules for sub, but this example seems to suggest sub is an email-like field.

If it is an email field, then this would have a similar issue to Entra ID, with an insecure-by-default config.
Auth0, Authentik, Keycloak, LemonLDAP: Using preferred_username as-is – this is an unstable claim.
Twitch: Using preferred_username as-is.

This is the display name, and can be changed by users, or recycled by Twitch.
GitHub, GitLab, Gitea, Mastodon, Twitter: Using user.login / user.nickname / user.username as-is.

Those accounts may be renamed.
Google: Using user.given_name as-is.

This may be freely changed by the user.

If an attacker with an unknown-to-Synapse IdP account can discover a Synapse server's localpart_template, and find that it is under user control, they could exploit a server with oidc_providers.allow_existing_users: true:

Identify a target user's localpart (this could be learned by federation).
Get a new account on the Synapse instance's IdP. If enable_registration: true (the default), an attacker would need a new IdP account for each takeover attempt, though this is a low bar for a public identity provider.
Attacker sets the field used in localpart_template to match the target user.
Attacker logs in to the target's Synapse instance with OIDC for the first time.
Synapse links the attacker's external user ID to the target account.
Attacker is now authenticated as the target user.

It's not clear to me (from static analysis) whether a user can discover what external user IDs are associated with their account; though it would appear as another unverified session (as they wouldn't have the E2EE keys).

What could change in the docs?

oidc_config.allow_existing_users should describe how it maps existing users (with localpart_template), and that enabling the option has significant security implications.

It should also note that enabling the option adds extra constraints to any claim used by localpart_template:

the identifier must be unique among all users, stable and never re-issued
violating those additional constraints may allow account take-overs
it's best to set up a custom claim (eg: matrix_username) in the IdP to map users, rather than using other, potentially-unstable identifiers (like email or username)

What could change in Synapse?

When "grandfathering" existing Matrix user accounts, Synapse should also authenticate the user by some other method, even if those methods are not normally available (eg: password backend). This would prevent account take-overs during the migration process.

There should be a way to prevent more than one external user ID being assigned to the same Matrix user. This would prevent account take-overs for accounts which are already migrated to OIDC.

element-hq / synapse